[OS.File] writeAtomic without flush

RESOLVED FIXED in mozilla21

Status

()

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

People

(Reporter: Yoric, Assigned: Yoric)

Tracking

({dev-doc-needed, perf})

Trunk
mozilla21
dev-doc-needed, perf
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 3 obsolete attachments)

OS.File.writeAtomic is quite easy to use, but it provides guarantees that are expensive and generally not necessary, i.e. it flushes the file before renaming it. We should ensure that writing without flushing is as easy, before everybody ends up flushing all the time.

Here are a few options:
- by default, |writeAtomic| could not flush, and require an option |{flush: true}| to flush (cheap by default, not safe enough for some uses);
- by default, |writeAtomic| could flush, and accept an option |{noFlush: true}| to prevent flushing (expensive but safe by default);
- we could have a function |write| that behaves as |writeAtomic| but does not flush.
I guess option 2. is the safest course. Nathan, any suggestion?
Flags: needinfo?(nfroyd)
Yeah, I think option 2 (flush by default, option to avoid flushing) is the safest course.  Any way we can call the option "flush" and have it be true by default?
Flags: needinfo?(nfroyd)
It is possible, but probably inconsistent with the existing options of writeAtomic: noCopy and noOverwrite.
Created attachment 713320 [details] [diff] [review]
First version

Actually, I was wrong about |noCopy|, that was for another function. However, |noOverwrite| is correct.

Attaching a first version.
Assignee: nobody → dteller
Attachment #713320 - Flags: review?(nfroyd)
Keywords: dev-doc-needed, perf
Comment on attachment 713320 [details] [diff] [review]
First version

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

I'm pretty sure I complained about noOverwrite, and possibly even noCopy.  (Though ISTR that noCopy made a little more sense than noOverwrite.)  Just because we have made bad decisions in the past doesn't mean that we should continue to be consistently bad in the future. :)

Fix the name of the option (and by extension the logic); r=me with that change.
Attachment #713320 - Flags: review?(nfroyd) → review+
Created attachment 713785 [details] [diff] [review]
v2
Attachment #713320 - Attachment is obsolete: true
Attachment #713785 - Flags: review+
Created attachment 713817 [details] [diff] [review]
v2

Same one, with improved documentation.
Attachment #713785 - Attachment is obsolete: true
Attachment #713817 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/11a8c0dd4e70
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21

Comment 11

5 years ago
I'm not following the api reasoning here. The point of write to to temp, flush, rename is to minimize chances of corrupting existing data(eg when replacing data).

With flush-less operation, I'm not sure what the benefit of rename is. Feel we should just have a writeFile(filename,data) that open/write/closes.
Well, |writeAtomic| without flush attempts to reduce chances of corrupting existing data. It just doesn't attempt quite as hard as with flush. I don't understand the issue.

Comment 13

5 years ago
(In reply to David Rajchenbach Teller [:Yoric] from comment #12)
> Well, |writeAtomic| without flush attempts to reduce chances of corrupting
> existing data. It just doesn't attempt quite as hard as with flush. I don't
> understand the issue.

I don't understand the benefit of doing it this way vs just writing data out in a big write() call. 
 Is the goal to reduce chances of a write() that writes less than was asked?
That's the idea, indeed.
(In reply to Taras Glek (:taras) from comment #13)
> (In reply to David Rajchenbach Teller [:Yoric] from comment #12)
> > Well, |writeAtomic| without flush attempts to reduce chances of corrupting
> > existing data. It just doesn't attempt quite as hard as with flush. I don't
> > understand the issue.
> 
> I don't understand the benefit of doing it this way vs just writing data out
> in a big write() call. 
>  Is the goal to reduce chances of a write() that writes less than was asked?

Why is that relevant?  OS.File's write always tries to write all the data given to it.

Comment 16

5 years ago
(In reply to Nathan Froyd (:froydnj) from comment #15)
> (In reply to Taras Glek (:taras) from comment #13)
> > (In reply to David Rajchenbach Teller [:Yoric] from comment #12)
> > > Well, |writeAtomic| without flush attempts to reduce chances of corrupting
> > > existing data. It just doesn't attempt quite as hard as with flush. I don't
> > > understand the issue.
> > 
> > I don't understand the benefit of doing it this way vs just writing data out
> > in a big write() call. 
> >  Is the goal to reduce chances of a write() that writes less than was asked?
> 
> Why is that relevant?  OS.File's write always tries to write all the data
> given to it.

If the data is split across multiple write syscalls, you have a [slightly] higher chance of incomplete data being written out.
Good catch. Indeed, unless/until we extend |writeAtomic| to accept streams, the renaming stuff is useless with option |flush: false|.

I believe that we will want streams for OS.File at some point (to reduce lag when writing large strings or json objects), but for the moment, the best thing to do is probably to completely remove the renaming if |flush: false|. As you point out, the write remains somewhat atomic, insofar as we only have one write syscall, so I believe that this implementation should remain in function |writeAtomic|.

What do you think, Taras?
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Flags: needinfo?(taras.mozilla)

Comment 18

5 years ago
(In reply to David Rajchenbach Teller [:Yoric] from comment #17)
> Good catch. Indeed, unless/until we extend |writeAtomic| to accept streams,
> the renaming stuff is useless with option |flush: false|.
> 
> What do you think, Taras?
This sounds like an improvement.

I would still prefer to have a new function. Problem atm is that people use writeAtomic because it's so darn convenient...writeAtomic + parameter is less convenient, so we may still end up too much fsyncing.
Flags: needinfo?(taras.mozilla)
I understand your point. I think that I will still go along with |OS.File.writeAtomic| and an option for the moment, rather than a new function |OS.File.write| that does the same thing, because we might find a better use for name |OS.File.write| at some point.
Created attachment 716096 [details] [diff] [review]
v3 (on top of v2)
Attachment #716096 - Flags: review?(nfroyd)
Comment on attachment 716096 [details] [diff] [review]
v3 (on top of v2)

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

WFM.
Attachment #716096 - Flags: review?(nfroyd) → review+
Created attachment 717494 [details] [diff] [review]
v4 (on top of v2)

As v3, with more documentation and more tests.
Attachment #716096 - Attachment is obsolete: true
Attachment #717494 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/df3c51f6c752
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.