924858 - [OS.File] Inconsistent cross-platform behavior due to O_APPEND

Status: RESOLVED FIXED

(Toolkit Graveyard :: OS.File, defect)

Description

[Nils Maier [:nmaier]]

Attachment: Testcase

The Unix-implementation of OS.File.open will default to O_APPEND, which creates inconsistent behavior between platforms, as Windows and/or the Windows-implementation does not implement this flag.

STR:
- Run the attached testcase (scratchpad for e.g. about:newtab) on Unix: OK
- Run on Windows: FAIL

Having the O_APPEND stuff the default is contra-intuitive anyway IMO, so I'd welcome seeing this bug fixed by not making O_APPEND the default, or even removing it from the available constants.

Comment 1

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

By specification, by default, we open in append mode, so this is a bug in the Windows implementation.
It seems that we fail to add FILE_APPEND_DATA to |access|.

Comment 2

[Nils Maier [:nmaier]]

FILE_APPEND_DATA will just give the right to append data.
O_APPEND will actually always append data by seeking to the end before writes.
I don't think Windows has an equivalent to O_APPEND (see http://mxr.mozilla.org/mozilla-central/source/nsprpub/pr/src/io/prfile.c#70)

Also, O_APPEND behavior by default (or at all) is bad and unexpected IMO and hence the specification should be changed.

Comment 3

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

I believe that this is too late for this purpose. The documentation/specification has been made available more than 1 year ago, so this is now "it's a bug" land rather than "it's a bad design" land. Otherwise, who knows what we could break unintentionally?

To fix the issue, we should call
 file.setPosition(0, File.POS_END)
if we are appending.

Comment 4

[Nils Maier [:nmaier]]

Any consumer relying on this right now is screwed anyway, as the Windows implementation behaves completely differently to the Unix. If somebody would actually rely on the append behavior today, their code would be broken on the majority platform. So existing code is either broken, unaffected (and will stay unaffected) or has that setPosition work-around already. I would have expected people doing the latter would have also reported this bug...
I'd also suspect that a lot of people don't even know exactly what "the file will be opened for appending" does actually mean - or what O_APPEND is for that matter. 

You cannot sanely fix this bug and please anybody: It will either lead to a behavioral change on Windows or on Unix with a risk of breaking things unintentionally.
So I'm still very much in favor of dropping the O_APPEND default (and "break" Unix) instead of adding a setPosition default-hack (and "break" Windows).
For people wanting append mode with known caveats, you could still create an |append| flag.

If you still go for making it the default, there must be an |append| flag as well, to turn it off, as the only way to do it right now - specifying your own |unixFlags| - isn't cross-platform.

Regarding setPosition, here is a quote from linux open(2):
> O_APPEND may lead to corrupted files on NFS file systems if more than one process appends data to a file at once. This is because NFS does not support appending to a file, so the client kernel has to simulate it, which can't be done without a race condition.

You'd be introducing the same race-condition with setPosition, affecting all of Windows... The nspr and MSVCRT open/fopen implementations also suffer from this race as well, FWIW.


The docs need to update, to talk about the behavioral differences that are shipping in all versions right now.

Comment 5

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

... unless it's used already on Android- or FirefoxOS-specific code or in an Android add-on, in which case the developers only care about the Unix version.

If you can convince me that this is not the case, I'll be happy to discuss API changes.

Comment 6

[Nils Maier [:nmaier]]

I can't seem to find any Android or B2G specific code using OS.File at all, let alone code that would be affected. Add-ons MXR seems down for me right now...
Anyway, proving a negative... Can you convince me that there are no Windows add-ons that would break?

Comment 7

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

Well, Niels^H^H^H^H^HNils, you are resilient :)

I would rather we take the chance of breaking Windows add-ons that rely on a bug and misuse the API (there are trivial ways to not append) rather than breaking Android add-ons or B2G code that relies on the API doing what it says. Doing otherwise would be somewhat insulting to our users, I believe.

However, what makes you believe it is so bad to append when we don't truncate?

Comment 8

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

Adding Nathan to the conversation, this might interest him.

Comment 9

[Nils Maier [:nmaier]]

(In reply to David Rajchenbach Teller [:Yoric] from comment #7) 
> (there are trivial ways to not append)

No, there are not. You need to come up with your own unixFlags, which isn't cross-platform either. Therefore you'd need at a very least to add an |append| flag to |options|. While I prefer it to be |false| by default, which I tried to make a case for, you prefer it to stay |true| by default.

In the end I'd be fine with either way, but I need to be able to reliably set append mode cross-platform, preferably without having to specify platform-specific flags.

> rather than
> breaking Android add-ons or B2G code that relies on the API doing what it
> says. Doing otherwise would be somewhat insulting to our users, I believe.

Insulting is a strong word.
B2G seems unaffected.
Should there be any affected Android-only add-on, which I honestly doubt, there will be a simple and backward-compatible fix (just add |append: true| to your flags) and we already have an amo-editors team, compatibility newsletter and blog posts for add-on developers...
Also, there is a far better probability that some test or moz-QA will catch any potentially breakage in Android/B2G and the code is then fixed before shipping a release than add-on authors fixing their stuff in time or at all. And as the majority add-on development platform still appears to be Windows, this would suggest keeping the current Windows behavior of no-default-append. 

> However, what makes you believe it is so bad to append when we don't
> truncate?

- It by default behaves differently to nsIFileInputStream (default: no-append) that most folks will be migrating from.
- It by default behaves differently from virtually any other other file I/O API in any language/platform I know.
- It has known race conditions on Linux with remote file systems
- It has known race conditions on ALL other platforms
- It needs a hack to even work on Windows at all
- It generally does not feel right: If I want to write to a file, I don't necessarily want to append to it.

Comment 10

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

(In reply to Nils Maier [:nmaier] from comment #9)
> (In reply to David Rajchenbach Teller [:Yoric] from comment #7) 
> > (there are trivial ways to not append)
> 
> No, there are not. You need to come up with your own unixFlags, which isn't
> cross-platform either. Therefore you'd need at a very least to add an
> |append| flag to |options|. While I prefer it to be |false| by default,
> which I tried to make a case for, you prefer it to stay |true| by default.
> 
> In the end I'd be fine with either way, but I need to be able to reliably
> set append mode cross-platform, preferably without having to specify
> platform-specific flags.

Well, the reliable way to not append is |trunc: true| (once we have fixed the bug, that is). The behavior is:
- if |trunc|, create or reopen, truncate;
- otherwise, if |create|, create;
- otherwise, append.

What would you change? Replace |trunc| with |append|?

> - It by default behaves differently to nsIFileInputStream (default:
> no-append) that most folks will be migrating from.
> - It by default behaves differently from virtually any other other file I/O
> API in any language/platform I know.
> - It has known race conditions on Linux with remote file systems
> - It has known race conditions on ALL other platforms
> - It needs a hack to even work on Windows at all
> - It generally does not feel right: If I want to write to a file, I don't
> necessarily want to append to it.

The race condition is a good point. Hack on Windows, well, everything needs hacks on Windows, so I won't accept that argument :)

Comment 11

[Nathan Froyd [:froydnj]]

(In reply to David Rajchenbach Teller [:Yoric] from comment #10)
> Well, the reliable way to not append is |trunc: true| (once we have fixed
> the bug, that is). The behavior is:
> - if |trunc|, create or reopen, truncate;
> - otherwise, if |create|, create;
> - otherwise, append.
> 
> What would you change? Replace |trunc| with |append|?

Do we have a good way to just open for writing without append and without truncating?  Do we care about that case?

Comment 12

[Nils Maier [:nmaier]]

(In reply to Nathan Froyd (:froydnj) from comment #11)
> Do we have a good way to just open for writing without append and without
> truncating?

Right now: No, not a good way, just |unixFlags| and whatever will happen with a Windows fix. That's why there needs to be a cross-platform |append| flag.

> Do we care about that case?

I do care about this case, as it is actually exactly my use case. DownThemAll! will open single a file multiple times at different positions writing data received from different connections. Without a way to write files without append and without trunc, I won't be able to use OS.File in a sane manner.

There might be other uses, e.g. with certain binary formats where you might only need to toggle certain bits at well known locations (and would do a copy, write, rename dance to be save). E.g. certain formats would write some data not fully known when the write starts but leave room for a fixed size header and once done seek back to the end to write the actual header. In O_APPEND, the seek back would be a no-op and result in an invalid file. The format is not always up to the developer to specify, meaning you cannot always just fix the format design. 

(In reply to David Rajchenbach Teller [:Yoric] from comment #10)
> Well, the reliable way to not append is |trunc: true| (once we have fixed
> the bug, that is). The behavior is:
> - if |trunc|, create or reopen, truncate;
> - otherwise, if |create|, create;
> - otherwise, append.
> 
> What would you change? Replace |trunc| with |append|?

No, add |append|, regardless of the its default value. This is a must-have showstopper thing. Nothing to do with |trunc|.
See above.

Comment 13

[Nathan Froyd [:froydnj]]

(In reply to Nils Maier [:nmaier] from comment #12)
> (In reply to Nathan Froyd (:froydnj) from comment #11)
> > Do we have a good way to just open for writing without append and without
> > truncating?
> 
> Right now: No, not a good way, just |unixFlags| and whatever will happen
> with a Windows fix. That's why there needs to be a cross-platform |append|
> flag.

OK, fine.

> > Do we care about that case?
> 
> I do care about this case, as it is actually exactly my use case.

Thank you for the description.

So it sounds like we have two neatly-separable issues:

1) The Windows implementation is buggy.  This should be fixed, and I am inclined to say that we should just make Windows follow the specification here.  That is, the absence of |trunc| in |options| should append, unlike what it does now on Windows.

2) We need to add some capability for opening a file for writing without specifying the moral equivalent of O_APPEND *or* O_TRUNC.  Whether this new capability is the default or requires specifying extra flags is a separate discussion.

Comment 14

[Nils Maier [:nmaier]]

I edited https://developer.mozilla.org/en-US/docs/JavaScript_OS.File/OS.File_for_workers#OS.File.open%28%29 and also the "on main thread" articles to reflect the current situation adding to the description of |write|
> Unless create or trunc are set or explicit unixFlags are given, the file will be opened for appending on Unix/Linux. However, the file is not opened for appending on Windows at the time of writing. See bug 924858.

I also fixed |truncate| to say |trunc|, because only the latter is actually used in code.

Comment 15

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

Ok, I finally understand what you needed, Nils.
For the moment, let's land a patch that matches the specifications, since that's a bug, and then, let's move to a new bug and actually implement the behavior that you need.

Comment 16

[Nils Maier [:nmaier]]

I thought about this a bit more and, regardless of append-by-default or not, the current Unix behavior does not make any sense at all:
Why do |trunc| or |create| never set O_APPEND? The default should be either "append is always on" or "append is never on", not "append is sometimes on, good luck with figuring out exactly when..."

That is:
file = File.OS.open(path, ...);
file.write(Uint8Array(1000));
file.setPosition(0, 0);
file.read(100);
file.write(Uint8Array(x));
should either always append to the end or always write at offset 100 (no append) regardless of the mode flags for |trunc| or |create|.

Comment 17

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

Let's move the proposed API changes to bug 925865.

Comment 18

[Nils Maier [:nmaier]]

Attachment: Part 1 - Add |append| mode flag to OS.File.open on Unix.

Actually, I'd prefer to add an append flag as part of this bug, as the flag is needed on Windows anyway (without it, there would be no way to turn the behavior off).

Comment 19

[Nils Maier [:nmaier]]

Attachment: Part 2 - Implement |append| mode for OS.File.open on Windows.

Comment 20

[Nils Maier [:nmaier]]

Alright, the patches will NOT change the default append behavior, so they are supposed to be backwards-compatible to *nix (but not Windows, of course).

I was thinking about adding a global mutex to the windows implementation to at least avoid races between concurrent OS.File threads on a given system, but that didn't really seem like to much overhead for something that would be still broken anyway (e.g. when non-OS.File processes write to the file).
Any thoughts?

Comment 21

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

I don't think that the global mutex is a good idea. It will make the execution weirder without managing to save users from themselves (or from us :) ). I believe that a documentation detailing the possible race condition is more critical in the short run. Once the bug is fixed, we'll need to brainstorm API improvements.

Comment 22

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

Comment on attachment 816060 [details] [diff] [review]
Part 2 - Implement |append| mode for OS.File.open on Windows.

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

Looks good.
Could you add a test?

P.S.: Ok, I confess, you are right, O_APPEND was not a very good idea.

Comment 23

[Nils Maier [:nmaier]]

(In reply to David Rajchenbach Teller [:Yoric] from comment #22)
> Comment on attachment 816060 [details] [diff] [review]
> Part 2 - Implement |append| mode for OS.File.open on Windows.
> 
> Review of attachment 816060 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good.
> Could you add a test?

The tests from Part 1 - Unix will also run for Part 2 - Windows. Or did you have anything Windows-specific in mind?

Comment 24

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

Comment on attachment 816059 [details] [diff] [review]
Part 1 - Add |append| mode flag to OS.File.open on Unix.

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

Ah, my bad, I seem to have missed Part 1.
Looks good, thanks.

::: toolkit/components/osfile/tests/xpcshell/test_osfile_async_append.js
@@ +10,5 @@
> + * (see bug 925865)
> + */
> +
> +// Test append mode.
> +function test_append(mode) {

Please make it return a Task.

@@ +20,5 @@
> +      yield OS.File.writeAtomic(path, new Uint8Array(500));
> +    }
> +
> +    // Open the file and write something.
> +    mode.read = mode.write = true;

I realize it's a test, but overwriting arguments is not very nice. I'd rather either you performed a copy or you created an indirection with Object.create().

Comment 25

[Nils Maier [:nmaier]]

Attachment: Part 1 - Add |append| mode flag to OS.File.open on Unix.

Unbitrotted. Also fixed File.copy regarding splice (splice doesn't work with O_APPEND)

Comment 26

[Nils Maier [:nmaier]]

Attachment: Part 2 - Implement |append| mode for OS.File.open on Windows.

Unbitrotted. Requesting another (fast) review to make sure the patch conforms to the new "style".

Comment 27

[Nils Maier [:nmaier]]

(In reply to Nils Maier [:nmaier] from comment #25)
> Created attachment 818845 [details] [diff] [review]
> Part 1 - Add |append| mode flag to OS.File.open on Unix.
> 
> Unbitrotted. Also fixed File.copy regarding splice (splice doesn't work with
> O_APPEND)

I should also add, that this is on top of bug 928239 now, and therefore the xpcshell.ini hunk won't apply without it.

Comment 28

[Nils Maier [:nmaier]]

Green try: https://tbpl.mozilla.org/?tree=Try&rev=8b06fd6b37f1

Comment 29

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

Could you add an explicit dependency to bug 928239?

Comment 30

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

Comment on attachment 818846 [details] [diff] [review]
Part 2 - Implement |append| mode for OS.File.open on Windows.

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

::: toolkit/components/osfile/modules/osfile_win_front.jsm
@@ +337,5 @@
>           access, share, security, disposition, flags, template));
> +
> +       if (mode.append) {
> +         file._appendMode = true;
> +       }

I'd rather have
 file._appendMode = !!mode.append
Fields that may or may not exist are good neither for readability nor for JIT performance.

Comment 31

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

Comment on attachment 818845 [details] [diff] [review]
Part 1 - Add |append| mode flag to OS.File.open on Unix.

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

Looks good, thanks.

Comment 32

[Nils Maier [:nmaier]]

(In reply to David Rajchenbach Teller [:Yoric] from comment #30)
> I'd rather have
>  file._appendMode = !!mode.append
> Fields that may or may not exist are good neither for readability nor for
> JIT performance.

Good point!

Comment 33

[Nils Maier [:nmaier]]

Attachment: Part 1 - Add |append| mode flag to OS.File.open on Unix. r=yoric

Unbitrotted (offsets). Carrying over r=yoric.
Part of this green try: https://tbpl.mozilla.org/?tree=Try&rev=62ec046931e3

Comment 34

[Nils Maier [:nmaier]]

Attachment: Part 2 - Implement |append| mode for OS.File.open on Windows. r=yoric

Unbitrotted (offsets). Carrying over r=yoric.
Part of this green try: https://tbpl.mozilla.org/?tree=Try&rev=62ec046931e3

Comment 35

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/a8af01ed5f52
https://hg.mozilla.org/integration/fx-team/rev/2c93630e3109

Comment 36

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/a8af01ed5f52
https://hg.mozilla.org/mozilla-central/rev/2c93630e3109

Comment 37

[Nils Maier [:nmaier]]

I added docs for |append| and also more details regarding different Gecko versions concerning append mode incl. examples on how to open existing files without it.
https://developer.mozilla.org/en-US/docs/JavaScript_OS.File/OS.File_for_the_main_thread
https://developer.mozilla.org/en-US/docs/JavaScript_OS.File/OS.File_for_workers