886997 - [OS.File] writeAtomic should not flush by default

Status: RESOLVED FIXED

(Toolkit Graveyard :: OS.File, defect)

Comment 1

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

Attachment: Don't flush by default

After discussing with vladan, we decided to change the default behavior of OS.File.writeAtomic.

Comment 2

[Nathan Froyd [:froydnj]]

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"

Comment 3

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

(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."

Comment 4

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

Attachment: Don't flush by default, v2

Applied feedback.
Try: https://tbpl.mozilla.org/?tree=Try&rev=58368022357f

Comment 5

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

inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/9ec0070b8378

Comment 6

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/9ec0070b8378