[OS.File] atomic write

RESOLVED FIXED in mozilla18

Status

()

Toolkit
OS.File
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: Yoric, Assigned: Yoric)

Tracking

unspecified
mozilla18
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments, 11 obsolete attachments)

5.52 KB, patch
Yoric
: review+
Details | Diff | Splinter Review
2.88 KB, patch
Yoric
: review+
Details | Diff | Splinter Review
3.88 KB, patch
Yoric
: review+
Details | Diff | Splinter Review
7.83 KB, patch
Yoric
: review+
Details | Diff | Splinter Review
2.65 KB, patch
Yoric
: review+
Details | Diff | Splinter Review
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.
Created attachment 656438 [details] [diff] [review]
1. Shared code

First, shared code. This actually implements both |OS.File.writeAtomic| and |OS.File.read|.
Assignee: nobody → dteller
Attachment #656438 - Flags: review?(nfroyd)
Created attachment 656444 [details] [diff] [review]
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.
Attachment #656444 - Flags: review?(nfroyd)
Created attachment 656446 [details] [diff] [review]
Companion testsuite

The test suite. Unfortunately, I have found no way to access two distinct disks and test failure cases of |writeAtomic|.
Attachment #656446 - Flags: review?(nfroyd)
Created attachment 656451 [details] [diff] [review]
1. Shared code

Sorry for the incomplete patch
Attachment #656438 - Attachment is obsolete: true
Attachment #656438 - Flags: review?(nfroyd)
Attachment #656451 - Flags: review?(nfroyd)
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?
Attachment #656451 - Flags: review?(nfroyd) → feedback+
(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 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.
Attachment #656446 - Flags: review?(nfroyd) → review+
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.
Attachment #656444 - Flags: review?(nfroyd) → review+
(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? :)
(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.
Depends on: 769191
Created attachment 656601 [details] [diff] [review]
1. Shared code, v2

Same code, but with |bytes| and |tmpPath| as part of |options|.
Attachment #656451 - Attachment is obsolete: true
Attachment #656601 - Flags: review?(nfroyd)
Created attachment 656602 [details] [diff] [review]
Companion testsuite, v2

Propagating changes to the test suite.
Attachment #656446 - Attachment is obsolete: true
Attachment #656602 - Flags: review?(nfroyd)
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.
Created attachment 656907 [details] [diff] [review]
2. Platform-specific code, v2

Updated documentation, tweaked style as per feedback. No changes to semantics.
Attachment #656444 - Attachment is obsolete: true
Attachment #656907 - Flags: review?(nfroyd)
Attachment #656907 - Flags: review?(nfroyd) → review+
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.
Attachment #656601 - Flags: review?(nfroyd) → review+
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 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.
Attachment #656602 - Flags: review?(nfroyd) → review+
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 */

?
Created attachment 664034 [details] [diff] [review]
1. Shared code, v3

Applied feedback.
Attachment #656601 - Attachment is obsolete: true
Attachment #664034 - Flags: review+
Created attachment 664035 [details] [diff] [review]
2. Platform-specific code, v3
Attachment #656907 - Attachment is obsolete: true
Attachment #664035 - Flags: review+
Attachment #664035 - Attachment is patch: true
Created attachment 664036 [details] [diff] [review]
Companion testsuite, v4
Attachment #656602 - Attachment is obsolete: true
Attachment #664036 - Flags: review+
Created attachment 664080 [details] [diff] [review]
5. Testsuite for the async version

Adding the test suite for the async version.
Attachment #664080 - Flags: review?(nfroyd)
Created attachment 664081 [details] [diff] [review]
4. Async code

Attaching the code for an async version. Note that I had to add bindings for |OS.File.remove| along the way.
Attachment #664081 - Flags: review?(nfroyd)
Comment on attachment 664081 [details] [diff] [review]
4. Async code

Some of that code was written at 4am and it shows. Removing r?
Attachment #664081 - Flags: review?(nfroyd)
Created attachment 664490 [details] [diff] [review]
4. Async code, v2

Same code, without some idiocies.
Attachment #664081 - Attachment is obsolete: true
Attachment #664490 - Flags: review?(nfroyd)
Attachment #664080 - Flags: review?(nfroyd) → review+
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?
Attachment #664490 - Flags: review?(nfroyd) → review+
Depends on: 793949
(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.
Created attachment 665300 [details] [diff] [review]
5. Testsuite for the async version
Attachment #664080 - Attachment is obsolete: true
Attachment #665300 - Flags: review+
Created attachment 665301 [details] [diff] [review]
4. Async code, v2
Attachment #664490 - Attachment is obsolete: true
Attachment #665301 - Flags: review+
Created attachment 665302 [details] [diff] [review]
1. Shared code, v3
Attachment #664034 - Attachment is obsolete: true
Attachment #665302 - Flags: review+
Keywords: checkin-needed
Duplicate of this bug: 781623
You need to log in before you can comment on or make changes to this bug.