The remote protocol says that actors reply to requests in the order the requests were received. This is what make pipelined requests work: the client can send many requests to a server, and store the handlers in a FIFO, to be paired up with replies. However, when we use promises (or any defer-to-later-event-tick mechanisms, really) in the server to handle requests, it's possible for the server to send replies to pipelined requests out of order. I believe this bug can occur when source maps are in use. However, DebuggerClient currently takes care never to pipeline requests, so until we pipeline, this is only a theoretical problem. I'm still a little foggy about event-driven programming, so let me spell everything out for my own sake. The following sequence is possible: - An actor receives a request. The response depends on the result of some I/O operation, so the actor creates a promise of the result, with a 'then' function that sends the reply. Control returns to the event loop. - The actor receives a second request, and things proceed as for the first: a second promise is created to send this request's reply, and the tick ends. - The I/O operations complete out of order, so the second request's promise is resolved first, and its reply is sent before the first request's reply. I'm sure this could be fixed in the server in a way that preserves all our opportunities for parallelism, but I haven't thought it through. Surely there is prior art.

We could fix this by moving the queuing of requests per actor from the client to DebuggerServerConnection.onPacket, right?

Yes; if this were an observable problem, that would probably be the best immediate fix. But that would mean that the server wouldn't even be able to start work on the second request until the first was done. What would be coolest is if we allowed the handlers to run async, but ordered the replies. Then, pipelined setBreakpoint packets could lead to pipelined source map requests.

Wouldn't something like this work?

(I didn't even run that)

(In reply to Dave Camp (:dcamp) from comment #4) > (I didn't even run that) I get the gist of that --- yes, that's a beautiful solution.

check it in!

This version I actually ran once, but I also pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=068b9ac4e141 This also includes something similar for protocol.js which handles its own packets.

This patch fixes my issues for the app manager. Is it ready for review?

Comment on attachment 786088 [details] [diff] [review] async-responses.diff It looks clean on try, so yes.

Comment on attachment 786088 [details] [diff] [review] async-responses.diff Review of attachment 786088 [details] [diff] [review]: ----------------------------------------------------------------- I love the tests. r=me with comments considered. ::: toolkit/devtools/server/main.js @@ +714,5 @@ > > this._actorPool = new ActorPool(this); > this._extraPools = []; > > + this._actorResponses = new Map; This totally needs a comment. It should explain what the keys to the map are, and what the contents mean. Since this is handling a somewhat obscure problem, a decent amount of detail would be good. @@ +957,5 @@ > + "error occurred while processing '" + aPacket.type, > + e); > + }); > + this._actorResponses.set(actor.actorID, response); > + return response; Why do we change DebuggerServerConnection.prototype.onPacket to return the response here? If that's not necessary, let's not. The indentation is funky... There's a lot of (pre-existing) promise plumbing here... is the following any better? Maybe not. let pendingResponse = this._actorResponses.get(actor.actorID) || resolve(null); let response = pendingResponse.then(() => { return resolve(ret).then(aResponse => { if (!aResponse.from) { aResponse.from = aPacket.to; } this.transport.send(aResponse); }) }) .then(null, (e) => { return this._unknownError( "error occurred while processing '" + aPacket.type, e); }); this._actorResponses.set(actor.actorID, response); ::: toolkit/devtools/server/tests/unit/test_protocol_async.js @@ +42,5 @@ > + let deferred = promise.defer(); > + dump("called promiseReturn: " + this.sequence + "\n"); > + let sequence = this.sequence++; > + // This should be enough to force a failure if the code is broken. > + do_timeout(150, () => { Since events are guaranteed to be processed in the order they're enqueued, wouldn't do_execute_soon work just as well here, and remove timing from the picture? To be sure, RootActor could have a member whose state simpleReturn changes and which promiseReturn checks in this callback; that would make the test's intent easier to follow, too. Similarly for the 'throw' cases. @@ +81,5 @@ > + > +function run_test() > +{ > + DebuggerServer.createRootActor = (conn => { > + return RootActor(conn); The 'new' isn't required here? (If not, can't we just say "DebuggerServer.createRootActor = RootActor"? Or is that obscure?) @@ +93,5 @@ > + client.connect((applicationType, traits) => { > + rootClient = RootFront(client); > + > + // Make sure a long-running async call returns before a later > + // short-running call I think this means to say, "Make sure we get replies in the same order that we sent their requests, even when earlier requests take several event ticks to complete." (The time they take doesn't determine the order; the requests do.) Also, this should go at the top of the file. @@ +99,5 @@ > + let sequence = 0; > + > + calls.push(rootClient.promiseReturn().then(ret => { > + do_check_eq(ret, sequence); > + do_check_eq(sequence++, 0); Can't we just drop the 'sequence' variable altogether and replace these two 'do_check_eq's in the tests with: do_check_eq(ret, 0 /* etc. */); Or does that loosen the test in some way I'm not noticing? @@ +118,5 @@ > + calls.push(rootClient.simpleThrow().then(() => { > + do_check_true(false, "simpleThrow shouldn't succeed!"); > + }, error => { > + do_check_true(true, "simple throw should throw"); > + return promise.resolve(null); Don't we guarantee the order of the errors, as well? I think we have to, or else the client can't pair up errors with their corresponding requests. @@ +144,5 @@ > + do_check_eq(ret, sequence); > + do_check_eq(sequence++, 5); > + })); > + > + promise.all.apply(null, calls).then(() => { This is very pretty.