1112640 - [AsyncShutdown] We should let code register blockers during shutdown

Status: RESOLVED FIXED

(Toolkit :: Async Tooling, defect)

Comment 1

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

At the moment, I have difficulties with bug 1043863 because we sometimes add the blockers in code triggered by `wait()`. The call to `addBlocker` is not very useful in these specific codepaths, but it shouldn't hurt either.

In other words, we should be able to call `addBlocker` at any moment, as long as `wait()` has not reached the state at which it stops waiting.

Comment 2

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

Attachment: MozReview Request: bz://1112640/Yoric

Comment 3

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

/r/1593 - Bug 1112640 - In an AsyncShutdown Barrier, addBlocker may now be called while we are already waiting for completion of the barrier;r=froydnj

Pull down this commit:

hg pull review -r cee68a5edc1c167ed5c6d1593f6d82423bcc5e4c

Comment 4

[Nathan Froyd [:froydnj]]

https://reviewboard.mozilla.org/r/1591/#review1007

::: toolkit/components/asyncshutdown/AsyncShutdown.jsm
(Diff revision 1)
> +    this._ensurePromise(key);

Why does it matter that this is a promise? AFAICT, you only use key as a key in the Map, never actually using it as a promise.  Am I missing something, or are you missing code?

::: toolkit/components/asyncshutdown/AsyncShutdown.jsm
(Diff revision 1)
> + * resolve once all Promise items have been resolved or removed.

Just to check my understanding here, PromiseSet is essentially Promise.all, except that we want the ability to take and/or insert items in the resolve chain while resolution is ongoing?  After some staring, I thought that you could almost implement wait() as:

promise = Promise.all(this._indirections.keys());

but that's not quite right, I think.  Is my understanding correct?

::: toolkit/components/asyncshutdown/AsyncShutdown.jsm
(Diff revision 1)
> -      let set = this._conditions.get(condition);
> +      // Split the condition between a trigger function and a promise.

I think you could profitably split out everything between here and the definition of |blocker| into a separate function, returning [trigger, promise].

Actually, wait, it is supremely weird that trigger sometimes isn't defined until the promise is actually resolved...is that right?

::: toolkit/components/asyncshutdown/AsyncShutdown.jsm
(Diff revision 1)
> +  this._indirections = new Map();

It is a little strange for the documentation to describe a set, and the lone member here be a Map.  Maybe a comment is warranted on what the key/value scheme is?

::: toolkit/components/asyncshutdown/AsyncShutdown.jsm
(Diff revision 1)
> +   * Key: Promise (as present in this._waitForMe or this._conditionToPromise).

The toplevel comment says "as present in `this._waitForMe`...but this comment adds an additional qualifier.  Which one is it?

I think this all makes sense...

Comment 5

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

Comment on attachment 8538518 [details]
MozReview Request: bz://1112640/Yoric

Updated on reviewboard.

Comment 6

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

https://reviewboard.mozilla.org/r/1591/#review1013

> Just to check my understanding here, PromiseSet is essentially Promise.all, except that we want the ability to take and/or insert items in the resolve chain while resolution is ongoing?  After some staring, I thought that you could almost implement wait() as:
> 
> promise = Promise.all(this._indirections.keys());
> 
> but that's not quite right, I think.  Is my understanding correct?

Indeed, it's Promise.all except any Promise added/removed is taken into account in the final resolution.

And yes, your solution wouldn't take into account Promise being added/removed.

> I think you could profitably split out everything between here and the definition of |blocker| into a separate function, returning [trigger, promise].
> 
> Actually, wait, it is supremely weird that trigger sometimes isn't defined until the promise is actually resolved...is that right?

> I think you could profitably split out everything between here and the definition of |blocker| into a separate function, returning [trigger, promise].

I just tried, and it look like this makes the code actually somewhat harder to read, so if you don't mind, I won't pursue in this direction.

> Actually, wait, it is supremely weird that trigger sometimes isn't defined until the promise is actually resolved...is that right?

That's not the resolution callback, but the constructor callback, which is guaranteed to be called immediately. One of the oddities of the Promise API, but `trigger` is always defined by the end of the `if () {} else {}` block.

> Why does it matter that this is a promise? AFAICT, you only use key as a key in the Map, never actually using it as a promise.  Am I missing something, or are you missing code?

Thanks for spotting this, I had failed to add cleanup code once the promise is resolved.

Comment 7

[Nathan Froyd [:froydnj]]

https://reviewboard.mozilla.org/r/1593/#review1329

::: toolkit/components/asyncshutdown/AsyncShutdown.jsm
(Diff revision 1)
> +   * Note that calling `wait()` calls Promise to be removed from the

This comment reads oddly to me...perhaps you meant to say "Note that calling wait() *causes* Promise(s) to be removed from the Set..."?

::: toolkit/components/asyncshutdown/AsyncShutdown.jsm
(Diff revision 1)
> +    this._ensurePromise(key);

I guess this is a primitive form of type-checking, but I don't see anything relating to the cleanup code once the promise is resolved that you suggested this new patch might be adding.  (I'm looking--or attempting to look--at both revisions of the patch on ReviewBoard, but I might have gotten the URLs for each review wrong...sure would be nice to have interdiffs!)

Holding off on r+ because I don't understand whether the problems in PromiseSet are my incompetence with reviewboard, or you uploading the wrong patches. :)

Comment 8

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

Actually, we do have interdiffs: https://reviewboard.mozilla.org/r/1591/diff/1-2/

Comment 9

[Nathan Froyd [:froydnj]]

(In reply to David Rajchenbach-Teller [:Yoric] (hard to reach until January 10th - use "needinfo") from comment #8)
> Actually, we do have interdiffs:
> https://reviewboard.mozilla.org/r/1591/diff/1-2/

That page gives me a 404?

Comment 10

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

/r/1593 - Bug 1112640 - In an AsyncShutdown Barrier, addBlocker may now be called while we are already waiting for completion of the barrier;r=froydnj
/r/2105 - Bug 1112640 - In an AsyncShutdown Barrier, addBlocker may now be called while we are already waiting for completion of the barrier;r=froydnj

Pull down these commits:

hg pull review -r 581562302bcc825df2e411e79c0bdc86d0126441

Comment 11

[Nathan Froyd [:froydnj]]

(In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #9)
> (In reply to David Rajchenbach-Teller [:Yoric] (hard to reach until January
> 10th - use "needinfo") from comment #8)
> > Actually, we do have interdiffs:
> > https://reviewboard.mozilla.org/r/1591/diff/1-2/
> 
> That page gives me a 404?

Apparently I was clicking too fast, or something.  It works now!

Comment 12

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

Or I had forgotten to mark you as reviewer for the second patch, I'm not sure.

Comment 13

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

/r/1593 - Bug 1112640 - In an AsyncShutdown Barrier, addBlocker may now be called while we are already waiting for completion of the barrier;r=froydnj
/r/2105 - Bug 1112640 - In an AsyncShutdown Barrier, addBlocker may now be called while we are already waiting for completion of the barrier;r=froydnj
/r/2117 - Bug 1112640 - In an AsyncShutdown Barrier, addBlocker may now be called while we are already waiting for completion of the barrier;r=froydnj

Pull down these commits:

hg pull review -r f053796c73076bd1e8a0518532936b7ab0fa4397

Comment 14

[Nathan Froyd [:froydnj]]

https://reviewboard.mozilla.org/r/1591/#review1327

Minor edits to the comments, and I think this is good.

::: toolkit/components/asyncshutdown/AsyncShutdown.jsm
(Diff revision 1)
> +   * Note that calling `wait()` calls Promise to be removed from the

This bit of the comment doesn't read right to me...maybe you meant "Note that calling `wait()` *causes* Promise to be removed from the Set..."?

::: toolkit/components/asyncshutdown/AsyncShutdown.jsm
(Diff revisions 1 - 2)
> -          // clients until we find one that is guilty and use its
> +          // clients that are still blocking and use its filename/lineNumber,

"search through all clients that are still blocking and use its filename/lineNumber" is...not clear.  Can you say something like "we choose a still-blocked client and use its filename/lineNumber as the abort location"?

Comment 15

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

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

Comment 16

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

Attachment: Folded patch

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=192554797c97

Comment 17

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

(applied feedback, btw)

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Empty patch? :)

Comment 19

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

Attachment: Folded patch

This time, the patch :)

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/f24c203c43f3

Comment 21

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

https://hg.mozilla.org/mozilla-central/rev/f24c203c43f3