890050 - [OS.File] writeAtomic should always rename

Status: RESOLVED FIXED

(Toolkit Graveyard :: OS.File, defect)

Description

[Vladan Djeric (:vladan)]

writeAtomic should always write to a temporary file first and rename it after writing, regardless of the flush argument

- It makes the behavior of writeAtomic predictable
- It's slightly safer (tm)
- It's much safer if I/O isn't being buffered (although I don't think unbuffered I/O is currently supported in OS.File)

Also, we need to update the documentation: https://developer.mozilla.org/en-US/docs/JavaScript_OS.File/OS.File_for_the_main_thread#OS.File.writeAtomic%28%29

Comment 1

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

Attachment: OS.File.writeAtomic now renames (again) by default

Comment 2

[Nathan Froyd [:froydnj]]

Comment on attachment 771961 [details] [diff] [review]
OS.File.writeAtomic now renames (again) by default

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

::: toolkit/components/osfile/osfile_shared_front.jsm
@@ +335,1 @@
>   * file. This is considerably slower but slightly safer: if

I'm trying to think of a better way to say this; "...will not flush the disk buffers...will flush the file" is not good structure, but I don't have any better ideas offhand.

@@ +338,5 @@
>   * is flushed, the file has more changes of not being corrupted.
> + * - {bool} rename - If |true| or unspecified, the function will write
> + * all contents to a temporary file and use that temporary file to
> + * overwrite |path|. If |false|, the function will write directly to
> + * |path|, which is a little faster but slightly less safe.

Do we really need rename?  Can't we just say that if you provide tmpPath (which is required), then we do renaming, otherwise we write directly to the file?

This whole interface feels really clunky. :(

@@ +360,5 @@
>      let encoding = options.encoding || "utf-8";
>      buffer = new TextEncoder(encoding).encode(buffer);
>    }
>  
> +  let bytesWritten;

I think you want to initialize this to 0; I guess undefined is a reasonable choice too, but a *little* proper typing of variables feels better.

Comment 3

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

Attachment: OS.File.writeAtomic now renames (again) by default, v2

Nicer API, better documentation, and a few additional tests.

Comment 4

[Nathan Froyd [:froydnj]]

Comment on attachment 772080 [details] [diff] [review]
OS.File.writeAtomic now renames (again) by default, v2

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

::: toolkit/components/osfile/osfile_shared_front.jsm
@@ +368,5 @@
> +      bytesWritten = dest.write(buffer, options);
> +      if (options.flush) {
> +        dest.flush();
> +      }
> +      return bytesWritten;

I think this is slightly clearer if it is moved outside the try.  YMMV.

Comment 5

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

Attachment: OS.File.writeAtomic now renames (again) by default, v3

Applied final change, unbitrotted.

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c8163a513999

Comment 7

[Vladan Djeric (:vladan)]

(In reply to Nathan Froyd (:froydnj) from comment #2)
> Do we really need rename?  Can't we just say that if you provide tmpPath
> (which is required), then we do renaming, otherwise we write directly to the
> file?

Looks like I'm late to comment on this, but the purpose of this bug was to restore writeAtomic to renaming by default, for reasons laid out in Comment 0. Otherwise, there's nothing atomic about writeAtomic with default args afaict. 

Let's talk on IRC when I'm back at work on Monday

Comment 8

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/c8163a513999