707679 - Efficient JS File API - Unix back-end

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

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

Implement the JS File API backend.

A few considerations:
- Linux/Android and BSD/MacOS differ on a number of subtle-yet-interesting points (e.g. Linux/Android has a number of `openat`-style system calls optimized for opened directories, BSD/MacOS has optimized tree walking, additional `fstat` properties and `open` flags, etc.);
- this back-end needs to handle Unix-style permissions;
- Unix-style file properties is rather simple, as `fstat` extracts plenty of meaningful information in one operation;
- in some contexts, information may be obtained through less-expensive operations, e.g. determining whether a file is a directory while walking a directory.

Comment 1

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

Attachment: Preview 2.

Still very much a work in progress.

Since previous preview:
- introduce a "native path" type, as suggested – as a typedef, though, I believe people would lynch me if I intend to add a new hierarchy of strings;
- removed a few functions/methods that had become useless by the way of this change;
- implemented a public |fstat| API;
- removed methods for reading a complete file, as expected;
- renamed |Rmdir| => |Remove|, |Rmcontents| => |Clear|;
- collapsed the variants of |Move|.

Mike, do you think that this |Resolve|/|Unresolve| API is meaningful? Or should we assume that a directory is always |Resolved| and should therefore be closed as early as possible?

Comment 2

[(dormant account)]

Lets get moving on landing this(ie switch to r?). Is it feasible to land something before we branch on Dec 20?

Comment 3

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

(In reply to Taras Glek (:taras) from comment #2)
> Lets get moving on landing this(ie switch to r?). Is it feasible to land
> something before we branch on Dec 20?

I can prioritize this, but the Windows version is lagging.

Comment 4

[(dormant account)]

(In reply to David Rajchenbach Teller [:Yoric] from comment #3)
> (In reply to Taras Glek (:taras) from comment #2)
> > Lets get moving on landing this(ie switch to r?). Is it feasible to land
> > something before we branch on Dec 20?
> 
> I can prioritize this, but the Windows version is lagging.

please do

Comment 5

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

Attachment: Prev 3. Unix back-end.

I attach a version of the Unix back-end.

The architecture of RawFile should not change much. I still need to implement the following features:
- changing attributes (chown, chmod, chgrp);
- truncate;
- dup (should be useful for clean multi-threaded operations);
- in the future, play nicely with AIO.

By opposition, RawDirectory is still at the stage of untested, early work in progress. The architecture of RawDirectory remains a big question. I am very interested in feedback about |Resolve|/|Unresolve|. The main reason for including these functions are for users who could be tempted to cache many directories for later use – something that we do with |nsIFile| atm. However, at JS-level, we have a data structure to represent paths. I tend to believe that we should recommend that users should never hold onto a RawDirectory and simply |Resolve| all directories on platforms for which it matters. To discourage this, at JS-level, it would be quite easy to display warnings if a directory (or a file) remains open for more than, say, 30 minutes.

Finally, we are missing:
- creation of temporary files;
- |copy|, |touch| a file in a RawDirectory;
- |move|, |copy|, |rm|, |touch| an arbitrary file in the fs;
- getting the contents of a directory (with platform-specific info).

Comment 6

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

Attachment: Prev 3. Back-end common files.

Comment 7

[(dormant account)]

Comment on attachment 581986 [details] [diff] [review]
Prev 3. Unix back-end.

Break this up atleast into 3 patches:
c/c++ bits
basic js bits
more js bitss

Comment 8

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

Attachment: Prev 4. Unix back-end.

Here's the same patch, minus the first few lines, which had been included by error. Sorry about that.

Comment 9

[(dormant account)]

Comment on attachment 582523 [details] [diff] [review]
Prev 4. Unix back-end.

This seems to be doing a lot for a thin layer. I think we should be passing all of the flags from JS. Forcing flags to be defined in C++ makes it impossible for addons to utilize a flag that we didn't provide.

Comment 10

[(dormant account)]

Comment on attachment 581988 [details] [diff] [review]
Prev 3. Back-end common files.

-ing since i -sed part4. This is not minimal enough.

Comment 11

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

(In reply to Taras Glek (:taras) from comment #9)
> Comment on attachment 582523 [details] [diff] [review]
> Prev 4. Unix back-end.
> 
> This seems to be doing a lot for a thin layer.

I am not really sure how I could do less while still giving access to OS-accelerated features.

> I think we should be passing
> all of the flags from JS. Forcing flags to be defined in C++ makes it
> impossible for addons to utilize a flag that we didn't provide.

Ok, I will refactor |OpenFlags| to allow that.

(In reply to Taras Glek (:taras) from comment #10)
> Comment on attachment 581988 [details] [diff] [review]
> Prev 3. Back-end common files.
> 
> -ing since i -sed part4. This is not minimal enough.

Just to be sure I understand: you want me to split the patch, is that it?

Comment 12

[(dormant account)]

(In reply to David Rajchenbach Teller [:Yoric] from comment #11)
> (In reply to Taras Glek (:taras) from comment #9)
> > Comment on attachment 582523 [details] [diff] [review]
> > Prev 4. Unix back-end.
> > 
> > This seems to be doing a lot for a thin layer.
> 
> I am not really sure how I could do less while still giving access to
> OS-accelerated features.

I think the api shouldn't provide much more than a syscall wrapper. Then the js side should figure out which syscall to use and what parameters to pass.

> 
> > I think we should be passing
> > all of the flags from JS. Forcing flags to be defined in C++ makes it
> > impossible for addons to utilize a flag that we didn't provide.
> 
> Ok, I will refactor |OpenFlags| to allow that.
> 
> (In reply to Taras Glek (:taras) from comment #10)
> > Comment on attachment 581988 [details] [diff] [review]
> > Prev 3. Back-end common files.
> > 
> > -ing since i -sed part4. This is not minimal enough.
> 
> Just to be sure I understand: you want me to split the patch, is that it?

yes. I like patches split up to be <15KB each(when reasonable).

Comment 13

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

(In reply to Taras Glek (:taras) from comment #12)
> (In reply to David Rajchenbach Teller [:Yoric] from comment #11)
> > (In reply to Taras Glek (:taras) from comment #9)
> > > Comment on attachment 582523 [details] [diff] [review]
> > > Prev 4. Unix back-end.
> > > 
> > > This seems to be doing a lot for a thin layer.
> > 
> > I am not really sure how I could do less while still giving access to
> > OS-accelerated features.
> 
> I think the api shouldn't provide much more than a syscall wrapper. Then the
> js side should figure out which syscall to use and what parameters to pass.

We could put all the logics into JS. I am not quite clear what we would gain, but I see a number of potential issues, which have made me choose this slightly thicker back-end:
- the resulting API is usable only by JS, not by C++;
- we lose performance, both in the implementation itself, and at boundaries (e.g. many Windows data structures are 64 bits or 128 bits, so we have to represent them as arrays in JS);
- we make it much easier to have resource leaks
- we lose some useful compile-time checking.

If you strongly believe that I should go for the syscall wrapper, I will go that way, but I believe that the current approach is better.

> yes. I like patches split up to be <15KB each(when reasonable).

Ok, will do.

Comment 14

[(dormant account)]

Just to be clear: I strongly believe that we should go for the syscall wrapper approach.

Comment 15

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

Sure. Let me just finish cleaning up my patch queue and I'll start this new strategy.

Comment 16

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

Attachment: Exporting libc constants to JS

Attaching a first draft. This module exports a global object |OS.Constants.libc|, with many (all?) of the important libc constants. I envision that this module can be later extended with additional constants as needed.

Comment 17

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

Attachment: Unix synchronous back-end

Attaching a fully revamped version of the Unix back-end. As discussed with Taras, this back-end is synchronous for the moment, and designed to be used only from a chrome worker thread. Once we have reached the stage at which this synchronous back-end works to satisfaction, we will build the asynchronous back-end on top of it. And only once the asynchronous back-end works will we start working first on the Windows back-end, then on the actual user-facing API.

Comment 18

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

Attachment: Companion testsuite

Comment 19

[Alexandre Poirot [:ochameau]]

Comment on attachment 609850 [details] [diff] [review]
Unix synchronous back-end

Nice. I just can't f- any js-ctypes patch!!

Wouldn't it be easier to use `new Type` instead of this 3th levels indirection?
  declareType > aDeclareType > gDeclareType > new Type()
Especially when, at the end, we do want a Type instance at the end.
(It would simplify code comprehension and allow removing init arguments that aren't used)
Similar comment apply to declareFFI, we might call gDeclareFFI directly.

Otherwise it looks really nice, js-ctypes burden doesn't look that bad in this library!
We are trying to provide some js-ctypes helpers in order to help addon developers start using it. And I think that your experience around js-ctype is really valuable. We may just take your helper methods and offer them in a ctype helper jetpack module!

What's the plan next? Would you build some cross platform library on top of that? I'm saying that because Jetpack FS API is cross platform, so that we would have to to this job, if you do not plan to do it.

Finally, what about CPU/memory performances? Especially compared to current xpcom components.

I can't wait to get other OSes and asynchronous support :)

Comment 20

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

(In reply to Alexandre Poirot (:ochameau) from comment #19)
> Comment on attachment 609850 [details] [diff] [review]
> Unix synchronous back-end
> 
> Nice. I just can't f- any js-ctypes patch!!
> 
> Wouldn't it be easier to use `new Type` instead of this 3th levels
> indirection?
>   declareType > aDeclareType > gDeclareType > new Type()
> Especially when, at the end, we do want a Type instance at the end.
> (It would simplify code comprehension and allow removing init arguments that
> aren't used)

For Type, you are most likely right.

> Similar comment apply to declareFFI, we might call gDeclareFFI directly.

For declareFFI, it is a little more complicated. But I'm cheating, because I already have the implementation of the next bug at hand, and this argument serves for the threaded back-end. The main thread uses its own |declareFFI| to declare a function that checks and serializes arguments, deserializes the result and uses a Promise to synchronize, while the worker thread uses its own |declareFFI| to declare a function that deserializes arguments, performs the ffi call, serializes the result and posts it back to the main thread.

> Otherwise it looks really nice, js-ctypes burden doesn't look that bad in
> this library!
> We are trying to provide some js-ctypes helpers in order to help addon
> developers start using it. And I think that your experience around js-ctype
> is really valuable. We may just take your helper methods and offer them in a
> ctype helper jetpack module!

This would be with pleasure. Do you want to open a bug on extracting the relevant part and making them usable by Jetpack?

> What's the plan next? Would you build some cross platform library on top of
> that? I'm saying that because Jetpack FS API is cross platform, so that we
> would have to to this job, if you do not plan to do it.

Definitely planned. I used to have one, but it was scrapped in favor of this design, which is expected to be more hackable.

> Finally, what about CPU/memory performances? Especially compared to current
> xpcom components.

Untested yet. I suspect CPU/memory will be somewhat worse due to all conversions, but there should be less system calls, which we consider more important atm.
 
> I can't wait to get other OSes and asynchronous support :)

Thanks :)

Comment 21

[Mike Hommey [:glandium]]

Comment on attachment 609850 [details] [diff] [review]
Unix synchronous back-end

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

::: toolkit/components/osfile/osfile_unix.jsm
@@ +536,5 @@
> +                    /*dest*/   Types.string,
> +                    /*flag*/   Types.int);
> +
> +       UnixFile.lockf =
> +         declareFFI("lockf", ctypes.default_abi,

I'm not convinced it's a good idea to expose file locking functions.

@@ +555,5 @@
> +                    /*return*/ Types.string,
> +                    /*template*/Types.string);
> +
> +       UnixFile.mkstmp =
> +         declareFFI("mkstmp", ctypes.default_abi,

didn't you mean mkstemp ?

Comment 22

[(dormant account)]

(In reply to David Rajchenbach Teller [:Yoric] from comment #20)
> Untested yet. I suspect CPU/memory will be somewhat worse due to all
> conversions, but there should be less system calls, which we consider more
> important atm.
>  

I expect this to be faster because we do a lot of conversions in xpconnect.

Comment 23

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

(In reply to Taras Glek (:taras) from comment #22)
> (In reply to David Rajchenbach Teller [:Yoric] from comment #20)
> > Untested yet. I suspect CPU/memory will be somewhat worse due to all
> > conversions, but there should be less system calls, which we consider more
> > important atm.
> >  
> 
> I expect this to be faster because we do a lot of conversions in xpconnect.

There is only one way to settle this: Benchmark Kombat!

Comment 24

[(dormant account)]

Comment on attachment 609850 [details] [diff] [review]
Unix synchronous back-end

Please uses spaces around your operators.

I'm not sure what the benefit of wrapping FILE* functions is. I think wrapping basic fd-functions is sufficient for a low level api.

Comment 25

[(dormant account)]

(In reply to David Rajchenbach Teller [:Yoric] from comment #23)
> (In reply to Taras Glek (:taras) from comment #22)
> > (In reply to David Rajchenbach Teller [:Yoric] from comment #20)
> > > Untested yet. I suspect CPU/memory will be somewhat worse due to all
> > > conversions, but there should be less system calls, which we consider more
> > > important atm.
> > >  
> > 
> > I expect this to be faster because we do a lot of conversions in xpconnect.
> 
> There is only one way to settle this: Benchmark Kombat!

It's true. Another thing to keep in mind that JIT guys have a hope in hell in optimizing ctypes code at runtime, not such hopes for layering-heavy xpconnect :)

Comment 26

[(dormant account)]

(In reply to Taras Glek (:taras) from comment #24)
> Comment on attachment 609850 [details] [diff] [review]
> Unix synchronous back-end
> 
> Please uses spaces around your operators.
> 
> I'm not sure what the benefit of wrapping FILE* functions is. I think
> wrapping basic fd-functions is sufficient for a low level api.

I should clarify this. I think we should add functions as we need them instead of aggressively mirroring libc from getgo. The big benefit of an extensible ctypes-based api like this is that if we miss an api that a user might need, they can add it themselves.

Comment 27

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

Attachment: Unix synchronous back-end

Ok, I have removed some functions that are probably not going to be useful in the near future, fixed a few trivial issues, added a little documentation, removed |gDeclareType| et al and resolved the scope of |export| (thanks, ochameau).

Comment 28

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

Attachment: Companion testsuite

Extended the test suite, reworked it a little so that it has chances of working with Android. Additional tests: access, read, write, unlink.

Comment 29

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

Attachment: Companion makefile

Comment 30

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

Attachment: Companion testsuite

Comment 31

[(dormant account)]

Comment on attachment 612541 [details] [diff] [review]
Unix synchronous back-end

r+, but please get rid of *at functions until we need them.

A real tookit reviewer needs to review this before this lands.

Comment 32

[(dormant account)]

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

looks good

Comment 33

[(dormant account)]

Comment on attachment 612546 [details] [diff] [review]
Companion makefile

I don't review makefiles unless I wrote them

Comment 34

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

Attachment: Unix synchronous back-end

Comment 35

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

Attachment: Non-Unix specific functions of the back-end

Ok, I have removed *at, as requested by Taras, and I have taken the opportunity to divide the patch in two. First part of the patch contains the Unix specific bits, while the second part of the patch contains bits that are shared between Unix and Windows implem.

Comment 36

[(dormant account)]

Comment on attachment 612832 [details] [diff] [review]
Unix synchronous back-end

+     let libc_candidates =  ["libSystem.dylib",
+                             "libc.so.6",
+                             "libc.so"];
+     for (let i = 0; i < libc_candidates.length; ++i) {

use for each

I think it's better to use todo instead of "note".

Comment 37

[(dormant account)]

Comment on attachment 612837 [details] [diff] [review]
Non-Unix specific functions of the back-end

this seems ok. 

Nit:
+       let key = ":"+type.name;
put spaces around your operators, elsewhere too.

Comment 38

[Doug Turner (:dougt)]

Comment on attachment 612837 [details] [diff] [review]
Non-Unix specific functions of the back-end

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

fixed up the ws on checkin.  r+ if you have answers for my questions.

::: toolkit/components/osfile/osfile_shared.jsm
@@ +2,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +{
> +  if (typeof Components != "undefined") {

Is this really the right way to test that we are running this jsm on the right thread?

@@ +11,5 @@
> +
> +    throw new Error("osfile_shared.jsm cannot be used from the main thread yet");
> +  }
> +
> +  (function(exports) {

Why not do something like:

if (exports.OS)
  return;

exports.OS = {};
exports.OS.Shared = {}
...

@@ +64,5 @@
> +       this.name = name;
> +       this.implementation = implementation;
> +       if (convert_from_c) {
> +         this.convert_from_c = convert_from_c;
> +       } else {// Optimization: Ensure harmony of shapes

what is harmony of shapes?

@@ +65,5 @@
> +       this.implementation = implementation;
> +       if (convert_from_c) {
> +         this.convert_from_c = convert_from_c;
> +       } else {// Optimization: Ensure harmony of shapes
> +         this.convert_from_c = Type.prototype.convert_from_c;

this.convert_from_c = convert_from_c || Type.prototype.convert_from_c; ?

@@ +91,5 @@
> +      */
> +     Types.int =
> +       new Type("int",
> +                ctypes.int);
> +     Types.int32_t =

missing white space after the "int" type

@@ +110,5 @@
> +                ctypes.int);
> +     Types.off_t =
> +       new Type("off_t",
> +                ctypes.int);
> +     Types.size_t =

Missing white space after the off_t type

@@ +113,5 @@
> +                ctypes.int);
> +     Types.size_t =
> +       new Type("size_t",
> +                ctypes.size_t);
> +     Types.ssize_t =

same

@@ +116,5 @@
> +                ctypes.size_t);
> +     Types.ssize_t =
> +       new Type("ssize_t",
> +                ctypes.ssize_t);
> +     Types.DWORD =

same

@@ +185,5 @@
> +      */// Note: Future versions will use a different implementation of this
> +        // function on the main thread, osfile worker thread and regular worker
> +        // thread
> +     let declareFFI = function declareFFI(lib, symbol, abi,
> +                                           returnType /*, argTypes ...*/) {

another ws nit.  line up returnType with lib

@@ +190,5 @@
> +       LOG("Attempting to declare FFI ", symbol);
> +       // We guard agressively, to avoid any late surprise
> +       if (typeof symbol != "string") {
> +         if (symbol instanceof String) {
> +           symbol = symbol.toString();

is there ever a |symbol| that is a typeof string, but isn't a instance of String?

Comment 39

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

(In reply to Doug Turner (:dougt) from comment #38)
> Comment on attachment 612837 [details] [diff] [review]
> Non-Unix specific functions of the back-end
> 
> Review of attachment 612837 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> fixed up the ws on checkin.  r+ if you have answers for my questions.
> 
> ::: toolkit/components/osfile/osfile_shared.jsm
> @@ +2,5 @@
> > + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> > + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> > +
> > +{
> > +  if (typeof Components != "undefined") {
> 
> Is this really the right way to test that we are running this jsm on the
> right thread?

This is the only technique I know of, but I can investigate further with the JS team.

> > +  (function(exports) {
> 
> Why not do something like:
> 
> if (exports.OS)
>   return;
> 
> exports.OS = {};
> exports.OS.Shared = {}

Not 100% sure I understand your question. I am actually building |OS.| in several layers, so I need some more careful checking.

> what is harmony of shapes?

I want to ensure that the VM representation  of the object always has the exact same Shape (as in "these fields are defined in the immediate object, not in a prototype"). I have not benchmarked this, so this may well be premature optimization.

> 
> @@ +65,5 @@
> > +       this.implementation = implementation;
> > +       if (convert_from_c) {
> > +         this.convert_from_c = convert_from_c;
> > +       } else {// Optimization: Ensure harmony of shapes
> > +         this.convert_from_c = Type.prototype.convert_from_c;
> 
> this.convert_from_c = convert_from_c || Type.prototype.convert_from_c; ?

ok.

> 
> @@ +91,5 @@
> > +      */
> > +     Types.int =
> > +       new Type("int",
> > +                ctypes.int);
> > +     Types.int32_t =
> 
> missing white space after the "int" type

Not sure I understand. Do you want a new line?

> > +      */// Note: Future versions will use a different implementation of this
> > +        // function on the main thread, osfile worker thread and regular worker
> > +        // thread
> > +     let declareFFI = function declareFFI(lib, symbol, abi,
> > +                                           returnType /*, argTypes ...*/) {
> 
> another ws nit.  line up returnType with lib
ok

> 
> @@ +190,5 @@
> > +       LOG("Attempting to declare FFI ", symbol);
> > +       // We guard agressively, to avoid any late surprise
> > +       if (typeof symbol != "string") {
> > +         if (symbol instanceof String) {
> > +           symbol = symbol.toString();
> 
> is there ever a |symbol| that is a typeof string, but isn't a instance of
> String?

I had the case in a previous version of this code, but I probably do not need that check anymore. Do you want me to remove it?

Comment 40

[Doug Turner (:dougt)]

> Not sure I understand. Do you want a new line?

Yes, basically if you look at all of those assignments, most have one line of white space between them.  Just a nit for us to be consistent.


> Do you want me to remove it?

Yes, or add a comment as to why this case could exist.

Otherwise great stuff!

Comment 41

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

Thanks.

By the way, I have had confirmation from Jorendorff (and #developers silence): detecting the presence of |Components| seems to be the state of the art for detecting whether JS code is executed in the main thread or in a worker.

Comment 42

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

Attachment: Non-Unix specific functions of the back-end

Comment 43

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

Attachment: Unix synchronous back-end

Comment 44

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

Attachment: Companion testsuite

Comment 45

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

(In reply to Taras Glek (:taras) from comment #36)
> Comment on attachment 612832 [details] [diff] [review]
> Unix synchronous back-end
> 
> +     let libc_candidates =  ["libSystem.dylib",
> +                             "libc.so.6",
> +                             "libc.so"];
> +     for (let i = 0; i < libc_candidates.length; ++i) {
> 
> use for each

In another bug, Gavin recommended not using for each on arrays, as this can be fooled if the array has additional properties. While this is quite unlikely in a JS module, chrome workers make no such guarantee, so I would rather not take the chance.

> I think it's better to use todo instead of "note".

Well, the note is actually more an explanation of why things are designed like this, rather than a TODO.

Comment 46

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

Attachment: Unix synchronous back-end

A few minor updates to the various patches, reflecting current work on the front-end. I promise I will now stop updating anything other than bugs :)

Comment 47

[(dormant account)]

Comment on attachment 618690 [details] [diff] [review]
Unix synchronous back-end

+       UnixFile.close = function(fd) {
+         fd.dispose();
+         if (ctypes.errno) {
+           throw new OSUnix.Error(ctypes.errno, "close");
+         }
+       };
+

same as in the windows bug, please do not modify semantics of low level functions, instead directly call close(fd) and bypass the finalizer. (Sorry if I missed this before)

Comment 48

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

(In reply to Taras Glek (:taras) from comment #47)
> Comment on attachment 618690 [details] [diff] [review]
> Unix synchronous back-end
> 
> +       UnixFile.close = function(fd) {
> +         fd.dispose();
> +         if (ctypes.errno) {
> +           throw new OSUnix.Error(ctypes.errno, "close");
> +         }
> +       };
> +
> 
> same as in the windows bug, please do not modify semantics of low level
> functions, instead directly call close(fd) and bypass the finalizer. (Sorry
> if I missed this before)

Ok, I will remove all the exceptions everywhere. Can I at least return an error data structure instead of -1 / 0 / NULL / INVALID_HANDLE / ...?

Comment 49

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

Attachment: Unix synchronous back-end

Same code, without exceptions and with simpler |close|.

Comment 50

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

Attachment: Non-Unix specific functions of the back-end

Comment 51

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

Attachment: Companion testsuite

Propagated the changes to the test suite.

Comment 52

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

Attachment: Unix synchronous back-end

Same patch with a little more polish. Added a hack to ensure that we can check whether |open| returns -1, removed redundant |_strerror| and |OS.Unix.Error|.

Comment 53

[(dormant account)]

Comment on attachment 619056 [details] [diff] [review]
Unix synchronous back-end

+       UnixFile.close = function close(fd) {
+         return fd.dispose();
+       };

I think should should say

return _close(fd.forget()).

Comment 54

[(dormant account)]

(In reply to David Rajchenbach Teller [:Yoric] from comment #48)

> Ok, I will remove all the exceptions everywhere. Can I at least return an
> error data structure instead of -1 / 0 / NULL / INVALID_HANDLE / ...?

I think wrappers should be wrappers, we can introduce diversions from the platform at higher levels.

Comment 55

[(dormant account)]

Comment on attachment 618986 [details] [diff] [review]
Non-Unix specific functions of the back-end

I think that's ok.

Comment 56

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

(In reply to Taras Glek (:taras) from comment #53)
> Comment on attachment 619056 [details] [diff] [review]
> Unix synchronous back-end
> 
> +       UnixFile.close = function close(fd) {
> +         return fd.dispose();
> +       };
> 
> I think should should say
> 
> return _close(fd.forget()).

Well, both have the exact same semantics, but I would prefer keeping the |dispose| version, if you do not mind, as it is faster, shorter and (I believe) simpler.

Comment 57

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

Attachment: Unix synchronous back-end

Minor fix wrt the return of |OS.File.Unix.pipe| in case of error. Also, see my comment above regarding |dispose|.

Comment 58

[(dormant account)]

Comment on attachment 619541 [details] [diff] [review]
Unix synchronous back-end

+
+       UnixFile.pipe = function pipe(array) {
+         if (_pipe(_pipebuf) == -1) {
+           return -1;
+         }
+         array[0] = ctypes.CDataFinalizer(_pipebuf[0], _close);
+         array[1] = ctypes.CDataFinalizer(_pipebuf[1], _close);
+         return 0;
+       };

nit: I would prefer to do ret = _pipe...; if (ret == -1) return ret; ..... return ret. 

You can keep your dispose call, but please comment that you are relying on the finalizer calling close and dispose returning proper success/error code

Comment 59

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

Attachment: Unix synchronous back-end

Comment 60

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

Attachment: Companion makefile

Comment 61

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

Attachment: Companion makefile

Comment 62

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

Attachment: Companion makefile

Comment 63

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

Attachment: Companion testsuite

Comment 64

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

Attachment: Unix synchronous back-end

Fixed a discrepancy between Linux and BSD (behavior of |getwd| is slightly different).

Comment 65

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

Attachment: Non-Unix specific functions of the back-end

Fixed an issue with 32-bits vs. 64-bits.

Comment 66

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

Attachment: Non-Unix specific functions of the back-end

Comment 67

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

Attachment: Non-Unix specific functions of the back-end

Fixed a bug that caused off_t to have the wrong size on some configurations.

Comment 68

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

Attachment: Companion testsuite

Comment 69

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

Attachment: Companion makefile

Comment 70

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

Attachment: Unix synchronous back-end

Comment 71

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

Attachment: Companion makefile

Comment 72

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

Attachment: Companion makefile

Comment 73

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

Attachment: Companion testsuite

Comment 74

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

Attachment: Unix synchronous back-end

Comment 75

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

Attachment: Non-Unix specific functions of the back-end

Comment 76

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f7bb11da7be6
https://hg.mozilla.org/integration/mozilla-inbound/rev/378acab7d10c
https://hg.mozilla.org/integration/mozilla-inbound/rev/87174904a926
https://hg.mozilla.org/integration/mozilla-inbound/rev/43b7500f5954

Sorry for the delay...

Comment 77

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f7bb11da7be6
https://hg.mozilla.org/mozilla-central/rev/378acab7d10c
https://hg.mozilla.org/mozilla-central/rev/87174904a926
https://hg.mozilla.org/mozilla-central/rev/43b7500f5954