Closed Bug 886997 Opened 6 years ago Closed 6 years ago

[OS.File] writeAtomic should not flush by default

Categories

(Toolkit :: OS.File, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla25

People

(Reporter: Yoric, Assigned: Yoric)

References

Details

(Keywords: dev-doc-complete)

Attachments

(1 file, 1 obsolete file)

No description provided.
Attached patch Don't flush by default (obsolete) — Splinter Review
After discussing with vladan, we decided to change the default behavior of OS.File.writeAtomic.
Assignee: nobody → dteller
Attachment #767439 - Flags: review?(nfroyd)
Comment on attachment 767439 [details] [diff] [review]
Don't flush by default

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

Just to make sure I understand the rationale for this change: OS.File is meant to provide a convenient interface to file operations from JavaScript; it chooses performance over safety when there is a clear demarcation between the two.  Therefore, we are opting to not flush by default because many uses will not require the safety guarantees (maybe that is too strong a word) made by flushing *and* will be adversely affected by the performance cost.  Is that a fair summary?

I'm just a little worried that many people are going to cargo-cult writeAtomic calls without realizing that they *do* need the safety guarantees.

All the comments for osfile_shared_front.jsm apply equally to ofsfile_async_front.jsm.

::: toolkit/components/osfile/osfile_shared_front.jsm
@@ +309,5 @@
> + * until the contents are fully written, the destination file is
> + * not modified.
> + *
> + * By default, files are not flushed. If your data is user-critical
> + * (e.g. preferences, application data, etc.), you may with to pass

"you may wish"

@@ +325,5 @@
>   * - {string} tmpPath The path at which to write the temporary file.
>   * - {bool} noOverwrite - If set, this function will fail if a file already
>   * exists at |path|. The |tmpPath| is not overwritten if |path| exist.
> + * - {bool} flush - If set to |true|, the function will flush the
> + * file. This is considerably slower behavior is slightly safer: if

Perhaps you meant to say "This considerably slower behavior is slightly safer"?

@@ +327,5 @@
>   * exists at |path|. The |tmpPath| is not overwritten if |path| exist.
> + * - {bool} flush - If set to |true|, the function will flush the
> + * file. This is considerably slower behavior is slightly safer: if
> + * the system shuts down improperly (typically due to a kernel freeze
> + * or a power failure) or if the device is disconnected or removed

Nit: I'd drop the "or removed".  It makes the sentence read better and is arguably subsumed by "disconnected".

@@ +328,5 @@
> + * - {bool} flush - If set to |true|, the function will flush the
> + * file. This is considerably slower behavior is slightly safer: if
> + * the system shuts down improperly (typically due to a kernel freeze
> + * or a power failure) or if the device is disconnected or removed
> + * before the buffer is flushed, the file has more changes of not being

"has more chances"
Attachment #767439 - Flags: review?(nfroyd) → review+
(In reply to Nathan Froyd (:froydnj) from comment #2)
> Comment on attachment 767439 [details] [diff] [review]
> Don't flush by default
> 
> Review of attachment 767439 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Just to make sure I understand the rationale for this change: OS.File is
> meant to provide a convenient interface to file operations from JavaScript;
> it chooses performance over safety when there is a clear demarcation between
> the two.  Therefore, we are opting to not flush by default because many uses
> will not require the safety guarantees (maybe that is too strong a word)
> made by flushing *and* will be adversely affected by the performance cost. 
> Is that a fair summary?

Well, the situation is the following:
- the additional fsync provides very little additional safety but the cost is, in some unpredictable cases, measurably quite high;
- the additional fsync is not sufficient to guarantee that the file is actually safely stored on disk but can contribute to lulling users in an unwarranted sense of absolute safety.

So, rather than being a case of "fast over safe" it's a case of "fast over still not safe enough."
https://hg.mozilla.org/mozilla-central/rev/9ec0070b8378
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
You need to log in before you can comment on or make changes to this bug.