Differential D11649

Fix crash in the file slave
Changes PlannedPublic
Actions

Authored by aacid on Mar 24 2018, 4:23 PM.

Details

Reviewers

dfaure

Group Reviewers

Frameworks

Summary

When doing a KIO::storedPut to a filename that has a .part file existing there were two problems:

We were never answering to the canResume signal, so it was crashing in waitForAnswer because we didn't get the proper answer
We were sending the data on start unconditionally, which means when the slave asked for canResume it had the wrong answer

The line removed in start seems not to be needed anymore, tests pass without it and KDE Connect that is one of the main users of this still seems to work.

I'm not totally sure it's the best of the ideas adding the handling of canResume to StoredTransferJob since users of it may already have implemented.

To be honest I'm not totally sure this is the totally correct fix. Comments more than welcome.

Test Plan

Without the code changes the newly added test makes the file ioslave crash

Diff Detail

Repository

R241 KIO

Branch

master

Lint

No Linters Available

Unit

No Unit Test Coverage

aacid created this revision.Mar 24 2018, 4:23 PM

Restricted Application added a project: Frameworks. · View Herald TranscriptMar 24 2018, 4:23 PM

aacid requested review of this revision.Mar 24 2018, 4:23 PM

nicolasfella added a subscriber: KDE Connect.Mar 28 2018, 4:47 PM

canResume is an "internal" signal, I don't think any apps can do anything meaningful with it, so I don't mind you connecting to it, except that I think this is the wrong layer...

BTW did you notice the section about resuming in docs/design.txt? It might help when thinking about all this.

autotests/jobtest.cpp
421	That doesn't describe the behaviour anymore, after the fix ;) Can you find a more descriptive name for which case this is testing? E.g. I'm not sure why there are two storedPut jobs on the same data -- to test data available after start and data available before start? But then why not fill in putDataBuffer2 upfront, rather than after 200ms? For readability/clarity, I mean.
440	Apps (and unittests) are supposed to connect to `result`, not `finished` (which is only for progress information to go away)
462	Maybe do this inside the 200ms lambda, to 1) make sure it happens afterwards (200<400 but QTimer can decide to do both together), and 2) shorten the delay to make the test run faster?
467	QVERIFY(job->exec()); would be shorter, removing the need for the first lambda, no? Or if you want to avoid an infinite hang if the job doesn't finish, QSignalSpy on result + QVERIFY(spy.wait()). I'm nitpicking, it's just that this form is a bit unusual and a bit more verbose.
src/core/storedtransferjob.cpp
98	The "Stored" in StoredTransferJob is just convenience (it stores the data for you), that should have no relation to the handling of resume. So I think this should be moved up to the base class TransferJob.

aacid planned changes to this revision.Aug 16 2018, 8:45 AM

Restricted Application added a subscriber: kde-frameworks-devel. · View Herald TranscriptAug 16 2018, 8:45 AM

Revision Contents
Changeset List

			Path	Packages
M			autotests/jobtest.h (1 line)
M			autotests/jobtest.cpp (51 lines)
M			src/core/slavebase.cpp (2 lines)
M			src/core/storedtransferjob.cpp (7 lines)
M			src/core/transferjob.cpp (2 lines)