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 leak of tokens during development builds #232

Merged
merged 1 commit into from
Feb 25, 2021

Conversation

bendemboski
Copy link
Contributor

The logic for choosing between the Map and WeakMap for completed operation tracking wasn't right, and the result was that everything was being added to the Map, leaking every token. For waitForPromise() the token is the promise itself, meaning the resolved values were also leaking, which can easily add up to a memory leak of epic proportions.

@bendemboski
Copy link
Contributor Author

I guess waitFor() on a coroutine is also extremely leaky since the token is the generator function itself, so its context probably includes this, meaning it will leak the containing object.

@scalvert
Copy link
Collaborator

Woah, thanks for finding this, @bendemboski. I'll take a look.

@scalvert
Copy link
Collaborator

Derp, so we'd essentially reversed the logic for token storage. Nice catch!

@@ -89,7 +89,7 @@ class TestWaiterImpl<T extends object | Primitive = Token> implements TestWaiter
private _getCompletedOperations(token: T) {
let type = typeof token;

return token !== null || (type !== 'function' && type !== 'object')
return token === null || (type !== 'function' && type !== 'object')
Copy link
Member

Choose a reason for hiding this comment

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

Can you make this mistake harder to make?

I'm thinking something like:

Suggested change
return token === null || (type !== 'function' && type !== 'object')
let isFunction = type === 'function';
let isObject = token !== null && type === 'object';
return isFunction || isObject

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, how about

    let isFunction = type === 'function';
    let isObject = token !== null && type === 'object';
    let isPrimitive = !isFunction && !isObject;

    return isPrimitive ? this.completedOperationsForPrimitives : this.completedOperationsForTokens;

Copy link
Member

Choose a reason for hiding this comment

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

Ya, seems great to me!

Copy link
Collaborator

Choose a reason for hiding this comment

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

I like.

The logic for choosing between the Map and WeakMap for completed operation tracking wasn't right, and the result was that everything was being added to the Map, leaking every token. For waitForPromise() the token is the promise itself, meaning the resolved values were also leaking, which can easily add up to a memory leak of epic proportions.
@rwjblue rwjblue merged commit 6c9b5da into emberjs:master Feb 25, 2021
@rwjblue rwjblue added the bug Something isn't working label Feb 25, 2021
@rwjblue rwjblue changed the title Fix potentially massive memory leak Fix leak of tokens during development builds Feb 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants