Skip to content
This repository has been archived by the owner on Apr 22, 2023. It is now read-only.

HTTP client mem leak when socket hang up? #3199

Closed
wants to merge 1 commit into from

Conversation

Filirom1
Copy link

@Filirom1 Filirom1 commented May 1, 2012

Hi,

In production we have a serious memory leak. I tried to figure it out using the module weak.
When I remove HTTP clients in my code, I don't have mem leaks. So I thought there was something strange in the HTTP client.

I started with #3179, and found something else when the socket hang up (socket hang up happens in our production code).

When executing this gist, weak tell us that some of our objects are not GC.

# node --expose-gc leak.js
webkit-devtools-agent started on 127.0.0.1:1337
We should do 18 requests
3MB used
Done: 0/18, not yet GC: 0
Got error: socket hang up
Got error: socket hang up
Got error: socket hang up
Got error: socket hang up
Got error: socket hang up 
Got error: socket hang up
Got error: socket hang up
Got error: socket hang up
Got error: socket hang up
Got error: socket hang up
Got error: socket hang up
Got error: socket hang up
Got error: socket hang up
Got error: socket hang up
Got error: socket hang up
Got error: socket hang up
Got error: socket hang up
Got error: socket hang up
90MB used
Done: 18/18, not yet GC: 7
all requests done, memory should be collected by now ?
39MB used
Done: 18/18, not yet GC: 7
all requests done, memory should be collected by now ?

Using webkit-devtools-agent, I found that the leak is retained by an HTTPParser. So i tried this :

exports.FreeList.prototype.free = function(obj) {
  //debug("free " + this.name + " " + this.list.length);
  if (this.list.length < this.max) {
-    this.list.push(obj);
+    // this.list.push(obj);
  }
};

And the leak in the gist disappear.

$ node --expose-gc gistfile1.js 
We should do 18 requests
2MB used
Done: 0/18, not yet GC: 0
Got error: socket hang up
Got error: socket hang up
Got error: socket hang up
Got error: socket hang up
Got error: socket hang up
Got error: socket hang up
Got error: socket hang up
Got error: socket hang up
Got error: socket hang up
Got error: socket hang up
Got error: socket hang up
Got error: socket hang up
Got error: socket hang up
Got error: socket hang up
Got error: socket hang up
Got error: socket hang up
Got error: socket hang up
Got error: socket hang up
89MB used
Done: 18/18, not yet GC: 0
all requests done, memory should be collected by now ?
2MB used
Done: 18/18, not yet GC: 0
all requests done, memory should be collected by now ?
2MB used
Done: 18/18, not yet GC: 0
all requests done, memory should be collected by now ?
2MB used
Done: 18/18, not yet GC: 0
all requests done, memory should be collected by now ?

I tried to do something smarter in this pull request. The http tests still works. I hope this pull-request will help.

Thanks for node. It rocks !

@crickeys
Copy link

crickeys commented May 1, 2012

still seeing if it helps memory leak, but at first run it seems to slow things down a lot

@Filirom1
Copy link
Author

Filirom1 commented May 1, 2012

Tomorrow I will try it in real life, and I will keep an eye on performance. But it's strange because I have just added a call on removeAllListeners();

@crickeys
Copy link

crickeys commented May 1, 2012

nevermind, I think the performance is on my end, still testing memory leak

isaacs added a commit that referenced this pull request May 1, 2012
Regarding #3199 and #3179 and issues seen in production.
Hopefully this fixes them.
mranney pushed a commit to Voxer/node that referenced this pull request May 1, 2012
Regarding nodejs#3199 and nodejs#3179 and issues seen in production.
Hopefully this fixes them.
@isaacs
Copy link

isaacs commented May 2, 2012

We've been tracking down an error that looks suspiciously like this at @joyent that @mranney is encountering at @Voxer.

If bfe9cdb doesn't fix it, then we'll look into other things.

e25611e is not acceptable, because it puts http-specific logic into FreeList, which is not where it belongs.

@isaacs
Copy link

isaacs commented May 2, 2012

Closing this pull req, because it's not the right solution.

@isaacs isaacs closed this May 2, 2012
isaacs added a commit to isaacs/node-v0.x-archive that referenced this pull request May 5, 2012
Regarding nodejs#3199 and nodejs#3179 and issues seen in production.
Hopefully this fixes them.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants