747876 - Efficient JS File API: synchronous front-end

Status: RESOLVED FIXED

(Core :: Networking: File, enhancement)

Description

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

Implement a cross-platform synchronous API on top of the Unix back-end (bug 707679) and the Windows back-end (bug 707681). This API should work on only on chrome workers.

Followup bug 729057 will deal with turning this synchronous API for workers in an asynchronous API for the main thread.

Comment 1

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

Attachment: Front-end glue

Comment 2

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

Attachment: Windows front-end

Comment 3

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

Attachment: Unix front-end

Comment 4

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

Attachment: Companion testsuite

Comment 5

[(dormant account)]

Comment on attachment 618660 [details] [diff] [review]
Windows front-end

A couple things.
+       write: function write(buffer, nbytes) {
+         this._ensureOpen();
+         WinFile.WriteFile(this._fd, buffer, nbytes, gBytesWrittenPtr);
+         return gBytesWritten.value;
+       },
Why no error handling. Is WinFile.WriteFile throwing an exception via some declareFFI-created-wrapper? As I said before, I'm against changing behavior of wrapped functions in any way(beyond cdatafinalizer safety valve).

Why is there worker/main-thread communication in declareFFI?

Comment 6

[(dormant account)]

Comment on attachment 618660 [details] [diff] [review]
Windows front-end

One of the reasons I asked to start with minimal functionality is because adding more stuff just increases your chances making a mistake
+     File.truncate = function truncate(path, options) {
+       return generic_open(path, false, Const.TRUNCATE_EXISTING, options);
+     };
^ wrong. Above is completely busted in presence of links(yes windows has those tool) and is much less efficient.
The correct way to do this is to seek + SetFileSize ala http://mxr.mozilla.org/mozilla-central/source/xpcom/glue/FileUtils.cpp#69

Please take out of all of APIs that are no strictly required from the higher level api(harder to make mistakes when doing mere wrappers).

Comment 7

[(dormant account)]

Comment on attachment 618661 [details] [diff] [review]
Unix front-end

+       setPositionFromEnd: function setPositionFromEnd(pos) {
+         this._ensureOpen();
+         return UnixFile.lseek(this._fd, pos, Const.SEEK_END);
+       },
+

I do not see utility in above & other setPosition*. it does nothing beyond obscuring the SEEK_END seek

r-ing because this code needs to be rewritten to take out exception handling from low level functions

Comment 8

[(dormant account)]

Comment on attachment 618662 [details] [diff] [review]
Companion testsuite

+  try {
+    test_init();
+    test_open_existing_file();
+    test_open_non_existing_file();
+    test_pump_existing_file();
+    test_copy_existing_file();
+    test_move_file();
+    test_append_to_file();
+    test_truncate_file();
+  } catch (x) {
+    log("Catching error: " + x);
+    log("Stack: " + x.stack);
+    ok(false, x.toString() + "\n" + x.stack);
+  }

Why not just let things fail?

Comment 9

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

(In reply to Taras Glek (:taras) from comment #5)
> Comment on attachment 618660 [details] [diff] [review]
> Windows front-end
> 
> A couple things.
> +       write: function write(buffer, nbytes) {
> +         this._ensureOpen();
> +         WinFile.WriteFile(this._fd, buffer, nbytes, gBytesWrittenPtr);
> +         return gBytesWritten.value;
> +       },
> Why no error handling. Is WinFile.WriteFile throwing an exception via some
> declareFFI-created-wrapper? As I said before, I'm against changing behavior
> of wrapped functions in any way(beyond cdatafinalizer safety valve).

Indeed, I have incorporated error-checking in the Type passed to declareFFI. |Type.zero_or_nothing| informs |declareFFI| that 0 is an error and that the function returns an unspecified value otherwise.

> Why is there worker/main-thread communication in declareFFI?

Er, what?

Comment 10

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

(In reply to Taras Glek (:taras) from comment #6)
> Comment on attachment 618660 [details] [diff] [review]
> Windows front-end
> 
> One of the reasons I asked to start with minimal functionality is because
> adding more stuff just increases your chances making a mistake
> +     File.truncate = function truncate(path, options) {
> +       return generic_open(path, false, Const.TRUNCATE_EXISTING, options);
> +     };
> ^ wrong. Above is completely busted in presence of links(yes windows has
> those tool) and is much less efficient.

Gasp. So much for the Windows API documentation.

> The correct way to do this is to seek + SetFileSize ala
> http://mxr.mozilla.org/mozilla-central/source/xpcom/glue/FileUtils.cpp#69

So three system calls instead of one is faster? Ok, I will fix that.

> Please take out of all of APIs that are no strictly required from the higher
> level api(harder to make mistakes when doing mere wrappers).

Well, as I mentioned yesterday, I still have no scenario and no clear idea which APIs are "strictly required". The two bugs mentioned by Dietrich when I asked for his use cases seem to require only "read whole file asynchronously as String" and "copy file asynchronously", so if you want, I can just throw away my synchronous front-end and just provide these two functions.

(In reply to Taras Glek (:taras) from comment #7)
> I do not see utility in above & other setPosition*. it does nothing beyond
> obscuring the SEEK_END seek

Well, the idea is simply to have the same function name for Windows and Unix, which is the whole point of this synchronous front-end. Using libc constants SEEK_END et al. on a API that also works under Windows would be rather bad style.

> r-ing because this code needs to be rewritten to take out exception handling
> from low level functions

(In reply to Taras Glek (:taras) from comment #8)
> Comment on attachment 618662 [details] [diff] [review]
> Companion testsuite
[...] 
> Why not just let things fail?

The idea is to get better stack reporting. I do not remember whether this is actually required, though.

Comment 11

[Doug Turner (:dougt)]

> ^ wrong. Above is completely busted in presence of links(yes windows has
> those tool) and is much less efficient.

We continue to care about links?  I did work in the nsifile stuff to support shortcuts - but was under the impression it was used very little (if not at all)

Comment 12

[Doug Turner (:dougt)]

I mean shortcuts...

Comment 13

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

Attachment: Companion testsuite

Comment 14

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

Attachment: Unix front-end

Shifted the exceptions from the back-end to the front-end, slightly changed the shape of the File object.

Comment 15

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

Attachment: Windows front-end

Exceptions moved from back-end to front-end, truncate function rewritten as asked. This code has not been tested yet, though.

Comment 16

[(dormant account)]

(In reply to Doug Turner (:dougt) from comment #12)
> I mean shortcuts...

i meant windows hardlinks :)

Comment 17

[(dormant account)]

Comment on attachment 618658 [details] [diff] [review]
Front-end glue

I'm leave this kind of stuff to dougt

Comment 18

[(dormant account)]

Comment on attachment 619067 [details] [diff] [review]
Unix front-end

+       close: function close() {
+         if (this._fd) {
+           let fd = this._fd;
+           this._fd = null;
+           delete this.fd;
+           Object.defineProperty(this, "fd", {get: File.prototype._nofd});
+           return this._closeResult = WinFile.close(fd);
+         }

WinFile, really?

I think rather than having setPositionFromEnd..etc, provide a single .seek and take an argument for START, CURRENT, END or whatever you want to call them.

for code like 
+         let result = UnixFile.lseek(this.fd, pos, Const.SEEK_CUR);
+         if (result == -1) {
+           throw new File.Error();
+         }
+         return result;

instead of cut'n'paste do
return checkError(UnixFile.lseek(this.fd, pos, Const.SEEK_CUR)) where checkError would do the error checking boilerplate

I'm not sure what Unix-style open is, your array of opens is more confusing than helpful. 

As I said before:instead of implementing everything, only implement functions as needed. 

Too much stuff here

Comment 19

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

(In reply to Taras Glek (:taras) from comment #18)
> Comment on attachment 619067 [details] [diff] [review]
> Unix front-end
> 
> +       close: function close() {
> +         if (this._fd) {
> +           let fd = this._fd;
> +           this._fd = null;
> +           delete this.fd;
> +           Object.defineProperty(this, "fd", {get: File.prototype._nofd});
> +           return this._closeResult = WinFile.close(fd);
> +         }
> 
> WinFile, really?

Oops. Forgot to |qref| before uploading.

> I think rather than having setPositionFromEnd..etc, provide a single .seek
> and take an argument for START, CURRENT, END or whatever you want to call
> them.

Ok. I marginally prefer my solution, as it avoids having to add yet another set of constants, but that's not big deal.

So, do you want me to put the constants in |OS.File|? In a subobject |OS.File.Constants|?


> for code like 
> +         let result = UnixFile.lseek(this.fd, pos, Const.SEEK_CUR);
> +         if (result == -1) {
> +           throw new File.Error();
> +         }
> +         return result;
> 
> instead of cut'n'paste do
> return checkError(UnixFile.lseek(this.fd, pos, Const.SEEK_CUR)) where
> checkError would do the error checking boilerplate

ok

> I'm not sure what Unix-style open is, your array of opens is more confusing
> than helpful. 

Well, that was the result of a design discussion with Dietrich, Paul O'Shannessy and Drew Willcoxon on the topic of OS.File API a few months ago, plus a more recent discussion with Jason Orendorff on the topic of OS-specific APIs.

One of the main ideas is to avoid "one size doesn't quite fit anybody" APIs such as the file opening API of nspr/xpcom that require Unix-specific options even on Windows. Rather, we offer a set of APIs that cover the most common cases that can be made portable without performance penalty (that's "open", "truncate", "truncateC", ...), plus OS-specific APIs (prefixed by "unix" or "win"), to handle the myriad of cases in which no portable solution can be envisioned.

Requires more documentation, of course.

> As I said before:instead of implementing everything, only implement
> functions as needed. 
> 
> Too much stuff here

I fully agree with the general idea. As I mentioned, for the moment, I cannot do this until I have a clear idea of what is needed, so I am experimenting with nice API design, in the hope of obtaining something that I can show to potential users and get some feedback. It was my understanding that this was also something that you wanted.

As I said before, if you could point me towards what is needed, that would be great. As I mentioned, for the moment, the only scenarios I have seen that do not involve a full API, both provided by Dietrich, are:
1/ read a full file to a string;
2/ copy a file.

As I told you, scenario 1 is not really possible in the current state of js-ctypes, but I plan to work on patches to make it possible (bug 552551).

So, if you prefer, as I wrote above, I can throw away all that code for the moment (or move it to another bug) and just provide file copy.

What do you think, Taras?

Comment 20

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

Attachment: Unix front-end

Attaching a new version.

ChangeLog:
- factored the error-handling mechanism;
- File.prototype.close now raises an error if |close| returns -1;
- plenty more documentation and clarifications;
- this time, the qref has been done :)
- added several missing error checks;
- now, all implementations of |copy| and |move| implement option |noOverwrite|.

Now, Taras, as I mentioned, if you can just tell me which features you want in priority, I will be able to divide the patch in smaller chunks adding only these features.

Comment 21

[(dormant account)]

(In reply to David Rajchenbach Teller [:Yoric] from comment #19)
> (In reply to Taras Glek (:taras) from comment #18)
> > Comment on attachment 619067 [details] [diff] [review]
> > Unix front-end
> > 
> > +       close: function close() {
> > +         if (this._fd) {
> > +           let fd = this._fd;
> > +           this._fd = null;
> > +           delete this.fd;
> > +           Object.defineProperty(this, "fd", {get: File.prototype._nofd});
> > +           return this._closeResult = WinFile.close(fd);
> > +         }
> > 
> > WinFile, really?
> 
> Oops. Forgot to |qref| before uploading.
> 
> > I think rather than having setPositionFromEnd..etc, provide a single .seek
> > and take an argument for START, CURRENT, END or whatever you want to call
> > them.
> 
> Ok. I marginally prefer my solution, as it avoids having to add yet another
> set of constants, but that's not big deal.
> 
> So, do you want me to put the constants in |OS.File|? In a subobject
> |OS.File.Constants|?
> 
> 
> > for code like 
> > +         let result = UnixFile.lseek(this.fd, pos, Const.SEEK_CUR);
> > +         if (result == -1) {
> > +           throw new File.Error();
> > +         }
> > +         return result;
> > 
> > instead of cut'n'paste do
> > return checkError(UnixFile.lseek(this.fd, pos, Const.SEEK_CUR)) where
> > checkError would do the error checking boilerplate
> 
> ok

I still see explicit if(!...) thow Error code in new patches.
> 
> > I'm not sure what Unix-style open is, your array of opens is more confusing
> > than helpful. 
> 
> Well, that was the result of a design discussion with Dietrich, Paul
> O'Shannessy and Drew Willcoxon on the topic of OS.File API a few months ago,
> plus a more recent discussion with Jason Orendorff on the topic of
> OS-specific APIs.
> 
> One of the main ideas is to avoid "one size doesn't quite fit anybody" APIs
> such as the file opening API of nspr/xpcom that require Unix-specific
> options even on Windows. Rather, we offer a set of APIs that cover the most
> common cases that can be made portable without performance penalty (that's
> "open", "truncate", "truncateC", ...), plus OS-specific APIs (prefixed by
> "unix" or "win"), to handle the myriad of cases in which no portable
> solution can be envisioned.
> 
> Requires more documentation, of course.

I think this way insanity lies. I think we should do what common apis(python, fopen) do and specify file mode as a string. permissions can be an optional argument.

I will strongly r- code with a bunch of similar looking open/create/etc functions. Having lots of really similar, but slightly different functions will increase the maintenance burden.


> 
> > As I said before:instead of implementing everything, only implement
> > functions as needed. 
> > 
> > Too much stuff here
> 
> I fully agree with the general idea. As I mentioned, for the moment, I
> cannot do this until I have a clear idea of what is needed, so I am
> experimenting with nice API design, in the hope of obtaining something that
> I can show to potential users and get some feedback. It was my understanding
> that this was also something that you wanted.
> 
> As I said before, if you could point me towards what is needed, that would
> be great. As I mentioned, for the moment, the only scenarios I have seen
> that do not involve a full API, both provided by Dietrich, are:
> 1/ read a full file to a string;
> 2/ copy a file.
> 
> As I told you, scenario 1 is not really possible in the current state of
> js-ctypes, but I plan to work on patches to make it possible (bug 552551).
> 
> So, if you prefer, as I wrote above, I can throw away all that code for the
> moment (or move it to another bug) and just provide file copy.
> 
> What do you think, Taras?

I don't understand why you can't do 1 atm.

Comment 22

[(dormant account)]

Comment on attachment 619068 [details] [diff] [review]
Windows front-end

as i mentioned, too many variants of same function, error handling needs to be factored out.

Comment 23

[(dormant account)]

Comment on attachment 619520 [details] [diff] [review]
Unix front-end

catch_negative -> throw_on_negative

As I mentioned elsewhere seek/read/open/etc functions need to be unified. It's it's up to you where you put the seek, etc constants.

If something is unclear, please chat with me before posting a revised patch.

Comment 24

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

(In reply to Taras Glek (:taras) from comment #21)
> > What do you think, Taras?
> 
> I don't understand why you can't do 1 atm.

Let's discuss this on irc. Essentially, two reasons:
- no support for encodings other than utf-8;
- no support for improperly encoded strings (i.e. typically what you get from one |read| operation).

Comment 25

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

(In reply to Taras Glek (:taras) from comment #23)
> Comment on attachment 619520 [details] [diff] [review]
> Unix front-end
> 
> catch_negative -> throw_on_negative

You are right, that's clearer.

> I think this way insanity lies. I think we should do what common apis(python,
> fopen) do and specify file mode as a string. permissions can be an optional
> argument.

[...]

Permissions are not an issue. Well, besides the fact that they are very much not portable between Unix and Windows, which I solve with the |options| object.

> As I mentioned elsewhere seek/read/open/etc functions need to be unified.
> It's it's up to you where you put the seek, etc constants.

[...]

> I will strongly r- code with a bunch of similar looking open/create/etc 
> functions. Having lots of really similar, but slightly different functions will
> increase the maintenance burden.

Ok. We are trading a little robustness in the API for a little robustness in the implementation, but this remains reasonable.


> If something is unclear, please chat with me before posting a revised patch.

Just to clarify: I posted the revised patch just to have a more readable base for discussion.

Comment 26

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

Attachment: Windows front-end

Comment 27

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

Attachment: Additional libc constants, for BSD copy

Comment 28

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

Attachment: Unix front-end

Comment 29

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

Attachment: Companion testsuite

Comment 30

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

Attachment: Companion testsuite

Comment 31

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

Attachment: Unix front-end

Comment 32

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

Attachment: Windows front-end

Comment 33

[(dormant account)]

Comment on attachment 624015 [details] [diff] [review]
Windows front-end

+         if (options && options.from) {
That's wrong when from is 0. .hasOwnProperty('from') or typeof options.from =='int'

+           throw_on_zero(
+             WinFile.ReadFile(this.fd, buffer, nbytes, gBytesReadPtr, null)
+           );
+           this.setPosition(pos);
+         } else {
+           throw_on_zero(
+             WinFile.ReadFile(this.fd, buffer, nbytes, gBytesReadPtr, null)
+           );

This is a weird api...read() should always set the file position after what was read in.

same with writeI)...especially write
write has same problems

I'm still not sure what the point of winOpen is.

+         case undefined:
+         case 'r':

add // fall through
when falling through a case on purpose

there is no default handler when you pass in a mode that cant be parsed..it should throw an exception. 

I also doubt that passing modes as a string is the right thing to do(even if i suggested it). This is not C, you can pass an array of [Const.* modes] and skip the parsing step.

+           WinFile.chdir(path); //FIXME: Not implemented ????

GetCurrentDirectory loop seems crazy, should expose MAX_PATH and use that + 1 for file length

Also, allocating on with a +1 buffer increase is crazy inefficient.


I don't see the point of the pump function...even if i were convinced that we need pumping I would say that buffer allocation should be the responsibility of the user(allows more buffer reuse and feature less fragile state)

Comment 34

[(dormant account)]

Comment on attachment 624014 [details] [diff] [review]
Unix front-end

Lets get one platform r+ed and then we can r+ the other...I prefer to review windows to start with.

Comment 35

[(dormant account)]

Comment on attachment 624013 [details] [diff] [review]
Companion testsuite

This is useful for reference, but i don't need to review this until we have r+ed the api.

Comment 36

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

(In reply to Taras Glek (:taras) from comment #33)
> Comment on attachment 624015 [details] [diff] [review]
> Windows front-end
> 
> +         if (options && options.from) {
> That's wrong when from is 0. .hasOwnProperty('from') or typeof options.from
> =='int'

Right.

> +           throw_on_zero(
> +             WinFile.ReadFile(this.fd, buffer, nbytes, gBytesReadPtr, null)
> +           );
> +           this.setPosition(pos);
> +         } else {
> +           throw_on_zero(
> +             WinFile.ReadFile(this.fd, buffer, nbytes, gBytesReadPtr, null)
> +           );
> 
> This is a weird api...read() should always set the file position after what
> was read in.
>
> 
> same with writeI)...especially write
> write has same problems

I was attempting to provide |pread|/|pwrite|-like functionality. If this is useless, I can drop that.

> I'm still not sure what the point of winOpen is.

Well, the idea is to let users access a lower-level API, by providing a function that can be used exactly as the Windows API.

Typical use would look like

  let file;
  if (OS.Win) {
    file = OS.Win.winOpen(/*my exact args, I know what I am doing*/)
  } else if (OS.Unix) {
    file = OS.Unix.unixOpen(/*my exact args, I know what I am doing*/)
  }
  // ...

Now, since I have added options winShare, winSecurity, etc., we can decide that this feature is redundant.

> 
> +         case undefined:
> +         case 'r':
> 
> add // fall through
> when falling through a case on purpose
> 
> there is no default handler when you pass in a mode that cant be parsed..it
> should throw an exception.

Good catch. In the Unix version, it throws a |TypeError|, but I obviously forgot to put this in the Windows version.

> I also doubt that passing modes as a string is the right thing to do(even if
> i suggested it). This is not C, you can pass an array of [Const.* modes] and
> skip the parsing step.

Mmmhh... Ok. Not the actual |Const| modes (these wouldn't be portable), but I can do something. Note that this will end up much more verbose:

  OS.File.open("foo", [OS.File.open.APPEND, OS.File.open.RW, OS.File.open.CREATE])

or something such. This will not really simplify the parsing step (we still need argument validation), but it might be easier on users.

We might also, more or less equivalently, introduce something along the lines of

  OS.File.open("foo", {append: true, rw: true, create: true})

A little easier on the eyes, but it is also easier to introduce a typo.

> 
> +           WinFile.chdir(path); //FIXME: Not implemented ????

Thanks.

> 
> GetCurrentDirectory loop seems crazy, should expose MAX_PATH and use that +
> 1 for file length

Unfortunately, under Windows, MAX_PATH is *not* the maximal length for a path. The maximal length for a path is "approximately" (sic) 32768 (doc is fuzzy, but it seems that this can be 32768 + the name of the drive or share), while the value of MAX_PATH, iirc, is 4095. I do not think that we want to allocate 32kb each time we request a path, so we have to try and be smarter.

> Also, allocating on with a +1 buffer increase is crazy inefficient.

Good thing I am not doing it, then :)
I am allocating with the value returned by |GetCurrentDirectory|, which is the required size of the buffer.

Rewriting the algorithm to make this clearer.

> I don't see the point of the pump function...even if i were convinced that
> we need pumping I would say that buffer allocation should be the
> responsibility of the user(allows more buffer reuse and feature less fragile
> state)

In that case, let's remove it. The Linux version needs it to implement copy (using splice), but we can probably do without on other platforms.

Comment 37

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

Attachment: Windows front-end

Attaching an updated patch. Removed winOpen and pump, fixed option.from in read/write, added some documentation on read/write, changed handling of modes in open, clarified get current directory.

Comment 38

[(dormant account)]

Comment on attachment 624323 [details] [diff] [review]
Windows front-end

+
+     let gBytesRead = new ctypes.int32_t(-1);
+     let gBytesReadPtr = gBytesRead.address();
+     let gBytesWritten = new ctypes.int32_t(-1);
+     let gBytesWrittenPtr = gBytesWritten.address();

these should be 'private' with _ prefix, right? I don't see a reason to expose these.
> 
> I was attempting to provide |pread|/|pwrite|-like functionality. If this is
> useless, I can drop that.
> 

drop that

> > I'm still not sure what the point of winOpen is.
> 
> Well, the idea is to let users access a lower-level API, by providing a
> function that can be used exactly as the Windows API.
> 

Usually you let apps use a native open..and provide functionality to import the fd..we can add that later when we actually need it


> We might also, more or less equivalently, introduce something along the
> lines of
> 
>   OS.File.open("foo", {append: true, rw: true, create: true})
> 
> A little easier on the eyes, but it is also easier to introduce a typo.
> 

Lets do that, lets also add validation such that if a bonus/unexpected options argument is passed, we throw an exception. We should do the options validation in a followup

pump didn't get removed

Comment 39

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

(In reply to Taras Glek (:taras) from comment #38)
> Comment on attachment 624323 [details] [diff] [review]
> Windows front-end
> 
> +
> +     let gBytesRead = new ctypes.int32_t(-1);
> +     let gBytesReadPtr = gBytesRead.address();
> +     let gBytesWritten = new ctypes.int32_t(-1);
> +     let gBytesWrittenPtr = gBytesWritten.address();
> 
> these should be 'private' with _ prefix, right? I don't see a reason to
> expose these.

They are actually private, just as closure-local variables, rather than as per-file fields. I am taking advantage of the fact that two concurrent read/write cannot take place concurrently to save (a little) memory and (a little) time.

> > I was attempting to provide |pread|/|pwrite|-like functionality. If this is
> > useless, I can drop that.
> > 
> 
> drop that

Done.

> > > I'm still not sure what the point of winOpen is.
> > 
> > Well, the idea is to let users access a lower-level API, by providing a
> > function that can be used exactly as the Windows API.
> > 
> 
> Usually you let apps use a native open..and provide functionality to import
> the fd..we can add that later when we actually need it

Done.

> > We might also, more or less equivalently, introduce something along the
> > lines of
> > 
> >   OS.File.open("foo", {append: true, rw: true, create: true})
> > 
> > A little easier on the eyes, but it is also easier to introduce a typo.
> > 
> 
> Lets do that, lets also add validation such that if a bonus/unexpected
> options argument is passed, we throw an exception. We should do the options
> validation in a followup

Let me attach a version that uses an object, with some option validation.

> pump didn't get removed

Oops. Done.

Comment 40

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

Attachment: Windows front-end

Thanks for the quick review. Here is a new (untested) version.

Comment 41

[(dormant account)]

Comment on attachment 624495 [details] [diff] [review]
Windows front-end

This seems ok

Comment 42

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

Attachment: Companion testsuite

Comment 43

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

Attachment: Windows front-end

Comment 44

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

I have updated the Windows and Unix front-ends to put them at the same level of features. Along the way, I have found a few issues in the Windows front-end, so if you want a re-review, here is the update:
- more documentation;
- removed some debugging code;
- added two |because*| getters to determine error reasons;
- a few |let| => |const|;
- added a mode flag |existing| for |open|, to open the file only if it exists;
- reworked the code that handles mode for |open| into something a little less ad-hoc;
- handled "truncate an existing file", as per your earlier recommendations.

Comment 45

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

Attachment: Companion testsuite

Comment 46

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

Attachment: Front-end glue

Comment 47

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

Attachment: Additional windows constants, for error-checking

Comment 48

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

Attachment: Unix front-end

Comment 49

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

Attachment: Companion testsuite

Comment 50

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

Attachment: Additional libc constants, for BSD copy

Comment 51

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

Attachment: Windows front-end

Comment 52

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

Attachment: Unix front-end

Comment 53

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

Attachment: Additional windows constants, for error-checking

Comment 54

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

Attachment: Front-end glue

Comment 55

[(dormant account)]

Comment on attachment 626449 [details] [diff] [review]
Windows front-end

+           case "existing":
+             existing = true;
+           default:


missing break

What's with because in the property names? Seems like they would be just as good without, no?

Comment 56

[(dormant account)]

Comment on attachment 626450 [details] [diff] [review]
Unix front-end

+           case "existing":
+             existing = true;
+           default:
+             throw new TypeError("Mode " + key + " not understood");
+           }
+         }

Same bug as in windows frontend. This leads me to believe that you should factor out the open argument parsing and share it.(post a followup patch to the windows code so dont have to rereview the whole thing)

I image it would take mode and return an object with members for 
+         let read = false;
+         let write = false;
+         let trunc = false;
+         let create = false;
+         let existing = false;

is the pump fallback useful on any platform in current code?

splice can fail even if it is available:
  EINVAL Target file system doesn't support splicing; target file is opened in append mode; neither of the descriptors refers to a pipe; or offset given for nonseekable device.


We need to work on filesystems too lazy to implement splice(fallback on sendfile?)

Also I'm ok with landing code that only works within a single (decent) filesystem and them applying fallbacks as a followup(ie for move, copy, splice)

Comment 57

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

(In reply to Taras Glek (:taras) from comment #55)
> Comment on attachment 626449 [details] [diff] [review]
> Windows front-end
> 
> +           case "existing":
> +             existing = true;
> +           default:
> 
> 
> missing break

Thanks.

> What's with because in the property names? Seems like they would be just as
> good without, no?

Sorry, I don't understand the question.

Comment 58

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

(In reply to Taras Glek (:taras) from comment #56)
> Comment on attachment 626450 [details] [diff] [review]
> Unix front-end
> 
> +           case "existing":
> +             existing = true;
> +           default:
> +             throw new TypeError("Mode " + key + " not understood");
> +           }
> +         }
> 
> Same bug as in windows frontend. This leads me to believe that you should
> factor out the open argument parsing and share it.(post a followup patch to
> the windows code so dont have to rereview the whole thing)
>
> 
> I image it would take mode and return an object with members for 
> +         let read = false;
> +         let write = false;
> +         let trunc = false;
> +         let create = false;
> +         let existing = false;
>

Ok, I'll find a way to factor this out.
 
> is the pump fallback useful on any platform in current code?

I do not think it is necessary for any tier one platform. I included this because I am pretty sure that there are some Unices that implement neither |splice| nor |copyfile|, and I wanted to have a fallback.

> splice can fail even if it is available:
>   EINVAL Target file system doesn't support splicing; target file is opened
> in append mode; neither of the descriptors refers to a pipe; or offset given
> for nonseekable device.
> 
> 
> We need to work on filesystems too lazy to implement splice(fallback on
> sendfile?)

Should work on Linux, but I'm not 100% confident that it will work on other OSes. For instance, on BSD, I believe that |sendfile| is only for sockets.

> Also I'm ok with landing code that only works within a single (decent)
> filesystem and them applying fallbacks as a followup(ie for move, copy,
> splice)

Ok.

Comment 59

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

Attachment: Windows front-end, continued

Is this what you had in mind?

Comment 60

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

Comment on attachment 626449 [details] [diff] [review]
Windows front-end

I'll ask dougt to review this once we have a final version.

Comment 61

[(dormant account)]

Comment on attachment 626749 [details] [diff] [review]
Windows front-end, continued

I would like to get rid of discrete variables in this.
+       let read = false;
+       let write = false;
+       let trunc = false;
+       let create = false;
+       let existing = false;
->
let ret = {
read:false, write:false...
}
+         case "truncate":
+           trunc = true;
+           write = true;
->
ret.trunk -> true
...
return ret
  let {
+           read, write, trunc, create, existing
+         } = OS.Shared._aux.normalizeOpenMode(options);

let mode = OS.Shared._aux.normalizeOpenMode(options);

but otherwise looks good

Comment 62

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

Attachment: Windows front-end, part 3

Here we go. I will fold the patches a little differently once I have your approval, Taras.

Comment 63

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

Attachment: Unix front-end, part 2

Applying requested changes to Unix front-end, too. Again, I will do some re-folding somewhere along the way.

Comment 64

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

Attachment: Scope-based resource control

This function proved useful both for part 2 of the Unix front-end and for the ongoing work on thumbnails.

Comment 65

[(dormant account)]

Comment on attachment 628286 [details] [diff] [review]
Scope-based resource control

10:56 <@taras> and i think instead of having magic cleanup function
10:56 <@taras> and passing resources
10:56 <@taras> you should just pass in 2 functions
10:56 <@taras> cleanup, action
10:57 <@taras> otherwise too much magic is happening in cleanup(), afaik
10:57 < Yoric> Well, I have the feeling that the common scenario is either:
10:57 < Yoric> - closing one file;
10:57 < Yoric> - or closing many files.
10:57 <@taras> so you make a function called try_finally
10:57 <@taras> and try_finally_fd
10:57 <@taras> for the common scenario
10:57 < Yoric> ok
10:57 <@taras> and try_finally_fsls for the other one
10:58 <@taras> err
10:58 <@taras> try_finally_fdls

it's not obvious what using does from name. I propose try_finally

Comment 66

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

Attachment: Unix front-end

Unix front-end, consolidated in one patch.

Comment 67

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

Attachment: Shared code

Shared code, extracted from its patch.

Comment 68

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

Attachment: Windows front-end

Windows front-end, consolidated in one patch.

Comment 69

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

Comment on attachment 626452 [details] [diff] [review]
Front-end glue

And a simpler review to rest a little bit after the complex stuff :)

Comment 70

[Doug Turner (:dougt)]

Comment on attachment 628734 [details] [diff] [review]
Windows front-end

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

on mac and unix, paths use '/' as the file node seperator.  on windows, it is '\'.  Suppose I need to use this API, and say -- want to delete a file that exists at $HOME/doomtext.  How do we do that in an cross platform way?

::: toolkit/components/osfile/osfile_win_front.jsm
@@ +42,5 @@
> +     // and we use the following global mutable values:
> +     let gBytesRead = new ctypes.int32_t(-1);
> +     let gBytesReadPtr = gBytesRead.address();
> +     let gBytesWritten = new ctypes.int32_t(-1);
> +     let gBytesWrittenPtr = gBytesWritten.address();

these are no more global than WinFile declared above, right?  These are basically instance member variables.  You probably should drop the 'g' prefix.  Maybe use 'm' instead?

@@ +56,5 @@
> +      */
> +     let File = function File(fd) {
> +       this._fd = fd;
> +     };
> +     File.prototype = {

add newline whitespace before File.prototype.

@@ +69,5 @@
> +       // Placeholder getter, used to replace |get fd| once
> +       // the file is closed.
> +       _nofd: function nofd(operation) {
> +         operation = operation ||
> +             this._nofd.caller.name ||

This is different than the unix one.  You added this._nofd.caller.name here.  Which is correct?

@@ +88,5 @@
> +       close: function close() {
> +         if (this._fd) {
> +           let fd = this._fd;
> +           this._fd = null;
> +           delete this.fd;

why the explict delete here?

@@ +118,5 @@
> +        * the end of the file has been reached.
> +        * @throws {OS.File.Error} In case of I/O error.
> +        */
> +       read: function read(buffer, nbytes, options) {
> +         // |gBytesReadPtr| is a pointer to |gBytesRead|.

useless comment.

@@ +175,5 @@
> +         // In this implementation,
> +         // OS.File.POS_START == OS.Constants.Win.FILE_BEGIN
> +         // OS.File.POS_CURRENT == OS.Constants.Win.FILE_CURRENT
> +         // OS.File.POS_END == OS.Constants.Win.FILE_END
> +         whence = (whence == undefined)?Const.FILE_BEGIN:whence;

spaces between operators

@@ +322,5 @@
> +
> +       let share = options.winShare || DEFAULT_SHARE;
> +       let security = options.winSecurity || null;
> +       let flags = options.winFlags || DEFAULT_FLAGS;
> +       let template = options.winTemplate?options.winTemplate._fd:null;

same - add spaces around ? and :

Comment 71

[Doug Turner (:dougt)]

Comment on attachment 628727 [details] [diff] [review]
Unix front-end

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

::: toolkit/components/osfile/osfile_unix_front.jsm
@@ +397,5 @@
> +       };
> +     } else {
> +       // If the OS does not implement file copying for us, we need to
> +       // implement it ourselves. For this purpose, we need to define
> +       // a pumping function.

What platform would this code run on?  What platforms do not have copyfile(3)?

Comment 72

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

Attachment: Windows front-end

Comment 73

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

Attachment: Unix front-end

Fixing a few typoes.

Comment 74

[Doug Turner (:dougt)]

Comment on attachment 630983 [details] [diff] [review]
Windows front-end

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

r with those changes.

::: toolkit/components/osfile/Makefile.in
@@ +26,5 @@
>  	$(NSINSTALL) $(srcdir)/osfile_shared.jsm $(FINAL_TARGET)/modules/osfile
>  	$(NSINSTALL) $(srcdir)/osfile_unix_back.jsm $(FINAL_TARGET)/modules/osfile
>  	$(NSINSTALL) $(srcdir)/osfile_unix_front.jsm $(FINAL_TARGET)/modules/osfile
>  	$(NSINSTALL) $(srcdir)/osfile_win_back.jsm $(FINAL_TARGET)/modules/osfile
> +	$(NSINSTALL) $(srcdir)/osfile_win_front.jsm $(FINAL_TARGET)/modules/osfile
\ No newline at end of file

\ No newline at end of file

::: toolkit/components/osfile/osfile_win_front.jsm
@@ +333,5 @@
> +                 ||(!("winAccess" in options) && "winDisposition" in options)) {
> +         throw new TypeError("OS.File.open requires either both options " +
> +           "winAccess and winDisposition or neither");
> +       } else {
> +         mode = OS.Shared._aux.normalizeOpenMode(mode);

Declare 'mode' above with the rest.

Comment 75

[Doug Turner (:dougt)]

Comment on attachment 630986 [details] [diff] [review]
Unix front-end

I am unsure where UnixFile.copyfile isn't going to be defined.  Why do we need this code?

Comment 76

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

(In reply to Doug Turner (:dougt) from comment #75)
> Comment on attachment 630986 [details] [diff] [review]
> Unix front-end
> 
> I am unsure where UnixFile.copyfile isn't going to be defined.  Why do we
> need this code?

UnixFile.copyfile is only defined under BSD/MacOS X. For all other platforms, we need to reimplement it.

Comment 77

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

(In reply to Doug Turner (:dougt) from comment #74)
> Comment on attachment 630983 [details] [diff] [review]
> Windows front-end
> 
> Review of attachment 630983 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r with those changes.
> 
> ::: toolkit/components/osfile/Makefile.in
> 
> \ No newline at end of file

Done.

> 
> ::: toolkit/components/osfile/osfile_win_front.jsm
> @@ +333,5 @@
> > +                 ||(!("winAccess" in options) && "winDisposition" in options)) {
> > +         throw new TypeError("OS.File.open requires either both options " +
> > +           "winAccess and winDisposition or neither");
> > +       } else {
> > +         mode = OS.Shared._aux.normalizeOpenMode(mode);
> 
> Declare 'mode' above with the rest.

Are you sure? I am just normalizing an argument. Declaring it might make code a little messier.

Comment 78

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

Attachment: Shared code

Comment 79

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

Attachment: Front-end glue

Comment 80

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

Attachment: Additional windows constants, for error-checking

Comment 81

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

Attachment: Windows front-end

Comment 82

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

Attachment: Additional libc constants, for BSD copy

Comment 83

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

Attachment: Unix front-end

Comment 84

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

Attachment: Companion testsuite

Comment 85

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

Ahaha, everything is reviewed.
I am tracking down an intermittent orange. I will land this once I have found it.

Comment 86

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

Attachment: Companion testsuite

Comment 87

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

Attachment: Shared code

Comment 88

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

Attachment: Front-end glue

Comment 89

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

Attachment: Unix front-end

Comment 90

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

Attachment: Windows front-end

Comment 91

[Tim Taubert [:ttaubert] (inactive)]

Will land.

Comment 92

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/integration/fx-team/rev/3cde4e8722eb
https://hg.mozilla.org/integration/fx-team/rev/9e4b5966a2e8
https://hg.mozilla.org/integration/fx-team/rev/03f35867f598
https://hg.mozilla.org/integration/fx-team/rev/4f0670e0828a
https://hg.mozilla.org/integration/fx-team/rev/06ae8c3e0898
https://hg.mozilla.org/integration/fx-team/rev/4adb8c310f9d
https://hg.mozilla.org/integration/fx-team/rev/4ef5ee480ddd

Comment 93

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/3cde4e8722eb
https://hg.mozilla.org/mozilla-central/rev/9e4b5966a2e8
https://hg.mozilla.org/mozilla-central/rev/03f35867f598
https://hg.mozilla.org/mozilla-central/rev/4f0670e0828a
https://hg.mozilla.org/mozilla-central/rev/06ae8c3e0898
https://hg.mozilla.org/mozilla-central/rev/4adb8c310f9d
https://hg.mozilla.org/mozilla-central/rev/4ef5ee480ddd

Comment 94

[Landry Breuil (:gaston)]

re https://hg.mozilla.org/mozilla-central/rev/9e4b5966a2e8, i'm afraid the copyfile API is OSX only. There's no copyfile.h on OpenBSD, and after a bit of googling there doesnt seem to be one on FreeBSD either.

I'm pretty sure it should be included only in the XP_MACOSX case..

Comment 95

[Landry Breuil (:gaston)]

from http://www.manpagez.com/man/3/copyfile/

HISTORY
     The copyfile() API was introduced in Mac OS X 10.5.

Comment 96

[Landry Breuil (:gaston)]

Attachment: Only include copyfile.h on mac

Only include copyfile.h on mac, tests passes fine here now :

/usr/obj/m-c/ $TEST_PATH=toolkit/components/osfile/tests/mochi/ gmake mochitest-chrome
...
201 INFO Passed: 195
202 INFO Failed: 0
...

Comment 97

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

Thanks for the quick patch, gaston.

Comment 98

[Landry Breuil (:gaston)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/b9981690ede0 (and indeed, i've put the wrong bug # in the commit message... DOH!)

Comment 99

[Ed Morley [:emorley]]

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

Landry, I don't suppose you could put a note in your calendar to add a reference pointing to here in bug 767876 once it exists? Would just help hg blame :-)

Comment 100

[Landry Breuil (:gaston)]

(In reply to Ed Morley [:edmorley] from comment #99)
> https://hg.mozilla.org/mozilla-central/rev/b9981690ede0
> 
> Landry, I don't suppose you could put a note in your calendar to add a
> reference pointing to here in bug 767876 once it exists? Would just help hg
> blame :-)

I'd have if i could.. but the now-existing bug 767876 is 'access denied' for everyone apparently :(