[FileCopyJob] Clean up after cancelation of file copy operation
ClosedPublic

Authored by chinmoyr on Feb 10 2019, 5:43 PM.

Details

Summary

Delete the destination file if the copy job was canceled. In case Overwrite flag
was provided to the job, delete the .part file.

Test Plan

Unit testing and manual testing done.

Diff Detail

Repository
R241 KIO
Branch
arcpatch-D18904 (branched from master)
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 9248
Build 9266: arc lint + arc unit
chinmoyr created this revision.Feb 10 2019, 5:43 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptFeb 10 2019, 5:43 PM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
chinmoyr requested review of this revision.Feb 10 2019, 5:43 PM
dfaure added inline comments.Feb 10 2019, 7:42 PM
autotests/jobtest.cpp
2066

no space after connect

2069

I'm curious, why this signal, just to call another lambda? Can't you move the code of the second lambda into this one? It's not like the signal is queued right now, it's a direct call.

2080

Add f.remove() here, to avoid leaving a 10MB file around...

(The alternative would be to use QTemporaryFile)

src/core/filecopyjob.cpp
478

Why the "probably"? ;-)

588

Why "not suspended"? If we suspend and kill, don't we want the cleanup?

589

This if makes little sense as is, since d->m_delJob can only be set when d->m_copyJob is nullptr (see line 515). So it's redundant, and the same as writing if (d->m_copyJob) -- and it looks less scary, I was initially worried when reading your line about what would happen before m_delJob was set or after m_delJob was null again.

590

A test for isLocalFile() is missing first.

Woohoo! Please add:

BUG: 125102
FIXED-IN: 5.56

To the Summary section. :)

Hmm actually maybe I take it back... is this in fact intended to fix 125102? Because the destination file is still destroyed when an overwrite operation is canceled in the middle. That's not what we want; we want the destination file to be left untouched when the user intentionally cancels an overwrite. This will probably require moving/copying to a temp file and replacing the destination at the end, once the operation has completed--if there's enough space on disk for this of course.

@ngraham The destination file is corrupted as soon as the user clicks overwrite. This patch deletes that file. Is that file supposed to be preserved (to prevent user panic)? My suggestion is to append ".part" or some similar extension when an overwrite starts, so the user knows that there data is gone.

chinmoyr updated this revision to Diff 51383.Feb 11 2019, 2:17 AM
chinmoyr marked 4 inline comments as done.

Fixed all issues.
Added test case for suspend.

so the user knows that there data is gone

*their

@ngraham The destination file is corrupted as soon as the user clicks overwrite.

That's exactly the problem we need to fix.

For example, if the user starts an overwrite by accident and then cancels it, their original file in the destination has been destroyed. This is bad. Canceling an accidental action should never be destructive.

That's why I'm recommending that for overwrite operations, we don't instantly delete or start overwriting the destination file, but rather move or copy the source file to a separate location, and then overwrite the destination file. This preserves the user's ability to cancel the job during the move or copy portion of the operation without destroying their own files.

Your reasoning seems to forget that there is a first confirmation, in case of overwriting, in the form of the "Overwrite/Skip/Cancel" dialog?

Otherwise I kind of like the idea, but there's the problem of overwriting a 4GB file on a 5GB partition... (no space for 4*2 GB temporarily).

Yes, obviously this won't work when disk space is limited and we'll need to check for that. In this case, I would like the the confirmation dialog to actually say to the user that the overwrite operation cannot be canceled without destroying the target file.

dfaure accepted this revision.Mar 2 2019, 11:32 PM

@ngraham I'm not convinced. Warning users that an operation can't be undone makes sense, but canceling in the middle of the operation is not usually something to warn about. It's nice to handle it better, but I wouldn't bother the user with "you know, if you cancel, you'll be in trouble". Well, yes, I'm OVERWRITING a file. If I'm too slow to cancel, I'll have the same trouble too.

Anyhow, that's for the GUI side of things, this commit seems fine in itself.

This revision is now accepted and ready to land.Mar 2 2019, 11:32 PM
ngraham accepted this revision.Mar 3 2019, 3:22 PM

OK. I'm open to being convinced out of my crazy idea. :)

In any event, this is good. Thanks for your work on this @chinmoyr!

chinmoyr updated this revision to Diff 53291.Mar 6 2019, 4:22 PM

Delete .part file when Overwrite flag is set.

@dfaure please have a look at it again.

chinmoyr updated this revision to Diff 53297.Mar 6 2019, 4:33 PM

Remove unrelated changes commited by mistake.

dfaure requested changes to this revision.Mar 6 2019, 10:04 PM

Withdrawing my approval, after realizing that this is missing the "enough free space at destination" check.
Imagine a 5GB partition with a 4GB file on it. You want to copy another version of that 4GB file onto it. Old implementation: it works. New implementation: we start copying 1GB into a .part file, then it fails (out of space), and then IIUC we finally do the right thing and overwrite the file. Right? So it ends up working, but it's much slower than it should behave. This could be improved with a free space check, right?

src/core/filecopyjob.cpp
479

"file write" sounds like putJob, which isn't what this is about.

I guess this should be called d->m_bFileCopyInProgress instead.

This revision now requires changes to proceed.Mar 6 2019, 10:04 PM

There's free space check in CopyJob. And Dolphin still gives an error in the said situation. But if some application decides to directly use FileCopyJob then IMO the free space check should be at application's side. Anyway, I tried adding it here and it felt awkward to me.

dfaure accepted this revision.Mar 9 2019, 9:07 AM

Ah, the free space check in CopyJob doesn't take into account that it's an overwrite? Heh, two bugs canceling each other, in a way ;-)

In the situation I describe, there *is* space to do the copying, just not with a temporary .part file.
So I didn't expect an error in that case, but rather directly overwriting without using .part.

But OK that's a separate issue then, the check itself could be improved.

This revision is now accepted and ready to land.Mar 9 2019, 9:07 AM
chinmoyr updated this revision to Diff 53604.Mar 10 2019, 4:43 PM
chinmoyr marked an inline comment as done.

Changed m_bFileWriteInProgress -> m_bFileCopyInProgress

chinmoyr updated this revision to Diff 53605.Mar 10 2019, 4:46 PM
chinmoyr retitled this revision from [FileCopyJob] Clean up after file copy operation is cancelled to [FileCopyJob] Clean up after cancelation of file copy operation.
chinmoyr edited the summary of this revision. (Show Details)

Update summary

This revision was automatically updated to reflect the committed changes.