Closed
Bug 890050
Opened 11 years ago
Closed 11 years ago
[OS.File] writeAtomic should always rename
Categories
(Toolkit Graveyard :: OS.File, defect)
Toolkit Graveyard
OS.File
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla25
People
(Reporter: vladan, Assigned: Yoric)
Details
Attachments
(1 file, 2 obsolete files)
20.42 KB,
patch
|
Yoric
:
review+
|
Details | Diff | Splinter Review |
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
Assignee | ||
Updated•11 years ago
|
Assignee: nobody → dteller
Assignee | ||
Comment 1•11 years ago
|
||
Attachment #771961 -
Flags: review?(nfroyd)
Comment 2•11 years ago
|
||
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.
Assignee | ||
Comment 3•11 years ago
|
||
Nicer API, better documentation, and a few additional tests.
Attachment #771961 -
Attachment is obsolete: true
Attachment #771961 -
Flags: review?(nfroyd)
Attachment #772080 -
Flags: review?(nfroyd)
Comment 4•11 years ago
|
||
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.
Attachment #772080 -
Flags: review?(nfroyd) → review+
Assignee | ||
Comment 5•11 years ago
|
||
Applied final change, unbitrotted.
Attachment #772080 -
Attachment is obsolete: true
Attachment #773296 -
Flags: review+
Assignee | ||
Updated•11 years ago
|
Keywords: checkin-needed
Comment 6•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/c8163a513999
Flags: in-testsuite+
Keywords: checkin-needed
Reporter | ||
Comment 7•11 years ago
|
||
(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•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/c8163a513999
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
Updated•11 months ago
|
Product: Toolkit → Toolkit Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•