D25117: file ioslave: stop copying as soon as the ioslave is killed

meven added a task: T11627: Improve KIO asynchronicity. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D25117 To: meven, #frameworks, dfaure, ngraham, apol Cc: apol, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D25117: file ioslave: stop copying as soon as the ioslave is killed

This revision was automatically updated to reflect the committed changes. Closed by commit R241:8a834785c875: file ioslave: stop copying as soon as the ioslave is killed (authored by meven). REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D25117?vs=74870=74895

D25117: file ioslave: stop copying as soon as the ioslave is killed

dfaure added a comment. I wouldn't worry about the performance impact of the added if() statement, we're just testing a bool testMode which is always false (so the call to contains() will never happen outside tests) and the branch predictor will find out as soon as the second time, that

D25117: file ioslave: stop copying as soon as the ioslave is killed

dfaure accepted this revision. This revision is now accepted and ready to land. REPOSITORY R241 KIO BRANCH arcpatch-D25117 REVISION DETAIL https://phabricator.kde.org/D25117 To: meven, #frameworks, dfaure, ngraham, apol Cc: apol, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham,

D25117: file ioslave: stop copying as soon as the ioslave is killed

meven marked 2 inline comments as done. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D25117 To: meven, #frameworks, dfaure, ngraham, apol Cc: apol, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D25117: file ioslave: stop copying as soon as the ioslave is killed

meven added inline comments. INLINE COMMENTS > meven wrote in jobtest.cpp:82 > It'd be nice to have an #idef BUILD_TEST to only include this code path only > when tests are compiled. > I didn't know how to achieve this. But then it could cause difficulties for downstream distros, running tests

D25117: file ioslave: stop copying as soon as the ioslave is killed

meven added inline comments. INLINE COMMENTS > jobtest.cpp:82 > +// to make sure io is not too fast > +qputenv("KIOSLAVE_FILE_ENABLE_TESTMODE", "1"); > + It'd be nice to have an #idef BUILD_TEST to only include this code path only when tests are compiled. I didn't know how to achieve

D25117: file ioslave: stop copying as soon as the ioslave is killed

meven updated this revision to Diff 74870. meven marked 3 inline comments as done. meven added a comment. Reduce the size of the tempfile for JobTest::cancelCopyAndCleanDest and restrict testMode slowness to copy where destination is slow REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE

D25117: file ioslave: stop copying as soon as the ioslave is killed

dfaure added inline comments. INLINE COMMENTS > meven wrote in jobtest.cpp:2045 > Making an ioslave reading an env var for testing purposes is not a great > alternative. Does this mean the file could be made smaller again? (to reduce disk-space requirements) > file_unix.cpp:317 > + > +

D25117: file ioslave: stop copying as soon as the ioslave is killed

meven updated this revision to Diff 74570. meven added a comment. rebasing REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D25117?vs=74543=74570 BRANCH arcpatch-D25117 REVISION DETAIL https://phabricator.kde.org/D25117 AFFECTED FILES

D25117: file ioslave: stop copying as soon as the ioslave is killed

meven updated this revision to Diff 74543. meven added a comment. Add an env variable KIOSLAVE_FILE_ENABLE_TESTMODE to slow down copy file operations to make tests reliable REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D25117?vs=74542=74543 BRANCH

D25117: file ioslave: stop copying as soon as the ioslave is killed

meven added inline comments. INLINE COMMENTS > jobtest.cpp:2077 > +} else { > +QVERIFY(QFile::exists(destToCheck)); > +} This is not right. It might hide a bug. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D25117 To: meven, #frameworks, dfaure, ngraham,

D25117: file ioslave: stop copying as soon as the ioslave is killed

meven planned changes to this revision. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D25117 To: meven, #frameworks, dfaure, ngraham, apol Cc: apol, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D25117: file ioslave: stop copying as soon as the ioslave is killed

meven added a comment. In D25117#602227 , @meven wrote: > Fix test I was missing the case when the dest file existed before copying. I think this is ready for landing. REPOSITORY R241 KIO REVISION DETAIL

D25117: file ioslave: stop copying as soon as the ioslave is killed

meven updated this revision to Diff 74542. meven marked 6 inline comments as done. meven added a comment. Fix test REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D25117?vs=72832=74542 BRANCH arcpatch-D25117 REVISION DETAIL

D25117: file ioslave: stop copying as soon as the ioslave is killed

dfaure added a comment. I don't see a problem with that. It's common to have activate a "test mode". In Qt (single process) there are tons of internal variables exported and used with "extern" in unittests. In multi-process scenarios we need cmdline args or env vars. REPOSITORY R241 KIO

D25117: file ioslave: stop copying as soon as the ioslave is killed

meven added inline comments. INLINE COMMENTS > dfaure wrote in jobtest.cpp:2045 > The alternative is to export an env var that makes the ioslave call some > msleep()... Making an ioslave reading an env var for testing purposes is not a great alternative. REPOSITORY R241 KIO REVISION

D25117: file ioslave: stop copying as soon as the ioslave is killed

dfaure added inline comments. INLINE COMMENTS > meven wrote in jobtest.cpp:2045 > This may-be excessive, but I needed that since I have a fast nvme drive. The alternative is to export an env var that makes the ioslave call some msleep()... REPOSITORY R241 KIO REVISION DETAIL

D25117: file ioslave: stop copying as soon as the ioslave is killed

meven added a comment. There is still an issue with the tests : It won't pass in wondws. So I am thinking duplicating the test `JobTest::cancelCopyAndCleanDest` and using an #ifdef to circumvent windows limitations. REPOSITORY R241 KIO REVISION DETAIL

D25117: file ioslave: stop copying as soon as the ioslave is killed

meven marked an inline comment as done. meven added a comment. Thanks for the suggestion @dfaure INLINE COMMENTS > jobtest.cpp:2045 > f.close(); > -QCOMPARE(f.size(), 1000); //~10MB > +QCOMPARE(f.size(), 5); //~500MB to make sure copy is not too > fast, or the tests

D25117: file ioslave: stop copying as soon as the ioslave is killed

meven updated this revision to Diff 72832. meven added a comment. Use QTRY_VERIFY instead of sleep + Q_VERIFY REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D25117?vs=72798=72832 BRANCH arcpatch-D25117_1 REVISION DETAIL https://phabricator.kde.org/D25117

D25117: file ioslave: stop copying as soon as the ioslave is killed

dfaure requested changes to this revision. dfaure added inline comments. This revision now requires changes to proceed. INLINE COMMENTS > jobtest.cpp:2075 > +QThread::msleep(500); > +QVERIFY(!QFile::exists(destToCheck)); > } How about QTRY_VERIFY instead of a hardcoded 500ms sleep?

D25117: file ioslave: stop copying as soon as the ioslave is killed

meven updated this revision to Diff 72798. meven added a comment. Stabilise test REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D25117?vs=72729=72798 BRANCH arcpatch-D25117_1 REVISION DETAIL https://phabricator.kde.org/D25117 AFFECTED FILES

D25117: file ioslave: stop copying as soon as the ioslave is killed

meven added a comment. I have to check on the tests. INLINE COMMENTS > dfaure wrote in jobtest.cpp:2171 > Why the short 500ms timeout? CI can be slow sometimes. It was the previous value used line 2176 for the same use, except the file cleaning will now be more immediate. REPOSITORY

D25117: file ioslave: stop copying as soon as the ioslave is killed

dfaure accepted this revision. This revision is now accepted and ready to land. REPOSITORY R241 KIO BRANCH arcpatch-D25117_1 REVISION DETAIL https://phabricator.kde.org/D25117 To: meven, #frameworks, dfaure, ngraham, apol Cc: apol, kde-frameworks-devel, LeGast00n, GB_2, michaelh,

D25117: file ioslave: stop copying as soon as the ioslave is killed

meven updated this revision to Diff 72729. meven added a comment. Use more appropriate error(KIO::ERR_USER_CANCELED) REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D25117?vs=70401=72729 BRANCH arcpatch-D25117_1 REVISION DETAIL

D25117: file ioslave: stop copying as soon as the ioslave is killed

dfaure requested changes to this revision. dfaure added inline comments. This revision now requires changes to proceed. INLINE COMMENTS > dfaure wrote in jobtest.cpp:2171 > Always use QVERIFY() around spy.wait(). > Well, I'm assuming we actually expect the signal to be emitted :-) > I would also

D25117: file ioslave: stop copying as soon as the ioslave is killed

meven added a comment. ping REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D25117 To: meven, #frameworks, dfaure, ngraham, apol Cc: apol, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D25117: file ioslave: stop copying as soon as the ioslave is killed

meven updated this revision to Diff 70401. meven added a comment. Let windows code path not being changed, add a todo for windows support REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D25117?vs=70102=70401 BRANCH arcpatch-D25117 REVISION DETAIL

D25117: file ioslave: stop copying as soon as the ioslave is killed

meven added a comment. In D25117#565877 , @dfaure wrote: > Nice, I didn't realize the slave had 5 seconds to cleanup after being killed, I thought it died immediately > (genericsig_handler in slavebase.cpp). I didn't realize there was

D25117: file ioslave: stop copying as soon as the ioslave is killed

dfaure requested changes to this revision. dfaure added a comment. This revision now requires changes to proceed. Nice, I didn't realize the slave had 5 seconds to cleanup after being killed, I thought it died immediately (genericsig_handler in slavebase.cpp). Hmm, the Windows code is

D25117: file ioslave: stop copying as soon as the ioslave is killed

meven updated this revision to Diff 70102. meven added a comment. Rebase on master REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D25117?vs=70101=70102 BRANCH arcpatch-D25117 REVISION DETAIL https://phabricator.kde.org/D25117 AFFECTED FILES

D25117: file ioslave: stop copying as soon as the ioslave is killed

meven updated this revision to Diff 70101. meven added a comment. Keep FileCopyJob::doKill REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D25117?vs=69208=70101 BRANCH arcpatch-D25117 REVISION DETAIL https://phabricator.kde.org/D25117 AFFECTED FILES

D25117: file ioslave: stop copying as soon as the ioslave is killed

meven added a comment. Anyone to review this ? 30 lines change + test REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D25117 To: meven, #frameworks, dfaure, ngraham, apol Cc: apol, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D25117: file ioslave: stop copying as soon as the ioslave is killed

meven added a reviewer: apol. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D25117 To: meven, #frameworks, dfaure, ngraham, apol Cc: apol, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D25117: file ioslave: stop copying as soon as the ioslave is killed

meven added a reviewer: ngraham. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D25117 To: meven, #frameworks, dfaure, ngraham Cc: apol, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D25117: file ioslave: stop copying as soon as the ioslave is killed

meven added a comment. friendly ping REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D25117 To: meven, #frameworks, dfaure Cc: apol, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D25117: file ioslave: stop copying as soon as the ioslave is killed

meven updated this revision to Diff 69208. meven added a comment. Fix test JobTest::cancelCopyAndCleanDest REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D25117?vs=69204=69208 BRANCH better-kill REVISION DETAIL https://phabricator.kde.org/D25117 AFFECTED

D25117: file ioslave: stop copying as soon as the ioslave is killed

meven updated this revision to Diff 69204. meven marked an inline comment as done. meven added a comment. Better safe than sorry, keep the doKill override REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D25117?vs=69187=69204 BRANCH better-kill REVISION

D25117: file ioslave: stop copying as soon as the ioslave is killed

apol added a comment. +1 makes a lot of sense overall. INLINE COMMENTS > filecopyjob.h:74 > bool doResume() override; > -bool doKill() override; > I don't think this change is ABI compatible (though after reading the docs, I'm not 100% sure).

D25117: file ioslave: stop copying as soon as the ioslave is killed

meven created this revision. meven added reviewers: Frameworks, dfaure. Herald added a project: Frameworks. Herald added a subscriber: kde-frameworks-devel. meven requested review of this revision. REVISION SUMMARY Previously when cancelling a copy the ioslave would continue. The file copy