Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2923163002:
[abandoned] Remove the destination file after CopyTask cancelation. (Closed)
|
Created:
3 years, 6 months ago by tetsui Modified:
3 years, 6 months ago Reviewers:
fukino CC:
chromium-reviews, oka+watch_chromium.org, rginda+watch_chromium.org, mtomasz+watch_chromium.org, yamaguchi+watch_chromium.org, fukino+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionRemove the destination file after CopyTask cancelation.
Prviously, when CopyTask was canceled, the destination file under copying
at the point was kept with empty or broken content.
Operations to files in general are not atomic, so there is no guarantee
either it always successfully deletes the destination file or the deleted file
is the file intended. However, in this specific case, it should be highly safe.
BUG=729529
TEST=Canceled single file copy, multiple file copy, and single directory copy
and confirmed that the incomplete file(s) were actually created and successfully
deleted. Used fileSystemProvider extension to artificially reproduce slow
filesystem.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Patch Set 1 #Patch Set 2 : Only delete the destination file when AbortError is returned. #Patch Set 3 : Delete directory #
Total comments: 4
Patch Set 4 : Resolve review comments. #Messages
Total messages: 24 (17 generated)
Description was changed from ========== Remove the destination file after CopyTask cancelation. Prviously, when CopyTask was canceled, the destination file under copying at the point was kept with empty or broken content. Operations to files in general are not atomic, so there is no guarantee either it always successfully deletes the destination file or the deleted file is the file intended. However, in this specific case, it should be highly safe. BUG=729529 ========== to ========== Remove the destination file after CopyTask cancelation. Prviously, when CopyTask was canceled, the destination file under copying at the point was kept with empty or broken content. Operations to files in general are not atomic, so there is no guarantee either it always successfully deletes the destination file or the deleted file is the file intended. However, in this specific case, it should be highly safe. BUG=729529 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by tetsui@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
tetsui@chromium.org changed reviewers: + fukino@chromium.org
fukino@: PTAL. Thanks!
* Title/Description says that we will remove the files on cancellation, but in the code, we always delete the file on error. Is it intentional? * Please make sure to write TEST= line about how you verified the change.
Description was changed from ========== Remove the destination file after CopyTask cancelation. Prviously, when CopyTask was canceled, the destination file under copying at the point was kept with empty or broken content. Operations to files in general are not atomic, so there is no guarantee either it always successfully deletes the destination file or the deleted file is the file intended. However, in this specific case, it should be highly safe. BUG=729529 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Remove the destination file after CopyTask cancelation. Prviously, when CopyTask was canceled, the destination file under copying at the point was kept with empty or broken content. Operations to files in general are not atomic, so there is no guarantee either it always successfully deletes the destination file or the deleted file is the file intended. However, in this specific case, it should be highly safe. BUG=729529 TEST=manual CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by tetsui@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/06/06 08:43:58, fukino wrote: > * Title/Description says that we will remove the files on cancellation, but in > the code, we always delete the file on error. Is it intentional? > * Please make sure to write TEST= line about how you verified the change. Sorry I forgot the most important part... I have to be more careful. I the latest patch on fileSystemProvider virtual filesystem with broken files and MTP storage with link device. I think it is better to have unit test. But, as file_operation_util.js does not have unit test, it would be a relatively large CL. So if you prefer unit test, please let me know.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
> Sorry I forgot the most important part... I have to be more careful. > I the latest patch on fileSystemProvider virtual filesystem with broken files > and MTP storage with link device. > I think it is better to have unit test. But, as file_operation_util.js does not > have unit test, it would be a relatively large CL. So if you prefer unit test, > please let me know. As the code path for file copy is getting complex, having a unit test or browser test to avoid future regression is great. It's OK to depend on a manual test for now and write the test in a separate patch. That said, could you explain the manual test a bit more specific? For example, TEST=canceled single file copy, multiple file copy, and single directory copy and confirmed that the incomplete file(s) were actually created and successfully deleted. (BTW, I'm not sure if the patch works for the directory copy...)
The CQ bit was checked by tetsui@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Remove the destination file after CopyTask cancelation. Prviously, when CopyTask was canceled, the destination file under copying at the point was kept with empty or broken content. Operations to files in general are not atomic, so there is no guarantee either it always successfully deletes the destination file or the deleted file is the file intended. However, in this specific case, it should be highly safe. BUG=729529 TEST=manual CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Remove the destination file after CopyTask cancelation. Prviously, when CopyTask was canceled, the destination file under copying at the point was kept with empty or broken content. Operations to files in general are not atomic, so there is no guarantee either it always successfully deletes the destination file or the deleted file is the file intended. However, in this specific case, it should be highly safe. BUG=729529 TEST=Canceled single file copy, multiple file copy, and single directory copy and confirmed that the incomplete file(s) were actually created and successfully deleted. Used fileSystemProvider extension to artificially reproduce slow filesystem. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
On 2017/06/07 04:26:54, fukino wrote: > > Sorry I forgot the most important part... I have to be more careful. > > I the latest patch on fileSystemProvider virtual filesystem with broken files > > and MTP storage with link device. > > I think it is better to have unit test. But, as file_operation_util.js does > not > > have unit test, it would be a relatively large CL. So if you prefer unit test, > > please let me know. > > As the code path for file copy is getting complex, having a unit test or browser > test to avoid future regression is great. > It's OK to depend on a manual test for now and write the test in a separate > patch. > That said, could you explain the manual test a bit more specific? > For example, > TEST=canceled single file copy, multiple file copy, and single directory copy > and confirmed that the incomplete file(s) were actually created and successfully > deleted. > (BTW, I'm not sure if the patch works for the directory copy...) Performed manual testing you mentioned, and also supported directory delete. Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2923163002/diff/40001/ui/file_manager/file_ma... File ui/file_manager/file_manager/background/js/file_operation_util.js (right): https://codereview.chromium.org/2923163002/diff/40001/ui/file_manager/file_ma... ui/file_manager/file_manager/background/js/file_operation_util.js:362: if (error.name === 'TypeMismatchError') { You don't need to try getFile first and handle the error. source.isFile can be used to distinguish file vs directory (see #404) https://codereview.chromium.org/2923163002/diff/40001/ui/file_manager/file_ma... ui/file_manager/file_manager/background/js/file_operation_util.js:364: entry.removeRecursively(function() { Hmm... I'm not sure if we should remove all files in the directory... Is it possible to remove only incomplete file(s)?
https://codereview.chromium.org/2923163002/diff/40001/ui/file_manager/file_ma... File ui/file_manager/file_manager/background/js/file_operation_util.js (right): https://codereview.chromium.org/2923163002/diff/40001/ui/file_manager/file_ma... ui/file_manager/file_manager/background/js/file_operation_util.js:362: if (error.name === 'TypeMismatchError') { On 2017/06/07 07:32:17, fukino wrote: > You don't need to try getFile first and handle the error. > source.isFile can be used to distinguish file vs directory (see #404) Done. https://codereview.chromium.org/2923163002/diff/40001/ui/file_manager/file_ma... ui/file_manager/file_manager/background/js/file_operation_util.js:364: entry.removeRecursively(function() { On 2017/06/07 07:32:17, fukino wrote: > Hmm... I'm not sure if we should remove all files in the directory... > Is it possible to remove only incomplete file(s)? onCopyProgress supplies copy start and end event for individual files. However, status.type === 'begin_copy_entry' does not contain status.destinationUrl, so at least we need path.join equivalent to do so (and it's not clean.)
Description was changed from ========== Remove the destination file after CopyTask cancelation. Prviously, when CopyTask was canceled, the destination file under copying at the point was kept with empty or broken content. Operations to files in general are not atomic, so there is no guarantee either it always successfully deletes the destination file or the deleted file is the file intended. However, in this specific case, it should be highly safe. BUG=729529 TEST=Canceled single file copy, multiple file copy, and single directory copy and confirmed that the incomplete file(s) were actually created and successfully deleted. Used fileSystemProvider extension to artificially reproduce slow filesystem. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Remove the destination file after CopyTask cancelation. Prviously, when CopyTask was canceled, the destination file under copying at the point was kept with empty or broken content. Operations to files in general are not atomic, so there is no guarantee either it always successfully deletes the destination file or the deleted file is the file intended. However, in this specific case, it should be highly safe. BUG=729529 TEST=Canceled single file copy, multiple file copy, and single directory copy and confirmed that the incomplete file(s) were actually created and successfully deleted. Used fileSystemProvider extension to artificially reproduce slow filesystem. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== |
