file ioslave: stop copying as soon as the ioslave is killed
Needs RevisionPublic

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

Details

Reviewers
dfaure
ngraham
apol
Group Reviewers
Frameworks
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
Branch
arcpatch-D25117
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 19022
Build 19040: arc lint + arc unit
meven created this revision.Sat, Nov 2, 12:28 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptSat, Nov 2, 12:28 PM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
meven requested review of this revision.Sat, Nov 2, 12:28 PM
apol added a subscriber: apol.Sun, Nov 3, 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.Sun, Nov 3, 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.Sun, Nov 3, 8:42 AM

Fix test JobTest::cancelCopyAndCleanDest

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

friendly ping

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

Anyone to review this ? 30 lines change + test

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

Keep FileCopyJob::doKill

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

Rebase on master

dfaure requested changes to this revision.Thu, Nov 21, 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
2171

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.Thu, Nov 21, 9:55 PM