840436 - [OS.File] writeAtomic without flush

Status: RESOLVED FIXED

(Toolkit Graveyard :: OS.File, defect)

Description

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

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.

Comment 1

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

I guess option 2. is the safest course. Nathan, any suggestion?

Comment 2

[Nathan Froyd [:froydnj]]

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?

Comment 3

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

It is possible, but probably inconsistent with the existing options of writeAtomic: noCopy and noOverwrite.

Comment 4

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

Attachment: First version

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

Attaching a first version.

Comment 5

[Nathan Froyd [:froydnj]]

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.

Comment 6

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

Attachment: v2

Comment 7

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

Try: https://tbpl.mozilla.org/?tree=Try&rev=76485a1f470c

Comment 8

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

Attachment: v2

Same one, with improved documentation.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/11a8c0dd4e70

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/11a8c0dd4e70

Comment 11

[(dormant account)]

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.

Comment 12

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

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

[(dormant account)]

(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?

Comment 14

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

That's the idea, indeed.

Comment 15

[Nathan Froyd [:froydnj]]

(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

[(dormant account)]

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

Comment 17

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

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?

Comment 18

[(dormant account)]

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

Comment 19

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

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.

Comment 20

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

Attachment: v3 (on top of v2)

Comment 21

[Nathan Froyd [:froydnj]]

Comment on attachment 716096 [details] [diff] [review]
v3 (on top of v2)

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

WFM.

Comment 22

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

Attachment: v4 (on top of v2)

As v3, with more documentation and more tests.

Comment 23

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

Try: https://tbpl.mozilla.org/?tree=Try&rev=760901f5401a

Comment 24

[Ryan VanderMeulen [:RyanVM]]

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

Comment 25

[Ryan VanderMeulen [:RyanVM]]

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