[OS.File] Add an option |backupTo| to writeAtomic

RESOLVED FIXED in mozilla30

Status

()

Toolkit
OS.File
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: Yoric, Assigned: lpy)

Tracking

({dev-doc-needed})

unspecified
mozilla30
dev-doc-needed
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [Async:ready][mentor=Yoric][lang=js])

Attachments

(1 attachment, 8 obsolete attachments)

At the moment, whenever we call |writeAtomic("foo", data)|, this always erases file "foo". In a number of cases, it would be useful to move the destination file, if it exists, instead of overwriting it.
Assignee: nobody → pylaurent1314
(Assignee)

Comment 1

4 years ago
Created attachment 8368380 [details] [diff] [review]
bug965196.patch
Attachment #8368380 - Flags: review?(dteller)
Comment on attachment 8368380 [details] [diff] [review]
bug965196.patch

Review of attachment 8368380 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/osfile/modules/osfile_async_front.jsm
@@ +902,5 @@
>   * (not just for the application but for the whole system) but also safer:
>   * if the system shuts down improperly (typically due to a kernel freeze
>   * or a power failure) or if the device is disconnected before the buffer
>   * is flushed, the file has more chances of not being corrupted.
> + * - {bool} backup - If |false| or unspecified, don't rename the destination file

"don't backup" instead of "don't rename"

@@ +904,5 @@
>   * or a power failure) or if the device is disconnected before the buffer
>   * is flushed, the file has more chances of not being corrupted.
> + * - {bool} backup - If |false| or unspecified, don't rename the destination file
> + * if exists. If |true|, before rename temporary file to destination file,
> + * rename the destination file as backup first.

Also note somewhere that there is a small window during the renaming at which the destination file won't exist, only the backup and the temporary file.

::: toolkit/components/osfile/modules/osfile_shared_front.jsm
@@ +436,5 @@
>    } finally {
>      tmpFile.close();
>    }
>  
> +  if (OS.File.exists(path) && options.backup) {

If you exchange the arguments of the &&, this will be much faster and disk-efficient:
 if (options.backup && OS.File.exists(path))

@@ +437,5 @@
>      tmpFile.close();
>    }
>  
> +  if (OS.File.exists(path) && options.backup) {
> +    OS.File.move(path, path + ".backup", {noCopy: true});

Let's not hardcode that name ".backup". Rather, move the file to |options.backup|.
Attachment #8368380 - Flags: review?(dteller) → feedback+
By the way, let's rename the option |backupTo|.
(Assignee)

Comment 4

4 years ago
Created attachment 8368551 [details] [diff] [review]
bug965196-V2.patch

Thank you :)
Attachment #8368380 - Attachment is obsolete: true
Attachment #8368551 - Flags: review?(dteller)
Comment on attachment 8368551 [details] [diff] [review]
bug965196-V2.patch

Review of attachment 8368551 [details] [diff] [review]:
-----------------------------------------------------------------

You should also handle the case in which options.tmpPath isn't defined.
Finally, I'd like a few tests.

::: toolkit/components/osfile/modules/osfile_async_front.jsm
@@ +903,5 @@
>   * if the system shuts down improperly (typically due to a kernel freeze
>   * or a power failure) or if the device is disconnected before the buffer
>   * is flushed, the file has more chances of not being corrupted.
> + * - {string} backupTo - If |null| or unspecified, don't backup the destination file
> + * if exists. If specified, before rename temporary file to destination file,

Let's remove the sentence "If |null| or unspecified..."

@@ +907,5 @@
> + * if exists. If specified, before rename temporary file to destination file,
> + * backup the destination file as |backupTo| first.
> + *
> + * Note that, there will be a small window during the renaming, at which
> + * the destination file won't exist, only the backup and the temporary file.

« You should take this into account in case the process or the operating system freezes or crashes during this small window. »
Attachment #8368551 - Flags: review?(dteller) → feedback+
(Assignee)

Comment 6

4 years ago
Created attachment 8369079 [details] [diff] [review]
bug965196.patch

test added
Attachment #8368551 - Attachment is obsolete: true
Attachment #8369079 - Flags: review?(dteller)
Comment on attachment 8369079 [details] [diff] [review]
bug965196.patch

Review of attachment 8369079 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/osfile/tests/xpcshell/test_osfile_writeAtomic_backupTo_option.js
@@ +1,1 @@
> +"use strict";

Please don't forget the public domain header.
(I know that it doesn't appear in all tests, that's a regrettable error)

@@ +1,3 @@
> +"use strict";
> +
> +do_print("starting tests");

Please remove this line.

@@ +1,5 @@
> +"use strict";
> +
> +do_print("starting tests");
> +
> +let {OS: {File, Path, Constants}} = Components.utils.import("resource://gre/modules/osfile.jsm", {});

Please add both an initializer and a cleanup task to remove the backup path if it exists.
Also, I'd like tests to check the behavior:
- if there is a file at |path| and if there is no file at |path|;
- if the backup file exists and if the backup file doesn't exist yet.

@@ +3,5 @@
> +do_print("starting tests");
> +
> +let {OS: {File, Path, Constants}} = Components.utils.import("resource://gre/modules/osfile.jsm", {});
> +
> +// test when |backupTo| is specified, destination file will be backuped

"backed up"
Also, mention that you're using tmpPath here and not in the next test.

@@ +9,5 @@
> +  let path = Path.join(Constants.Path.tmpDir,
> +                      "test_backupTo_option_with_tmpPath.tmp");
> +  yield File.writeAtomic(path, "abc", { tmpPath: path + ".tmp",
> +                                        backupTo: path + ".backup"});
> +  do_check_true(yield File.exists(path + ".backup"));

Why would that file exist? When did you create a file at |path|?
Attachment #8369079 - Flags: review?(dteller) → feedback+
Blocks: 883609
(Assignee)

Comment 8

4 years ago
Created attachment 8369499 [details] [diff] [review]
bug965196-V4.patch

update the patch
Attachment #8369079 - Attachment is obsolete: true
Attachment #8369499 - Flags: review?(dteller)
(Assignee)

Comment 9

4 years ago
Created attachment 8369505 [details] [diff] [review]
bug965196-V5.patch
Attachment #8369499 - Attachment is obsolete: true
Attachment #8369499 - Flags: review?(dteller)
Attachment #8369505 - Flags: review?(dteller)
Comment on attachment 8369505 [details] [diff] [review]
bug965196-V5.patch

Review of attachment 8369505 [details] [diff] [review]:
-----------------------------------------------------------------

We may still need a few cycles, but we're getting there.

::: toolkit/components/osfile/modules/osfile_async_front.jsm
@@ +903,5 @@
>   * if the system shuts down improperly (typically due to a kernel freeze
>   * or a power failure) or if the device is disconnected before the buffer
>   * is flushed, the file has more chances of not being corrupted.
> + * - {string} backupTo - If specified, backup the destination file as |backupTo|.
> + *

Nit: Please remove this empty line.

@@ +907,5 @@
> + *
> + * Note that, there will be a small window during the renaming, at which
> + * the destination file won't exist, only the backup and the temporary file.
> + * Also, the process or the operating system may freeze or crash
> + * during this small window.

Maybe "Note that this function renames the destination file before overwriting it. If the process or the operating system freezes or crashes during the short window between these operations, the destination file will have been moved to its backup."

::: toolkit/components/osfile/modules/osfile_shared_front.jsm
@@ +411,5 @@
>      options.bytes = buffer.byteLength;
>    }
>  
> +  if (options.backupTo && OS.File.exists(path)) {
> +    OS.File.move(path, options.backupTo, {noCopy: true});

That's too early. We want the move to take place as late as possible, to minimize the window during which |path| doesn't exist.

::: toolkit/components/osfile/tests/xpcshell/test_osfile_writeAtomic_backupTo_option.js
@@ +17,5 @@
> + * test_backupTo_option_without_destination_file.tmp.backup
> + * test_backupTo_option_with_backup_file.tmp
> + * test_backupTo_option_with_backup_file.tmp.backup
> + */
> +function clearFiles() {

Since you use yield to handle peomises, you should wrap this in a Task.spawn.

@@ +23,5 @@
> +                "test_backupTo_option_without_tmpPath.tmp",
> +                "test_non_backupTo_option.tmp",
> +                "test_backupTo_option_without_destination_file.tmp",
> +                "test_backupTo_option_with_backup_file.tmp"];
> +  for (let index = 0; index < pathes.length; ++index) {

Nit:
for (let path of [....]) {
}

@@ +59,5 @@
> +  let path = Path.join(Constants.Path.tmpDir,
> +                       "test_backupTo_option_with_tmpPath.tmp");
> +  try {
> +    let openedFile = yield File.open(path, { create: true });
> +    yield openedFile.close();

Just use a OS.File.writeAtomic(path, "default contents"). This is more readable.

@@ +61,5 @@
> +  try {
> +    let openedFile = yield File.open(path, { create: true });
> +    yield openedFile.close();
> +  } catch (ex) {
> +    // ignore

Why do you ignore the error? It could be important. Just don't try/catch.

@@ +65,5 @@
> +    // ignore
> +  }
> +  yield File.writeAtomic(path, "abc", { tmpPath: path + ".tmp",
> +                                        backupTo: path + ".backup" });
> +  do_check_true(yield File.exists(path + ".backup"));

Nit:
 do_check_true((yield File.exists(path + ".backup")));
(with the double-parenthesis)

Also, please check that the contents of the backup file is the same as the original contents of |path|.

Essentially, same remarks for the tests below.
Attachment #8369505 - Flags: review?(dteller) → feedback+
Summary: [OS.File] Add an option |backup| to writeAtomic → [OS.File] Add an option |backupTo| to writeAtomic
(Assignee)

Comment 11

4 years ago
Created attachment 8370192 [details] [diff] [review]
bug965196-V6.patch
Attachment #8369505 - Attachment is obsolete: true
Attachment #8370192 - Flags: review?(dteller)
Comment on attachment 8370192 [details] [diff] [review]
bug965196-V6.patch

Review of attachment 8370192 [details] [diff] [review]:
-----------------------------------------------------------------

Almost there.

::: toolkit/components/osfile/modules/osfile_shared_front.jsm
@@ +409,5 @@
>      options = Object.create(options);
>      options.bytes = buffer.byteLength;
>    }
>  
> +  

Nit: Please remove these newlines.

@@ +416,5 @@
>  
>    if (!options.tmpPath) {
>      // Just write, without any renaming trick
> +    if (options.backupTo && OS.File.exists(path)) {
> +      OS.File.move(path, options.backupTo, {noCopy: true});

Let's optimize a little further:
 if (options.backupTo) {
   try {
     OS.File.move(...);
   } catch (ex if ex.becauseNoSuchFile) {
     // The file doesn't exist, nothing to backup.
   }
 }

This will save one I/O, hence a little battery.

@@ +444,5 @@
>      tmpFile.close();
>    }
>  
> +  if (options.backupTo && OS.File.exists(path)) {
> +    OS.File.move(path, options.backupTo, {noCopy: true});

Same here.

::: toolkit/components/osfile/tests/xpcshell/test_osfile_writeAtomic_backupTo_option.js
@@ +51,5 @@
> + */
> +add_task(function* test_backupTo_option_with_tmpPath() {
> +  let path = Path.join(Constants.Path.tmpDir,
> +                       "test_backupTo_option_with_tmpPath.tmp");
> +  yield File.writeAtomic(path, "default contents");

Please define "default contents" as a per-function constant. Also, just in case, let's concatenate a random number at the end of that constant, to further ensure that we're not confusing data from two successive runs or tests. Same thing for the other tests.

@@ +53,5 @@
> +  let path = Path.join(Constants.Path.tmpDir,
> +                       "test_backupTo_option_with_tmpPath.tmp");
> +  yield File.writeAtomic(path, "default contents");
> +  yield File.writeAtomic(path, "abc", { tmpPath: path + ".tmp",
> +                                        backupTo: path + ".backup" });

Same thing for "abc".

@@ +55,5 @@
> +  yield File.writeAtomic(path, "default contents");
> +  yield File.writeAtomic(path, "abc", { tmpPath: path + ".tmp",
> +                                        backupTo: path + ".backup" });
> +  do_check_true((yield File.exists(path + ".backup")));
> +  let contents = yield File.read(path + ".backup", undefined);

You don't need this |undefined|.

@@ +112,5 @@
> + * |backupTo| specified
> + * |tmpPath| not specified
> + * backup file exists
> + * destination file exists
> + * @result destination file will be backed up 

Nit: Please remove the whitespace at the end of the line.

@@ +123,5 @@
> +  let openedFile = yield File.open(path + ".backup", { write: true, create: true });
> +  openedFile.write(new Uint8Array(1000));
> +  let stat = yield openedFile.stat();
> +  do_check_eq(1000, stat.size);
> +  yield openedFile.close();

Why not simply
yield File.writeAtomic(path + ".backup", "something something something");
?
Attachment #8370192 - Flags: review?(dteller) → feedback+
(Assignee)

Comment 13

4 years ago
Created attachment 8371464 [details] [diff] [review]
bug965196-V7.patch

At the very beginning, I created the back up file and checked its contents to ensure that the back up file existed and it was exactly that back up file I have created. But you are right, that was redundant and should be simplified.
Attachment #8370192 - Attachment is obsolete: true
Attachment #8371464 - Flags: review?(dteller)
(Assignee)

Comment 14

4 years ago
Created attachment 8371466 [details] [diff] [review]
bug965196-V8.patch

forgot refresh the patch
Attachment #8371464 - Attachment is obsolete: true
Attachment #8371464 - Flags: review?(dteller)
Attachment #8371466 - Flags: review?(dteller)
Comment on attachment 8371466 [details] [diff] [review]
bug965196-V8.patch

Review of attachment 8371466 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me.

::: toolkit/components/osfile/tests/xpcshell/test_osfile_writeAtomic_backupTo_option.js
@@ +50,5 @@
> + * @result destination file will be backed up
> + */
> +add_task(function* test_backupTo_option_with_tmpPath() {
> +  let defaultContents = "default contents" + Math.random();
> +  let writeContents = "abc" + Math.random();

Nit: Can you make it DEFAULT_CONTENTS and WRITE_CONTENTS?
Attachment #8371466 - Flags: review?(dteller) → review+
(Assignee)

Comment 16

4 years ago
Created attachment 8371518 [details] [diff] [review]
bug965196-Final.patch

Thank you :)
Attachment #8371466 - Attachment is obsolete: true
Attachment #8371518 - Flags: review+
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/1f5b2cbebe63
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [Async:ready][mentor=Yoric][lang=js] → [Async:ready][mentor=Yoric][lang=js][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/1f5b2cbebe63
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Whiteboard: [Async:ready][mentor=Yoric][lang=js][fixed-in-fx-team] → [Async:ready][mentor=Yoric][lang=js]
Target Milestone: --- → mozilla29
correcting milestone
Target Milestone: mozilla29 → mozilla30
Keywords: dev-doc-needed
You need to log in before you can comment on or make changes to this bug.