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

Possible memory leak of proxies by holding strong references to revoke functions? #237

Open
ajvincent opened this issue Feb 24, 2021 · 2 comments

Comments

@ajvincent
Copy link
Owner

ajvincent commented Feb 24, 2021

The current ObjectGraphHandler code holds strong references to revoke() functions that Proxy.revocable() provides. I'm no longer sure that's a good idea. On the one hand, it definitely makes it easy to revoke an entire object graph in one action. On the other hand, that's potentially a lot of revoke functions to hold and if they internally hold strong references to their adjoining proxy, that's not great.

An alternative would be to adjust the ProxyHandler to watch a internal "I am dead" property, and store references from proxy to revoker in a WeakMap. Then, at the start of the ProxyHandler's traps, if the handler considers itself dead, it can immediately revoke the proxy, and reflect back a static revoked proxy's matching trap, throwing the exception that would normally throw for a revoked proxy.

@erights, am I right here to worry about revoke functions being a memory leak for a membrane?

const revokers = [];
{
  const {revoke} = Proxy.revocable({}, Reflect);
  revokers.push(revoke); // does this mean the proxy leaks?
}

https://262.ecma-international.org/11.0/#sec-proxy.revocable

@ajvincent
Copy link
Owner Author

A real-world membrane isn't supposed to hold strong references to the proxies. So while the above example initially seems contrived, there's the argument that if the revoker is the only thing left after other references have been dropped, then the proxy might remain alive. That's my question.

@ajvincent
Copy link
Owner Author

In the esmodules-0.10 branch, I have forcibly moved revokers into a "revocable multimap" (source/core/RevocableMultimap.mjs), and that is the only place they're kept. This means the memory leak is strictly the responsibility of the underlying engines, although it's worth auditing to see if custom revokers (specifically, those I create not from Proxy.revocable()) hold strong references.

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

No branches or pull requests

1 participant