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

AsyncHook MemoryLeak fixes #60

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

alexgarbarev
Copy link

It looks like we have to disable hook, when namespace is destoryed. Otherwise it would live forever and hence it would retain namespace object with all it's massive _context.

More details how I replicated and fixed the issue described here: liberation-data/drivine#35 (comment)

* in case our namespace is retained mistakenly, so
* we releasing heavist part at least
*/
namespace._contexts = null;
Copy link

Choose a reason for hiding this comment

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

Couple of typos, and this shouldn't be JSDoc block. Maybe:

/*
 * Zeroing _contexts as heaviest part of Namespace
 * In case our namespace is retained mistakenly, so
 * at least we are releasing heaviest part
 */

Copy link
Author

Choose a reason for hiding this comment

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

Yeah, I agree with these changes. Thanks for suggestions.

@orloffv
Copy link

orloffv commented Nov 12, 2020

@Jeff-Lewis ping(

@diestrin
Copy link

Any updates on this @Jeff-Lewis ?

@SimonX200
Copy link

It looks like we have to disable hook, when namespace is destoryed. Otherwise it would live forever and hence it would retain namespace object with all it's massive _context.

You haven't yet noticed that cls-hooked has a major memory leak?

See:

#63

And btw: a performance problem as well?

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.

5 participants