Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: workaround leak through promise resolvers #1222

Merged
merged 7 commits into from
Jun 25, 2022

Conversation

mhofman
Copy link
Contributor

@mhofman mhofman commented Jun 22, 2022

The investigation into Agoric/agoric-sdk#5507 lead us into discovering a leak caused by promises, in particular when using Promise.race with a long pending promise. The root cause really seem that the promise resolve functions hold onto the promise strongly (instead of just an internal record representing the reactions registered on the promise), and through that, the result of said promise. Promise.race is implemented as adding the result's promise's resolver functions to each of the contenders reactions, causing the long pending promise to keep the raced result alive.

This PR re-implements Promise.race based on the solution in nodejs/node#17469 (comment). It also updates makePromiseKit to return resolvers that will sever their hold on the original resolvers (and thus the promise) once called. That latter approach, while simpler, is not sufficient for Promise.race as reactions would keep queueing on the long pending promise (engines are not sufficiently smart to realize a reaction is a native resolver that has become inert).

Finally it installs the new race implementation as a Promise.race vetted shim in @endo/init.

(I'm half considering re-writing promises in userland with proper support for short-circuiting, which would avoid most of these issues, but the few spec places creating primordial promises would damper that. A long term solution might be to rewrite the spec with proper short-circuiting, maybe as part of tc39/proposal-faster-promise-adoption)

Copy link
Member

@kriskowal kriskowal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could alternately move this into ses as a taming of the existing Promise.race. It would only be a slight feature creep.

packages/promise-kit/index.js Outdated Show resolved Hide resolved
@michaelfig
Copy link
Member

We could alternately move this into ses as a taming of the existing Promise.race. It would only be a slight feature creep.

Why not just as a separate "vetted shim" package, loaded by @endo/init?

@kriskowal
Copy link
Member

We could alternately move this into ses as a taming of the existing Promise.race. It would only be a slight feature creep.

Why not just as a separate "vetted shim" package, loaded by @endo/init?

That’s even better.

@mhofman
Copy link
Contributor Author

mhofman commented Jun 23, 2022

Updated to use a vetted shim approach, and reverted the explicit uses to rely on the shim.

Also updated the implementation to follow the spec more closely. It'd be nice to test against test262, but I don't believe we have the infra for that.

@mhofman mhofman changed the title fix(promise-kit): workaround leak through promise resolvers fix: workaround leak through promise resolvers Jun 24, 2022
packages/promise-kit/index.js Outdated Show resolved Hide resolved
packages/promise-kit/memo-race.js Outdated Show resolved Hide resolved
Copy link
Contributor

@erights erights left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not done yet. Just giving you bits as they occur to minimize latency.

packages/promise-kit/index.js Show resolved Hide resolved
@mhofman mhofman requested a review from erights June 24, 2022 20:36
Copy link
Contributor

@erights erights left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did not do a thorough review of memo-race.js . But nothing seemed wrong, and you already have @kriskowal 's approval. I'll defer to that. Please proceed.

packages/promise-kit/src/memo-race.js Outdated Show resolved Hide resolved
@mhofman
Copy link
Contributor Author

mhofman commented Jun 25, 2022

Added a blurb to NEWS.md and addressed @erights type feedback in 0377da0 then rebase the whole thing cleaning up the history: 0377da0..219e49d

@mhofman mhofman enabled auto-merge June 25, 2022 01:04
@mhofman mhofman merged commit 149492d into master Jun 25, 2022
@mhofman mhofman deleted the mhofman/fix-promise-leak branch June 25, 2022 01:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants