Closed Bug 1112640 Opened 5 years ago Closed 5 years ago

[AsyncShutdown] We should let code register blockers during shutdown

Categories

(Toolkit :: Async Tooling, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla38

People

(Reporter: Yoric, Assigned: Yoric)

References

Details

Attachments

(1 file, 2 obsolete files)

No description provided.
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.
Attached file MozReview Request: bz://1112640/Yoric (obsolete) —
Attachment #8538518 - Flags: review?(nfroyd)
/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
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...
Attachment #8538518 - Flags: review?(nfroyd)
Comment on attachment 8538518 [details]
MozReview Request: bz://1112640/Yoric

Updated on reviewboard.
Attachment #8538518 - Flags: review?(nfroyd)
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.
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. :)
Attachment #8538518 - Flags: review?(nfroyd)
(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?
Attachment #8538518 - Flags: review?(nfroyd)
/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
(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!
Or I had forgotten to mark you as reviewer for the second patch, I'm not sure.
/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
Attachment #8538518 - Flags: review?(nfroyd) → review+
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"?
(applied feedback, btw)
Empty patch? :)
Keywords: checkin-needed
Attached patch Folded patchSplinter Review
This time, the patch :)
Assignee: nobody → dteller
Attachment #8549001 - Attachment is obsolete: true
Attachment #8549919 - Flags: review+
https://hg.mozilla.org/integration/fx-team/rev/f24c203c43f3
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/f24c203c43f3
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → mozilla38
You need to log in before you can comment on or make changes to this bug.