Closed Bug 965196 Opened 6 years ago Closed 6 years ago

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

Categories

(Toolkit :: OS.File, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: Yoric, Assigned: lpy)

References

Details

(Keywords: dev-doc-needed, Whiteboard: [Async:ready][mentor=Yoric][lang=js])

Attachments

(1 file, 8 obsolete files)

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
Attached patch bug965196.patch (obsolete) — Splinter Review
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|.
Attached patch bug965196-V2.patch (obsolete) — Splinter Review
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+
Attached patch bug965196.patch (obsolete) — Splinter Review
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+
Attached patch bug965196-V4.patch (obsolete) — Splinter Review
update the patch
Attachment #8369079 - Attachment is obsolete: true
Attachment #8369499 - Flags: review?(dteller)
Attached patch bug965196-V5.patch (obsolete) — Splinter Review
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
Attached patch bug965196-V6.patch (obsolete) — Splinter Review
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+
Attached patch bug965196-V7.patch (obsolete) — Splinter Review
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)
Attached patch bug965196-V8.patch (obsolete) — Splinter Review
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+
Thank you :)
Attachment #8371466 - Attachment is obsolete: true
Attachment #8371518 - Flags: review+
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
Closed: 6 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
You need to log in before you can comment on or make changes to this bug.