707909 - [OS.File] serialize files

Status: RESOLVED INVALID

(Toolkit :: OS.File, enhancement)

Description

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

Sending OS.File, OS.Dir objects to a worker would be very useful. We should implement this.

It is probably quite easy to do the following:
- add a factory OS.File.ofHandle;
- add a custom toSource() for OS.File, OS.Dir that returns respectively "new OS.File.ofHandle(...)" and "new OS.Dir(...)".

Is this sufficient?

Comment 1

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

After experimenting, we need something a little different. Files are opened in a I/O worker thread, then sent to the main thread. Garbage-collection, however, should apply only to the main thread.

So, we need the following components:
- in the worker-only implementation of OS.File, an operation |OS.File.prototype.forget| that ensures that garbage-collection does not close the file;
- in the worker-only implementation of OS.File, an operation |OS.File.toMsg| that returns the file descriptor in a form that can be sent to the main thread;
- in the worker-only implementation of OS.File, a type |OS.Shared.Type.file_handle| that can be used to deserialize this descriptor;
- main thread stuff (in another bug).

Same stuff for directory iterators.

Comment 2

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

Re-scoping: let's handle files for the moment, directories later.

Comment 3

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

Attachment: Support code for (de)serializing files

This patch implements supports for serialization of files.
Deserialization support will be distinct between main thread and worker threads, due to garbage-collection issues.

Comment 4

[Nathan Froyd [:froydnj]]

Comment on attachment 649557 [details] [diff] [review]
Support code for (de)serializing files

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

Clearing r? due to wrong patch.

Comment 5

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

Attachment: Support code for (de)serializing files

Oops

Comment 6

[Nathan Froyd [:froydnj]]

Comment on attachment 649668 [details] [diff] [review]
Support code for (de)serializing files

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

I don't see a definition of fromMsg anywhere, which is what I would expect to see _setHandle used for.  Am I missing something, or is the patch incomplete?

Comment 7

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

The definitions of fromMsg (one for the controller, one for the worker) are part of bug 777711 and I have not managed to detach them cleanly.

Indeed, the worker code uses |_setHandle|. The controller code... is a little more imaginative :)

Comment 8

[Nathan Froyd [:froydnj]]

Comment on attachment 649668 [details] [diff] [review]
Support code for (de)serializing files

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

This is a little tricky to review since it's at best half of the story (no deserialization, no tests).  Let me know if I'm wildly off base with my suggestions and if I am, I'll try to take a better look at the other bits in bug 777711.

::: toolkit/components/osfile/osfile_shared_front.jsm
@@ +107,5 @@
> +  _setHandle: function _setHandle(fd) {
> +    if (fd == null) {
> +      throw new TypeError("Expecting a non-null file handle");
> +    }
> +    this._fd = fd;

Seems like we'd want to check that this._fd is null here or try to clean up an existing _fd somehow; we'd leak in the latter case.

@@ +119,5 @@
> +  forget: function forget() {
> +    if (!this._fd) {
> +      throw new TypeError("Cannot call |forget| on a closed File");
> +    }
> +    this._fd.forget();

Seems like you'd want to return this value and/or null out this._fd.

@@ +128,5 @@
> + * Convert an instance of |File| into something that can be serialized
> + * between threads (not necessarily a string).
> + *
> + * Note: If necessary, do not forget to call
> + * |File.prototype.forget| to ensure that your file descriptor

.prototype makes my head hurt sometimes; don't you mean "...do not forget to call |forget| to ensure..."?

@@ +138,5 @@
> +    return null;
> +  }
> +  // |fd| may or may not be a CDataFinalizer, get back to the C type
> +  let as_cval = exports.OS.Shared.Type.fd.implementation(fd);
> +  let as_msg = exports.OS.Shared.Type.fd.toMsg(as_cval);

It surprises me that with all the emphasis on types that you have, we're simply relying on the serialization of as_cval here, rather than packaging it up into an { fd: ... } object.

@@ +139,5 @@
> +  }
> +  // |fd| may or may not be a CDataFinalizer, get back to the C type
> +  let as_cval = exports.OS.Shared.Type.fd.implementation(fd);
> +  let as_msg = exports.OS.Shared.Type.fd.toMsg(as_cval);
> +  return as_msg;

Nit: just dispense with as_msg here.

Comment 9

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

FYI, Taras wants me to change a big piece of the design. There are chances that this will make this bug useless.

Comment 10

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

Indeed, this has become useless atm.