Allow for garbage collection of cancellation handlers on long-lived tokens #52
Description
@stefanpenner raised this (I guess) in the July meeting but it was deferred.
I'll use the xhrAdapted and delay function from the docs. Let's assume the following code:
async function reload(el, url, token) {
await.cancelToken = token;
while (true) {
el.innerHTML = await xhrAdapted(url, token);
await delay(5000, token);
}
}
reload(document.getElementById("news"), "ticker.html", new CancelToken(…));With the current proposal, this code will leak memory like crazy. On every iteration of the loop, no less than 4 callbacks are registered on token.promise, and they are never garbage-collected until cancel is called (which might be never) or the token is collected itself. Two of the callbacks are closing over xhr and id respectively, keeping all of the created request and timer objects as well.
If the promisifications of XHR and timeout would set all closure variables to null explicitly after called resolve/reject, and doing nothing in the token callback when they are tested to be null, the memory could be theoretically released. However, this is a rather fragile approach, it's error-prone (easy to forget), and to collect the 4 callbacks when they are proven to do nothing any more would require heavy (and afaik yet unknown) engine sophistication.
For this to work out, we need a way unsubscribe callbacks, from tokens at least and possibly from promises in general. Let's not be too radical and keep it limited to the former.
Imo it is imperative that Promise. will unsubscribe the callback from the token (so that it no longer is called on a cancellation request) when the resulting promise is resolved. Whether that should possibly be extended until the promise is settled, not only cancellablewithCancelTokenresolve/reject have been called, remains to be discussed.
I am not sure to what extent the spec should cover this garbage collection. A very simple fix would be to insert a new step
If reject.[[AlreadyResolved]].[[value]] is
true, return.
between steps 2 and 3 in §1.3.1.1 Promise.withCancelToken Cancelation Reaction Functions. Then we'd leave it to the engines to figure out when callbacks are supposed to be collectable.
A better idea would be to explicitly remove the callback from the [[CancelTokenPromise]].[[PromiseFulfillReactions]] to make the intent clear. I would model that by adding the PromiseReaction that is registered on the cancellation token to the [[AlreadyResolved]] record of the promise resolving functions (via CreateResolvingFunctions), and then in those function after the [[Value]] test set not only the [[Value]] to false, but also the [[AlreadyResolved]].[[CancelationReaction]].[[Capability]] to empty and the [[AlreadyResolved]].[[CancelationReaction]].[[Handler]] to undefined.
Activity
domenic commentedon Sep 1, 2016
The important thing to remember is that browsers are free to make any of the sort of unobservable optimizations you suggest. They don't belong in the spec.
domenic commentedon Sep 1, 2016
Closing, but will certainly reopen if I misunderstood and the optimizations you suggest require observable semantic changes.
bergus commentedon Sep 1, 2016
Yes, please reopen. The additional step I suggested does change the observable behaviour to make the very important garbage collection possible in the first place.
While
example(1000, 2000)(where cancellation happens before resolution) will remain unaffected and log "It's over" after 1s, the behaviour ofwould change. When the token is cancelled after the promise is resolved, the cancel action is no more executed (because there usually is nothing to cancel any more). So instead of logging "It's done" and "It's over" after 1 and 2s respectively, it should only log "It's done".
domenic commentedon Sep 1, 2016
OK. I am going to rename this then since it sounds like your issue is not about memory leaks but instead about Promise.withCancelToken sometimes executing its cancelation action twice.
bergus commentedon Sep 1, 2016
No, the cancel action is not executed twice. It's just executed after all work is done, when there is nothing to cancel any more. And this really does create a memory leak - try my example and monitor the number of callbacks on
token.promise.domenic commentedon Sep 1, 2016
I see, you just used
"It's " + rin both.The number of callbacks on
token.promiseis a spec-internal device and does not govern how much memory will be consumed. Callbacks which the implementation knows will never fire are not going to be retained.I guess there may be a remaining semantic problem with
Promise.withCancelTokenin edge cases, potentially, but it sounds like there is a misunderstanding of how the memory model works here. I can investigate thePromise.withCancelTokenbehavior.bergus commentedon Sep 1, 2016
Yeah, one can only hope so. The problem is really that they will fire, they just don't do anything observable. Detecting that is much harder.
And no, this is not an edge case. The problem is that cancel actions (the functions installed on
token.promise) will be retained (and executed!) even after the action they could have cancelled is finished, as long as the token lives longer than the single promise - which is pretty much always.domenic commentedon Sep 1, 2016
It's not hard if it's written into the spec that they won't do anything observable.
bergus commentedon Sep 1, 2016
Sure, but writing the spec in that way makes it hard to see that they need to be garbage-collected. You didn't realise the importance of this problem immediately, or didn't even think about it in the first place, so it might be doubted that implementors will. Compare this to the (Weak)Map/(Weak)Set section of the spec which talks about the intended garbage collection behaviour extensively.
So if you choose to pick up my first suggested change you should probably add a Note to it that not executing the callback is done to make it garbage-collectable even before the token.
domenic commentedon Sep 1, 2016
Sure, we can try to add an note, although the committee has in the past preferred to not include such things, so my guess is that has a good chance of being removed after committee review.
21 remaining items