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

remove extra closure #3446

Merged
merged 1 commit into from Feb 28, 2017
Merged

remove extra closure #3446

merged 1 commit into from Feb 28, 2017

Conversation

@cjihrig
Copy link
Contributor

@cjihrig cjihrig commented Feb 28, 2017

Closes outmoded/discuss#433

This commit removes an extra closure which captures circular
memory references.
@Marsup
Copy link
Contributor

@Marsup Marsup commented Feb 28, 2017

Care to elaborate why this would fix the situation ? This seems equivalent.

@cjihrig
Copy link
Contributor Author

@cjihrig cjihrig commented Feb 28, 2017

The error handler was capturing the things that were reported as leaking in outmoded/discuss#433. By removing the arrow function, the closure goes away, and the memory associated with those things can be released.

@Marsup
Copy link
Contributor

@Marsup Marsup commented Feb 28, 2017

JS is so stupid sometimes... Good catch !

@hueniverse hueniverse self-assigned this Feb 28, 2017
@hueniverse hueniverse added the bug label Feb 28, 2017
@hueniverse hueniverse added this to the 16.1.1 milestone Feb 28, 2017
@hueniverse hueniverse merged commit 513cea8 into hapijs:master Feb 28, 2017
1 check passed
@AdriVanHoudt
Copy link
Contributor

@AdriVanHoudt AdriVanHoudt commented Mar 1, 2017

So this was an actual leak?

@sjuchno
Copy link

@sjuchno sjuchno commented Mar 1, 2017

It only affects you if you are creating and removing many servers. If you are creating a new server per test then those were never released from memory. There is no issue in production.

@AdriVanHoudt
Copy link
Contributor

@AdriVanHoudt AdriVanHoudt commented Mar 1, 2017

@sjuchno makes sense thanks!

@arb
Copy link
Contributor

@arb arb commented Mar 2, 2017

Can someone explain this in more detail? Is it because request had a reference to protect and this extra closure wouldn't let the ref to the request object get GCed?

@cjihrig
Copy link
Contributor Author

@cjihrig cjihrig commented Mar 3, 2017

I made a mistake. I made the change to this.domain.on('error', this._onError) and verified in the heap snapshot that the leak was gone. However, when I ran the tests, there were failures, and I had to add the bind(this). This means that the domain still has a reference to all of the hapi objects, and the leak is still there.

The problem is that there are 193 domain objects left around. Most are from lab, but some from protect.js are getting out. From the heap snapshots, it looks like it happens through a list of Immediate objects (setImmediate() in Node core makes them) that connect to lab domains. Still need to do more digging.

@cjihrig
Copy link
Contributor Author

@cjihrig cjihrig commented Mar 3, 2017

It looks like lab is adding a bunch of Immediates to its domain here. Some of those Immediates have _idlePrev pointers to other Immediate objects that were created by hapi, whose domain pointer references the domain created in protect.js. lab never removes the Immediates that it adds, so they stay here, along with the references to all of the hapi stuff. Adding a couple activeDomain.remove(immed); here seems to let the hapi domain references go.

@geek what do you think?

@geek
Copy link
Member

@geek geek commented Mar 3, 2017

@cjihrig sounds like a good change to me... we didn't bother with the removal likely because the process is so short lived with lab

@hueniverse
Copy link
Contributor

@hueniverse hueniverse commented Mar 20, 2017

Should this be revered in hapi?

@cjihrig
Copy link
Contributor Author

@cjihrig cjihrig commented Mar 20, 2017

There's no harm in writing it like this. It still saves on an unnecessary closure.

@lock
Copy link

@lock lock bot commented Jan 9, 2020

This thread has been automatically locked due to inactivity. Please open a new issue for related bugs or questions following the new issue template instructions.

@lock lock bot locked as resolved and limited conversation to collaborators Jan 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

7 participants