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

Status: RESOLVED FIXED

(Toolkit Graveyard :: OS.File, defect)

Description

[David Teller [:Yoric] - still alive but not very active]

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.

Comment 1

[Peiyong Lin[:lpy](UTC+8)]

Attachment: bug965196.patch

Comment 2

[David Teller [:Yoric] - still alive but not very active]

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|.

Comment 3

[David Teller [:Yoric] - still alive but not very active]

By the way, let's rename the option |backupTo|.

Comment 4

[Peiyong Lin[:lpy](UTC+8)]

Attachment: bug965196-V2.patch

Thank you :)

Comment 5

[David Teller [:Yoric] - still alive but not very active]

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. »

Comment 6

[Peiyong Lin[:lpy](UTC+8)]

Attachment: bug965196.patch

test added

Comment 7

[David Teller [:Yoric] - still alive but not very active]

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|?

Comment 8

[Peiyong Lin[:lpy](UTC+8)]

Attachment: bug965196-V4.patch

update the patch

Comment 9

[Peiyong Lin[:lpy](UTC+8)]

Attachment: bug965196-V5.patch

Comment 10

[David Teller [:Yoric] - still alive but not very active]

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.

Comment 11

[Peiyong Lin[:lpy](UTC+8)]

Attachment: bug965196-V6.patch

Comment 12

[David Teller [:Yoric] - still alive but not very active]

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");
?

Comment 13

[Peiyong Lin[:lpy](UTC+8)]

Attachment: 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.

Comment 14

[Peiyong Lin[:lpy](UTC+8)]

Attachment: bug965196-V8.patch

forgot refresh the patch

Comment 15

[David Teller [:Yoric] - still alive but not very active]

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?

Comment 16

[Peiyong Lin[:lpy](UTC+8)]

Attachment: bug965196-Final.patch

Thank you :)

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/1f5b2cbebe63

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1f5b2cbebe63

Comment 19

[Carsten Book [:Tomcat]]

correcting milestone