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

Authored by meven on Nov 2 2019, 12:28 PM.

Details

Summary

Previously when cancelling a copy the ioslave would continue.
The file copy job would then remove the file.
The dest file being removed would cause an IO error in the slave resulting in warnings such as :

kf5.kio.kio_file: Could not change permissions for "...
kf5.kio.kio_file: Couldn't preserve group for "...
kf5.kio.kio_file: Couldn't preserve access and modification time for "...

Preserve clean up of intermediate file copy leftover.

Test Plan

Copy a file, cancel the copy -> the destination file is cleaned
Copy a file, overwriting another -> the .part intermediate file is cleaned

Diff Detail

Repository
R241 KIO
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
meven created this revision.Nov 2 2019, 12:28 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptNov 2 2019, 12:28 PM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
meven requested review of this revision.Nov 2 2019, 12:28 PM
apol added a subscriber: apol.Nov 3 2019, 2:34 AM

+1 makes a lot of sense overall.

src/core/filecopyjob.h
74 ↗(On Diff #69187)

I don't think this change is ABI compatible (though after reading the docs, I'm not 100% sure).
https://community.kde.org/Policies/Binary_Compatibility_Issues_With_C%2B%2B

You could always define it to call the parent though.

meven updated this revision to Diff 69204.Nov 3 2019, 7:25 AM
meven marked an inline comment as done.

Better safe than sorry, keep the doKill override

meven updated this revision to Diff 69208.Nov 3 2019, 8:42 AM

Fix test JobTest::cancelCopyAndCleanDest

meven added a comment.Nov 8 2019, 12:06 PM

friendly ping

meven added a reviewer: apol.Nov 9 2019, 2:36 PM
meven added a comment.Nov 14 2019, 8:37 AM

Anyone to review this ? 30 lines change + test

meven updated this revision to Diff 70101.Nov 21 2019, 1:09 PM

Keep FileCopyJob::doKill

meven updated this revision to Diff 70102.Nov 21 2019, 1:13 PM

Rebase on master

dfaure requested changes to this revision.Nov 21 2019, 9:55 PM

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 probably a no-op then, there's no signal handler there to call setKillFlags(), so no opportunity to cleanup before dying.
Some research indicates that we'd have to use SetConsoleCtrlHandler on Windows for similar behaviour.
https://stackoverflow.com/questions/2007516/is-there-a-posix-sigterm-alternative-on-windows-a-gentle-kill-for-console-ap
https://danielkaes.wordpress.com/2009/06/04/how-to-catch-kill-events-with-python/

OK so at least it seems feasible, so I won't object to the commit even if it doesn't implement it on Windows.
-1 just for the missing QVERIFY.

autotests/jobtest.cpp
2117

Always use QVERIFY() around spy.wait().
Well, I'm assuming we actually expect the signal to be emitted :-)
I would also remove the 500 then, the default value will be fine.

This revision now requires changes to proceed.Nov 21 2019, 9:55 PM

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 a timeout somewhere.

Hmm, the Windows code is probably a no-op then, there's no signal handler there to call setKillFlags(), so no opportunity to cleanup before dying.
Some research indicates that we'd have to use SetConsoleCtrlHandler on Windows for similar behaviour.
https://stackoverflow.com/questions/2007516/is-there-a-posix-sigterm-alternative-on-windows-a-gentle-kill-for-console-ap
https://danielkaes.wordpress.com/2009/06/04/how-to-catch-kill-events-with-python/

I did not realize that.

OK so at least it seems feasible, so I won't object to the commit even if it doesn't implement it on Windows.

This would be a regression in Windows, because of the code removal in filecopyjob.cpp.
I need to implement signal handling for windows starting at KIOPrivate::sendTerminateSignal in kioglobal_p_win.cpp. currently it simply kills directly the process. and use SetConsoleCtrlHandler as you suggested to fix this.
Or add a #ifdef Q_OS_WIN block in filecopyjob.cpp.

meven updated this revision to Diff 70401.Nov 27 2019, 9:24 AM

Let windows code path not being changed, add a todo for windows support

dfaure requested changes to this revision.Jan 3 2020, 9:44 PM
dfaure added inline comments.
autotests/jobtest.cpp
2117

Why the short 500ms timeout? CI can be slow sometimes.

src/ioslaves/file/file_unix.cpp
412

shouldn't this be error(KIO::ERR_USER_CANCELED) instead? It's not like the copy actually succeeded...

This revision now requires changes to proceed.Jan 3 2020, 9:44 PM
meven updated this revision to Diff 72729.Jan 4 2020, 10:06 AM

Use more appropriate error(KIO::ERR_USER_CANCELED)

meven added a comment.Jan 4 2020, 10:07 AM

I have to check on the tests.

autotests/jobtest.cpp
2117

It was the previous value used line 2176 for the same use, except the file cleaning will now be more immediate.

dfaure accepted this revision.Jan 4 2020, 10:07 AM
This revision is now accepted and ready to land.Jan 4 2020, 10:07 AM
meven updated this revision to Diff 72798.Jan 5 2020, 2:52 PM

Stabilise test

dfaure requested changes to this revision.Jan 5 2020, 5:37 PM
dfaure added inline comments.
autotests/jobtest.cpp
2128

How about QTRY_VERIFY instead of a hardcoded 500ms sleep?

This way, if the file gets deleted after 100ms, we can move on.

This revision now requires changes to proceed.Jan 5 2020, 5:37 PM
meven updated this revision to Diff 72832.Jan 5 2020, 8:45 PM

Use QTRY_VERIFY instead of sleep + Q_VERIFY

meven marked an inline comment as done.Jan 6 2020, 8:52 PM

Thanks for the suggestion @dfaure

autotests/jobtest.cpp
2098

This may-be excessive, but I needed that since I have a fast nvme drive.

meven added a comment.Jan 6 2020, 8:54 PM

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.

dfaure added inline comments.Jan 6 2020, 10:59 PM
autotests/jobtest.cpp
2098

The alternative is to export an env var that makes the ioslave call some msleep()...

meven added inline comments.Jan 7 2020, 7:44 AM
autotests/jobtest.cpp
2098

Making an ioslave reading an env var for testing purposes is not a great alternative.

dfaure added a comment.EditedJan 7 2020, 8:09 AM

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.

See KIOSLAVE_ENABLE_TESTMODE to make them call QStandardPaths::setTestModeEnabled(true) for instance.

An env var that slows them down should be a separate one though, of course :)

meven updated this revision to Diff 74542.Wed, Jan 29, 5:54 AM
meven marked 6 inline comments as done.

Fix test

meven added a comment.Wed, Jan 29, 5:57 AM

Fix test

I was missing the case when the dest file existed before copying.
I think this is ready for landing.

meven planned changes to this revision.Wed, Jan 29, 6:29 AM
meven added inline comments.
autotests/jobtest.cpp
2130

This is not right. It might hide a bug.

meven updated this revision to Diff 74543.Wed, Jan 29, 7:47 AM

Add an env variable KIOSLAVE_FILE_ENABLE_TESTMODE to slow down copy file operations to make tests reliable

meven updated this revision to Diff 74570.Wed, Jan 29, 1:47 PM

rebasing

dfaure added inline comments.Sun, Feb 2, 10:17 AM
autotests/jobtest.cpp
2098

Does this mean the file could be made smaller again?

(to reduce disk-space requirements)

src/ioslaves/file/file_unix.cpp
317

Maybe in testMode, the slave can become particularly slow for one given filename.
E.g. if (testMode && fileName.contains("slow"))

This way, for other files, it remains fast, and for that one file it can be really reliably slow (possibly sleeping for a whole second, or something like that).

It also makes the more generically named "KIOSLAVE_FILE_ENABLE_TESTMODE" available for enabling other kinds of "test mode" in the ioslave (the filename acting as an additional switch).

meven updated this revision to Diff 74870.Sun, Feb 2, 6:19 PM
meven marked 3 inline comments as done.

Reduce the size of the tempfile for JobTest::cancelCopyAndCleanDest and restrict testMode slowness to copy where destination is slow

meven added inline comments.Sun, Feb 2, 6:19 PM
autotests/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.

2098

I have reduced the size space needed to a smaller value than before this patch.

meven added inline comments.Mon, Feb 3, 8:01 AM
autotests/jobtest.cpp
82

But then it could cause difficulties for downstream distros, running tests as part of their packaging process.

meven marked 2 inline comments as done.Mon, Feb 3, 8:03 AM
dfaure accepted this revision.Mon, Feb 3, 8:17 AM
This revision is now accepted and ready to land.Mon, Feb 3, 8:17 AM
dfaure added a comment.Mon, Feb 3, 8:20 AM

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 it's always false.

This revision was automatically updated to reflect the committed changes.