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 memory being retained until promise queue is completely empty #1544

Merged
merged 3 commits into from Sep 3, 2018

Conversation

Projects
None yet
3 participants
@Shingyx
Contributor

Shingyx commented Aug 30, 2018

Achieved by draining each queue element in a separate function.

Resolves #1529. The issue provides a way to easily reproduce this issue.

I'm currently looking into adding a test to assert this behaviour.

Fix memory leak on long promise chains
Achieved by draining each queue element in a separate function

@Shingyx Shingyx force-pushed the Shingyx:promise-chain-memory-leak branch from 15fbdb9 to 7bf1a93 Aug 31, 2018

@benjamingr benjamingr requested a review from petkaantonov Aug 31, 2018

@Shingyx Shingyx force-pushed the Shingyx:promise-chain-memory-leak branch 4 times, most recently from 32bb36d to 15dca69 Aug 31, 2018

@Shingyx

This comment has been minimized.

Contributor

Shingyx commented Aug 31, 2018

I've now added unit tests, based on the code snippet from the linked issue. It exposed a new issue when longStackTraces is enabled, where somehow the _trace is being retained after the promise is fulfilled and finishes scheduling the next promise in the queue.

I fixed this by dereferencing _trace after the promise is scheduled, to try minimise risk. Placing the dereference outside of the if (BIT_FIELD_READ(LENGTH_MASK) > 0) resulted in failing generator.js tests for some reason when running npm test, while the same tests passed when running node tools/test --run=generator.js...

@benjamingr

This comment has been minimized.

Collaborator

benjamingr commented Aug 31, 2018

This generally LGTM but I won't have time to look at it until I'm back from holiday. Good find!

cc @petkaantonov @spion

@Shingyx Shingyx force-pushed the Shingyx:promise-chain-memory-leak branch from 15dca69 to 90d1470 Aug 31, 2018

Use --expose-gc instead of v8 require
This is because Browserify fails when parsing require("v8")

@Shingyx Shingyx force-pushed the Shingyx:promise-chain-memory-leak branch from b9e9ea6 to bacdae3 Sep 1, 2018

Show resolved Hide resolved .travis.yml
@petkaantonov

This comment has been minimized.

Owner

petkaantonov commented Sep 2, 2018

Why would equal code only when called through function not cause memory leak?

@Shingyx

This comment has been minimized.

Contributor

Shingyx commented Sep 3, 2018

When I think about it, it's not technically a memory leak. It's more that the _drainQueue function didn't free the memory it allocated until the queue is completely drained. I'll update the title. I can update the commit messages too if you like.

I think the issue occurs because of how the garbage collector works, at least in V8. I think if memory is allocated inside the function _drainQueue, that memory will only be freed when _drainQueue completely finishes, i.e. when the queue is completely drained. By moving the memory allocation (i.e. queue.shift() and fn.call(receiver, arg)) to a separate function _drainQueueStep, then memory can be freed more often.

@Shingyx Shingyx changed the title from Fix memory leak on long promise chains to Fix memory being retained until promise queue is completely empty Sep 3, 2018

@petkaantonov petkaantonov merged commit f290da0 into petkaantonov:master Sep 3, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@Shingyx Shingyx deleted the Shingyx:promise-chain-memory-leak branch Sep 4, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment