801598 - Extract the communication mechanisms of OMT OS.File into its own module (PromiseWorker)

Status: RESOLVED FIXED

(Toolkit :: Async Tooling, defect)

Description

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

Attachment: Example usage of this spawned version of OS.File

Experience with porting the Thumbnails storage to OS.File seems to indicate that OS.File is used in two fashions for the same module:
- in a custom thread (bug 794804), along with client-specific code (e.g. to remove all the files of a given directory that are older than some date, except files that appear in some list) - bug ; and, simultaneously
- using the shared OS.File worker (bug 753768), to use the regular OS.File operations and the infrastructure of message passing and promises.

Using both threads in the same module is rather fragile, as it introduces race conditions, plus it requires reimplementing some of the message-passing code of async OS.File. At the same time, we probably do not want to let any client add random code to the async OS.File thread, as this could cause impossible-to-debug slowdowns/lockups,

What we could do is let clients spawn instances of async OS.File extended which their own code.

Comment 1

[Nathan Froyd [:froydnj]]

(In reply to David Rajchenbach Teller [:Yoric] from comment #0)
> Using both threads in the same module is rather fragile, as it introduces
> race conditions, plus it requires reimplementing some of the message-passing
> code of async OS.File. At the same time, we probably do not want to let any
> client add random code to the async OS.File thread, as this could cause
> impossible-to-debug slowdowns/lockups,

This paragraph confuses me, particularly the first sentence--it sounds like you're saying using OS.File from multiple threads is fragile, which I hope is not the case!  And we have APIs to let client code run on the OS.File thread?  That sounds insane.

Could you please explain this in more detail?

Comment 2

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

Let me rephrase.

I mean that:
- using two distinct threads to touch the same files is fragile;
- the official async OS.File thread offers all the necessary architecture to pass instructions and results/exceptions back and forth (which is good), but none of the mechanism to execute client code (which seems reasonable);
- the thread spawned for the PageThumbsStorage needs to reimplement some of this architecture for instructions/results/exceptions (which is error-prone), but can execute client code (which is the only reason to spawn it in the first place).

If we introduce the ability to launch a thread that contains both the messaging architecture and the ability to run client code, we will be able to get rid of these potential race conditions. Which looks like the best way to implement e.g. bug 753768.

Comment 3

[Nathan Froyd [:froydnj]]

Comment on attachment 671397 [details]
Example usage of this spawned version of OS.File

I'm not a fan.  I know that it's better to move your code to where the data is, rather than serializing the data and sending it over some channel, but that sort of action/executor architecture is not what we designed OS.File to do.  I mean, if we're going down this path, why didn't we make everything a sync api that you must execute on a separate worker thread?

Did we investigate to see if there's anything that can make the message-passing faster in the first place?

Comment 4

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

Well, as you may recall, we do have a sync api that can be executed on a separate worker thread (see WIP doc at https://developer.mozilla.org/en-US/docs/JavaScript_OS.File ).

In the case of bug 794804, we have a possibly very large directory, and walking it asynchronously + collecting required data requires 2 to 4 complex messages per entry. If the directory contains thousands of entries, this is the kind of thing that will have a very clear impact. By contrast, doing everything on its own thread reduces the number of messages to ~2.

I am pretty sure that the same kind of issues can creep in with Sync, with cache-related bugs, and with Adblock – I actually had a discussion with Wladimir Palent in which he mentioned how communication costs basically killed his attempt to put code on a worker.

Comment 5

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

After discussing this with froydnj, we concluded that there is another, simpler method to achieve the same purpose: we just need to refactor OS.File to make the communication mechanism a public API. Rescoping this bug.

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 671397 [details]
Example usage of this spawned version of OS.File

Removing feedback? as the bug's scope changed since the request and will require a new API.

Comment 7

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

Not working actively on this. If someone is interested, please feel free to grab this bug.

Comment 8

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

Attachment: 1. Exposing communications mechanism into its own API

Since bug 1024382 and the differential Updates mechanism for SessionWorker are going to cause lots of refactoring on the SessionWorker, I figured it's the right time to bring the error reporting of SessionWorker up to level with that of OS.File – and for this, to start sharing the code instead of using a copy & paste of an old version of OS.File.

Comment 9

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

Attachment: 2. Applying to OS.File itself

This simplifies nicely the code of OS.File itself.

Comment 10

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

Attachment: 3. Applying to Thumbnails

Comment 11

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

Attachment: 4. Applying to Session Restore

Comment 12

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

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

Comment 13

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

I have double-checked – all the oranges above are due to bug 1016387, which I had in my mq for sanity checking.

Comment 14

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8444780 [details] [diff] [review]
3. Applying to Thumbnails

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

::: toolkit/components/thumbnails/PageThumbs.jsm
@@ +792,5 @@
>  /**
>   * Interface to a dedicated thread handling I/O
>   */
> +let PageThumbsWorker = {
> +  __proto__: new AbstractPromiseWorker("resource://gre/modules/PageThumbsWorker.js"),

__proto__ is deprecated. Better:

let aw = new AbstractPromiseWorker("resource://gre/modules/PageThumbsWorker.js");
let PageThumbsWorker = Object.create(aw, {
  log: {
    value: function () {
      // No logging supported.
    }
  },
});

(I *think* I got that right.)

let PageThumbsWorker = Object.create(aw);
PageThumbsWorker.log = function () {
  // No logging supported.
};

That works too of course. I think I like that more.

@@ +795,5 @@
> +let PageThumbsWorker = {
> +  __proto__: new AbstractPromiseWorker("resource://gre/modules/PageThumbsWorker.js"),
> +  log: function() {
> +    // No logging supported
> +  }

Can this be provided by the AbstractPromiseWorker? As a no-op?

::: toolkit/components/thumbnails/PageThumbsWorker.js
@@ +18,5 @@
>  let File = OS.File;
>  let Type = OS.Shared.Type;
>  
> +let worker = {
> +  __proto__: new PromiseWorker.AbstractWorker(),

Same here about __proto__ being deprecated.

@@ +24,5 @@
> +    return Agent[method](...args);
> +  },
> +  log: function(...args) {
> +    // No logging
> +  },

Can this be provided by the abstract worker?

@@ +27,5 @@
> +    // No logging
> +  },
> +  postMessage: function(message, ...transfers) {
> +    self.postMessage(message, ...transfers);
> +  },

Can't this too be provided by the abstract worker?

@@ +34,1 @@
>    }

Same here?

Comment 15

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8444782 [details] [diff] [review]
4. Applying to Session Restore

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

See comments for previous patch.

Comment 16

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

Attachment: 1a Cumulative diff

Small addition to 1, as per ttaubert's feedback.

Comment 17

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

Attachment: 2. Applying to OS.File itself, v2

Applied Tim's feedback.

Comment 18

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

Attachment: 3. Applying to Thumbnails, v2

Applied feedback.

Comment 19

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

Attachment: 4. Applying to Session Restore, v2

Comment 20

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8445133 [details] [diff] [review]
3. Applying to Thumbnails, v2

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

That's better :)

Comment 21

[Nathan Froyd [:froydnj]]

Comment on attachment 8444775 [details] [diff] [review]
1. Exposing communications mechanism into its own API

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

Either I'm confused about how |options| works, or we don't have tests for things.

I'd really like to figure out how to not handle OS.File errors in the base PromiseWorker...can we add another function to the required API?

::: toolkit/components/promiseworker/PromiseWorker.jsm
@@ +16,5 @@
>   */
>  
>  "use strict";
>  
> +this.EXPORTED_SYMBOLS = ["AbstractPromiseWorker"];

I think you need to make the naming between this patch and later patches for this class consistent.  (BasePromiseWorker seems to be what later parts want it to be.)

@@ +225,5 @@
>     */
> +  post: function(fun, args, options, closure) {
> +    return Task.spawn(function* postMessage() {
> +      let id = ++this._id;
> +      let message = {fun: fun, args: args, id: id};

It looks like we're not sending options to the worker here?  The worker is still expecting |options|, AFAICS.

@@ +276,5 @@
> +      if (typeof options !== "object" ||
> +        !("outExecutionDuration" in options)) {
> +        return reply.ok;
> +      }
> +      // If data.durationMs is not present, return data.ok (there was an

s/data/reply/g, I think.

::: toolkit/components/promiseworker/moz.build
@@ +6,5 @@
> +
> +PARALLEL_DIRS += [
> +    'worker'
> +]
> +    

Nit: whitespace.

::: toolkit/components/promiseworker/worker/PromiseWorker.js
@@ +20,5 @@
> +if (typeof Components != "undefined") {
> +  throw new Error("This module is meant to be used from the worker thread");
> +}
> +if (typeof require == "undefined" || typeof module == "undefined") {
> +  throw new Error("this module is meant to be imported using the implementation fo require() at resource://gre/modules/workers/require.js");

Typo: "implementation of require"

@@ +142,5 @@
> +      } else {
> +        this.postMessage({ok: result, id:id, durationMs: durationMs});
> +      }
> +    } else if (exn instanceof SharedAll.OSError) {
> +      this.log("Sending back OS.File error", exn, "id is", id);

This seems a little weird to have this special-cased for OS.File in the base class.

@@ +167,5 @@
> +      this.postMessage({fail: error, id: id, durationMs: durationMs});
> +    } else {
> +      // Other exceptions do not, and should be propagated through DOM's
> +      // built-in mechanism for uncaught errors, although this mechanism
> +      // may lose interesting information.

Can you provide comments for the EXCEPTION_NAMES and StopIteration cases; this comment makes it sounds like we have just inserted special handling for those cases on a whim.

::: toolkit/components/promiseworker/worker/moz.build
@@ +4,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/.
> +
> +JS_MODULES_PATH = 'modules/workers'
> +    

Nit: whitespace.

Comment 22

[Nathan Froyd [:froydnj]]

Comment on attachment 8445131 [details] [diff] [review]
2. Applying to OS.File itself, v2

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

::: toolkit/components/osfile/modules/osfile_async_front.jsm
@@ +379,5 @@
>     * must be clonable.
>     * @return {Promise} A promise conveying the result/error caused by
>     * calling |method| with arguments |args|.
>     */
> +  post: function post(method, args = undefined, closure = undefined) {

This calling convention change ought to require a lot more updates somewhere?

Comment 23

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

(In reply to Nathan Froyd (:froydnj) from comment #21)
> I'd really like to figure out how to not handle OS.File errors in the base
> PromiseWorker...can we add another function to the required API?

I'll see what I can do.

> I think you need to make the naming between this patch and later patches for
> this class consistent.  (BasePromiseWorker seems to be what later parts want
> it to be.)

Ah, there seems to be something wrong with my qref, because on my computer, it's BasePromiseWorker.

> It looks like we're not sending options to the worker here?  The worker is
> still expecting |options|, AFAICS.

Well, these are options for the sending. If options need to be sent to the worker, the caller is in charge of putting them in `args` (which is the case in OS.File).

> This seems a little weird to have this special-cased for OS.File in the base
> class.

Right. I'll see what I can do.

(In reply to Nathan Froyd (:froydnj) from comment #22)
> ::: toolkit/components/osfile/modules/osfile_async_front.jsm
> @@ +379,5 @@
> >     * must be clonable.
> >     * @return {Promise} A promise conveying the result/error caused by
> >     * calling |method| with arguments |args|.
> >     */
> > +  post: function post(method, args = undefined, closure = undefined) {
> 
> This calling convention change ought to require a lot more updates somewhere?

Actually, no, it's just a cleanup, but it doesn't change the calling convention.

Comment 24

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

Attachment: 1. Exposing communications mechanism into its own API, v2

Applied feedback.

Comment 25

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

Attachment: 2. Applying to OS.File itself, v3

Propagated changes.

Comment 26

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

Attachment: 1. Exposing communications mechanism into its own API, v3

Clarified comments. Reverted to old mechanism for passing `options`.

Comment 27

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

Attachment: 2. Applying to OS.File itself, v4

Propagated changes.

Comment 28

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

Attachment: 4. Applying to Session Restore, v3

Fixed typo. Tim, it's not clear to me whether you wanted to re-review this patch.

Comment 29

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

Attachment: 3. Applying to Thumbnails, v3

Fixed typo.

Comment 30

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

Try: https://tbpl.mozilla.org/?tree=Try&rev=3ef06cd31eac

Comment 31

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8447678 [details] [diff] [review]
4. Applying to Session Restore, v3

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

::: browser/components/sessionstore/src/SessionFile.jsm
@@ +269,5 @@
>        // write instructions.
>        isFinalWrite = this._isClosed = true;
>      }
>  
> +    let deferWritten = Promise.defer();

Nit: maybe |deferredWritten| as it returns a Deferred object?

@@ +276,5 @@
>  
> +      // Ensure that we can write sessionstore.js cleanly before the profile
> +      // becomes unaccessible.
> +      AsyncShutdown.profileBeforeChange.addBlocker(
> +        "SessionFile: Finish writing the sessionstore.js",

Nit: either "writing sessionstore.js" or "the sessionstore.js file" :)

::: browser/components/sessionstore/src/SessionWorker.jsm
@@ +18,5 @@
>  
>  this.EXPORTED_SYMBOLS = ["SessionWorker"];
>  
> +this.SessionWorker = new BasePromiseWorker("resource:///modules/sessionstore/SessionWorker.js");
> +// As the PageThumbsWorker performs I/O, we can receive instances of

s/PageThumbsWorker/SessionWorker

Comment 32

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

Attachment: 4. Applying to Session Restore, v4

Applied feedback.

Comment 33

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

Try: https://tbpl.mozilla.org/?tree=Try&rev=9c934b0706af

Comment 34

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/781fc1eb104a

Comment 35

[Carsten Book [:Tomcat]]

(In reply to Carsten Book [:Tomcat] from comment #34)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/781fc1eb104a

Hey Yoric, had to back this out for https://tbpl.mozilla.org/php/getParsedLog.php?id=42917674&tree=Mozilla-Inbound - is this a problem of this checkin or because i failed and only checkedin this one csets where the others had to land too ?

Comment 36

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

All the csets need to land together. Otherwise, several modules will fail to communicate with their threads, which would be a shame.

Comment 37

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cef11c929fbe
https://hg.mozilla.org/integration/mozilla-inbound/rev/f733737ec4c7
https://hg.mozilla.org/integration/mozilla-inbound/rev/3fd7bfce65ee
https://hg.mozilla.org/integration/mozilla-inbound/rev/2a4264bab28d

Comment 38

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/cef11c929fbe
https://hg.mozilla.org/mozilla-central/rev/f733737ec4c7
https://hg.mozilla.org/mozilla-central/rev/3fd7bfce65ee
https://hg.mozilla.org/mozilla-central/rev/2a4264bab28d