786211 - [OS.File] atomic write

Status: RESOLVED FIXED

(Toolkit Graveyard :: OS.File, defect)

Description

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

Implement a counterpart to the safe output stream: a function that writes data to a temporary file, flushes, then renames the file to its final name.

Comment 1

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

Attachment: 1. Shared code

First, shared code. This actually implements both |OS.File.writeAtomic| and |OS.File.read|.

Comment 2

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

Attachment: 2. Platform-specific code

This defines the platform-specific code for OS.File.{read, writeAtomic}. It adds a new option to |OS.File.move|, to ensure that we are not moving across disk boundaries, which would make the whole write non-atomic.

Comment 3

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

Attachment: Companion testsuite

The test suite. Unfortunately, I have found no way to access two distinct disks and test failure cases of |writeAtomic|.

Comment 4

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

Attachment: 1. Shared code

Sorry for the incomplete patch

Comment 5

[Nathan Froyd [:froydnj]]

Comment on attachment 656451 [details] [diff] [review]
1. Shared code

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

::: toolkit/components/osfile/osfile_shared_front.jsm
@@ +114,5 @@
>     * - {number} offset The offset in |buffer| at which to start extracting
>     * data
>     *
>     * @return {number} The number of bytes actually written, which may be
> +   * less than |bytes|.

Doh, thanks for fixing this.

@@ +322,5 @@
> + *
> + * @param {string} path The path of a file to write or overwrite.
> + * @param {string} tmpPath The name of a temporary file used to store
> + * the contents until the write is complete. This temporary file must
> + * be on the same drive as the destination file.

Do we really need tmpPath?  I think nsIFile/nsISafeOutputStream manages to do this same sort of thing without requiring an explicit temporary path.

If we must keep tmpPath, I think saying "device" instead of "drive" makes the description slightly less OS-specific.

@@ +324,5 @@
> + * @param {string} tmpPath The name of a temporary file used to store
> + * the contents until the write is complete. This temporary file must
> + * be on the same drive as the destination file.
> + * @param {ArrayBuffer|C void*} buffer The contents to write.
> + * @param {number} bytes The number of bytes to write.

It is kind of depressing that we have to specify bytes even with an ArrayBuffer argument.  WDYT about making bytes an 'options' argument instead?

Comment 6

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

(In reply to Nathan Froyd (:froydnj) from comment #5)
> Comment on attachment 656451 [details] [diff] [review]
> 1. Shared code
> 
> Review of attachment 656451 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/components/osfile/osfile_shared_front.jsm
> @@ +114,5 @@
> >     * - {number} offset The offset in |buffer| at which to start extracting
> >     * data
> >     *
> >     * @return {number} The number of bytes actually written, which may be
> > +   * less than |bytes|.
> 
> Doh, thanks for fixing this.
> 
> @@ +322,5 @@
> > + *
> > + * @param {string} path The path of a file to write or overwrite.
> > + * @param {string} tmpPath The name of a temporary file used to store
> > + * the contents until the write is complete. This temporary file must
> > + * be on the same drive as the destination file.
> 
> Do we really need tmpPath?  I think nsIFile/nsISafeOutputStream manages to
> do this same sort of thing without requiring an explicit temporary path.

There are essentially three possibilities:
1/ have the user provide |tmpPath|;
2/ have the implementation guess a temporary file with |mkstemp|/|GetTempFileName|;
3/ have the implementation bruteforce a temporary file name.

So:
1/ Is the fastest, simplest to implement, and follows Taras' specifications.
2/ Is the simplest to use and the most complex to code (impedence mismatch between js-ctypes and mkstemp/GetTempFileName).
3/ Is generally bad imo.

I would like to introduce 1/ now (it is, after all, the simplest to code) and extend it to 2/ later.

One way we could do this is

   function(path, buffer, number, options)

where for the moment |options| must contain |tmpPath|.

And extend future versions so that they make up |tmpPath| if it is not provided as an option.

Alternatively, we could keep the current signature and extend future versions so that they work even with |tmpPath == null|.

> If we must keep tmpPath, I think saying "device" instead of "drive" makes
> the description slightly less OS-specific.

Good idea.

> 
> @@ +324,5 @@
> > + * @param {string} tmpPath The name of a temporary file used to store
> > + * the contents until the write is complete. This temporary file must
> > + * be on the same drive as the destination file.
> > + * @param {ArrayBuffer|C void*} buffer The contents to write.
> > + * @param {number} bytes The number of bytes to write.
> 
> It is kind of depressing that we have to specify bytes even with an
> ArrayBuffer argument.  WDYT about making bytes an 'options' argument instead?

I have thought about it and did not find any perfect solution. We could make |bytes| part of |options| and fail if |buffer| is a pointer without |bytes|, provided we adapt all |write|-style functions.

Comment 7

[Nathan Froyd [:froydnj]]

Comment on attachment 656446 [details] [diff] [review]
Companion testsuite

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

::: toolkit/components/osfile/tests/mochi/worker_test_osfile_front.js
@@ +351,5 @@
> +  dest.close();
> +
> +  OS.File.writeAtomic(tmp_file_name, tmp_file_name + ".tmp", readResult.buffer, readResult.bytes);
> +  compare_files("test_readall_writeall_file (OS.File.readAll + writeAtomic 2)",
> +                src_file_name, tmp_file_name);

Out of paranoia, I might check that the .tmp version got cleaned up.  Up to you.

Comment 8

[Nathan Froyd [:froydnj]]

Comment on attachment 656444 [details] [diff] [review]
2. Platform-specific code

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

::: toolkit/components/osfile/osfile_unix_front.jsm
@@ +349,5 @@
>        * a file already exists at |destPath|. Otherwise, if this file exists,
>        * it will be erased silently.
> +      * @option {bool} noCopy - If set, this function will fail if the
> +      * operation is more sophisticated than a simple renaming, i.e. if
> +      * |sourcePath| and |destPath| are not situated on the same disk.

"disk" -> "device", I think.

@@ +562,5 @@
>           return;
>  
>         // If the error is not EXDEV ("not on the same device"),
>         // throw it.
> +       if (options.noCopy || ctypes.errno != Const.EXDEV) {

I think this test is slightly clearer if you switch the order in which you test the conditions.  Comment should be updated.

::: toolkit/components/osfile/osfile_win_front.jsm
@@ +410,5 @@
>        * a file already exists at |destPath|. Otherwise, if this file exists,
>        * it will be erased silently.
> +      * @option {bool} noCopy - If set, this function will fail if the
> +      * operation is more sophisticated than a simple renaming, i.e. if
> +      * |sourcePath| and |destPath| are not situated on the same disk.

"disk" -> "drive" (since this is Windows-specific)

@@ +427,5 @@
>         options = options || noOptions;
>         let flags;
> +       if (options.noCopy) {
> +         flags = 0;
> +       } else {

I think this is slightly better as:

let flags = 0;
if (!options.noCopy) {
  flags = ...;
}

though it'd be even better if the option were named 'copyAllowed' or something positive, so that the usual test isn't a double negative.  Sorry for not saying something about noOverwrite.

Comment 9

[Nathan Froyd [:froydnj]]

(In reply to David Rajchenbach Teller [:Yoric] from comment #6)
> One way we could do this is
> 
>    function(path, buffer, number, options)
> 
> where for the moment |options| must contain |tmpPath|.
> 
> And extend future versions so that they make up |tmpPath| if it is not
> provided as an option.

I think I like this a little better than...

> Alternatively, we could keep the current signature and extend future
> versions so that they work even with |tmpPath == null|.

...this one.  Though it's still ugly.

Also, you should make sure you clean up tmpPath appropriately in case of failure.

> > @@ +324,5 @@
> > > + * @param {string} tmpPath The name of a temporary file used to store
> > > + * the contents until the write is complete. This temporary file must
> > > + * be on the same drive as the destination file.
> > > + * @param {ArrayBuffer|C void*} buffer The contents to write.
> > > + * @param {number} bytes The number of bytes to write.
> > 
> > It is kind of depressing that we have to specify bytes even with an
> > ArrayBuffer argument.  WDYT about making bytes an 'options' argument instead?
> 
> I have thought about it and did not find any perfect solution. We could make
> |bytes| part of |options| and fail if |buffer| is a pointer without |bytes|,
> provided we adapt all |write|-style functions.

I think we should do that.  Seems silly to treat JS data structures as the second-class citizen here.

Are these requests venturing too far into the "OS.File should be user-friendly" discussion that we love so much? :)

Comment 10

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

(In reply to Nathan Froyd (:froydnj) from comment #9)
> (In reply to David Rajchenbach Teller [:Yoric] from comment #6)
> > One way we could do this is
> > 
> >    function(path, buffer, number, options)
> > 
> > where for the moment |options| must contain |tmpPath|.
> > 
> > And extend future versions so that they make up |tmpPath| if it is not
> > provided as an option.
> 
> I think I like this a little better than...
> 
> > Alternatively, we could keep the current signature and extend future
> > versions so that they work even with |tmpPath == null|.
> 
> ...this one.  Though it's still ugly.
> 
> Also, you should make sure you clean up tmpPath appropriately in case of
> failure.
> 
> > > @@ +324,5 @@
> > > > + * @param {string} tmpPath The name of a temporary file used to store
> > > > + * the contents until the write is complete. This temporary file must
> > > > + * be on the same drive as the destination file.
> > > > + * @param {ArrayBuffer|C void*} buffer The contents to write.
> > > > + * @param {number} bytes The number of bytes to write.
> > > 
> > > It is kind of depressing that we have to specify bytes even with an
> > > ArrayBuffer argument.  WDYT about making bytes an 'options' argument instead?
> > 
> > I have thought about it and did not find any perfect solution. We could make
> > |bytes| part of |options| and fail if |buffer| is a pointer without |bytes|,
> > provided we adapt all |write|-style functions.
> 
> I think we should do that.  Seems silly to treat JS data structures as the
> second-class citizen here.

Good point.

> Are these requests venturing too far into the "OS.File should be
> user-friendly" discussion that we love so much? :)

Applied changes as part of bug 769191.

Comment 11

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

Attachment: 1. Shared code, v2

Same code, but with |bytes| and |tmpPath| as part of |options|.

Comment 12

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

Attachment: Companion testsuite, v2

Propagating changes to the test suite.

Comment 13

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

Applied all the changes except this one:

(In reply to Nathan Froyd (:froydnj) from comment #8)
> though it'd be even better if the option were named 'copyAllowed' or
> something positive, so that the usual test isn't a double negative.  Sorry
> for not saying something about noOverwrite.

I am pretty sure that, by default, we want |move| to work regardless of devices, so the option should not be |copyAllowed|. It could be |onlyRename| (and |onlyCreate| for |noOverwrite|), though.

Comment 14

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

Attachment: 2. Platform-specific code, v2

Updated documentation, tweaked style as per feedback. No changes to semantics.

Comment 15

[Nathan Froyd [:froydnj]]

Comment on attachment 656601 [details] [diff] [review]
1. Shared code, v2

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

::: toolkit/components/osfile/osfile_shared_front.jsm
@@ +359,5 @@
> +  try {
> +    bytesWritten = tmpFile.write(buffer, options);
> +    tmpFile.flush();
> +  } catch (x) {
> +    tmpFile.close();

Assuming you can do try {} catch {} finally {} in JS, let's put the close in a finally {} block so that it's not duplicated along the error and non-error paths.

Comment 16

[Nathan Froyd [:froydnj]]

Comment on attachment 656601 [details] [diff] [review]
1. Shared code, v2

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

::: toolkit/components/osfile/osfile_shared_front.jsm
@@ +352,5 @@
> +AbstractFile.writeAtomic =
> +     function writeAtomic(path, buffer, options) {
> +  options = options || noOptions;
> +
> +  let tmpPath = options.tmpPath;

Ooo, sorry, you need to complain if this option is not present.

Comment 17

[Nathan Froyd [:froydnj]]

Comment on attachment 656602 [details] [diff] [review]
Companion testsuite, v2

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

You need a test that doesn't provide options.tmpPath and checks for thrown exceptions.

Comment 18

[Nathan Froyd [:froydnj]]

Comment on attachment 656601 [details] [diff] [review]
1. Shared code, v2

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

::: toolkit/components/osfile/osfile_shared_front.jsm
@@ +317,5 @@
> + */
> +AbstractFile.read = function read(path, bytes) {
> +  let file = exports.OS.File.open(path);
> +  try {
> +    return file.read(bytes);

Sorry, I'm being too hasty with my reviews this afternoon.  WDYT about checking:

  let r = file.read(bytes)
  if (r.buffer.length == bytes))
    return r.buffer;
  else
    return r.buffer.view(r.bytes); /* or however typed arrays say it */

?

Comment 19

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

Attachment: 1. Shared code, v3

Applied feedback.

Comment 20

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

Attachment: 2. Platform-specific code, v3

Comment 21

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

Attachment: Companion testsuite, v4

Comment 22

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

Attachment: 5. Testsuite for the async version

Adding the test suite for the async version.

Comment 23

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

Attachment: 4. Async code

Attaching the code for an async version. Note that I had to add bindings for |OS.File.remove| along the way.

Comment 24

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

Comment on attachment 664081 [details] [diff] [review]
4. Async code

Some of that code was written at 4am and it shows. Removing r?

Comment 25

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

Attachment: 4. Async code, v2

Same code, without some idiocies.

Comment 26

[Nathan Froyd [:froydnj]]

Comment on attachment 664490 [details] [diff] [review]
4. Async code, v2

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

Couple things to be addressed below before checking this in.

::: toolkit/components/osfile/osfile_async_front.jsm
@@ +620,5 @@
> + * is required. This requirement should disappear as part of bug 793660.
> + *
> + * @param {string} path The path of the file to modify.
> + * @param {ArrayByffer} buffer A buffer containing the bytes to write.
> + * @param {number} bytes The number of bytes to write.

This is not actually a parameter, it's an option.  Please document it as such.

@@ +638,5 @@
> +  };
> +  if ("byteLength" in buffer && (!("bytes" in options))) {
> +    options.bytes = buffer.byteLength;
> +  };
> +  // As options.tmpPath is a path, we need to encode it as |Type.path|

This comment doesn't seem to apply to the code.  Remove it?  Or fix the code?

Comment 27

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

(In reply to Nathan Froyd (:froydnj) from comment #26)
> Comment on attachment 664490 [details] [diff] [review]
> 4. Async code, v2
> 
> Review of attachment 664490 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Couple things to be addressed below before checking this in.
> 
> ::: toolkit/components/osfile/osfile_async_front.jsm
> @@ +620,5 @@
> > + * is required. This requirement should disappear as part of bug 793660.
> > + *
> > + * @param {string} path The path of the file to modify.
> > + * @param {ArrayByffer} buffer A buffer containing the bytes to write.
> > + * @param {number} bytes The number of bytes to write.
> 
> This is not actually a parameter, it's an option.  Please document it as
> such.

Done.

> @@ +638,5 @@
> > +  };
> > +  if ("byteLength" in buffer && (!("bytes" in options))) {
> > +    options.bytes = buffer.byteLength;
> > +  };
> > +  // As options.tmpPath is a path, we need to encode it as |Type.path|
> 
> This comment doesn't seem to apply to the code.  Remove it?  Or fix the code?

Actually, it does, but it should be a few lines above.

Comment 28

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

Attachment: 5. Testsuite for the async version

Comment 29

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

Attachment: 4. Async code, v2

Comment 30

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

Attachment: 1. Shared code, v3

Comment 31

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

Try: https://tbpl.mozilla.org/?tree=Try&rev=bf297dffc16a

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c19497a7d1dd
https://hg.mozilla.org/integration/mozilla-inbound/rev/73a24144ec2d
https://hg.mozilla.org/integration/mozilla-inbound/rev/480fc4fb5ff7
https://hg.mozilla.org/integration/mozilla-inbound/rev/b1a246930333
https://hg.mozilla.org/integration/mozilla-inbound/rev/dd84629231a1

Comment 34

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/c19497a7d1dd
https://hg.mozilla.org/mozilla-central/rev/73a24144ec2d
https://hg.mozilla.org/mozilla-central/rev/480fc4fb5ff7
https://hg.mozilla.org/mozilla-central/rev/b1a246930333
https://hg.mozilla.org/mozilla-central/rev/dd84629231a1