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.

(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?

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 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?

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.

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 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.

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

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.

This simplifies nicely the code of OS.File itself.

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

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

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 on attachment 8444782 [details] [diff] [review] 4. Applying to Session Restore Review of attachment 8444782 [details] [diff] [review]: ----------------------------------------------------------------- See comments for previous patch.

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

Applied Tim's feedback.

Applied feedback.

Comment on attachment 8445133 [details] [diff] [review] 3. Applying to Thumbnails, v2 Review of attachment 8445133 [details] [diff] [review]: ----------------------------------------------------------------- That's better :)

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 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?

(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.

Applied feedback.

Propagated changes.

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

Propagated changes.

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

Fixed typo.

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

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

Applied feedback.

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

(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 ?

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

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

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