JCMais / node-libcurl

libcurl bindings for Node.js
https://npmjs.org/package/node-libcurl
MIT License
660 stars 117 forks source link

Memory leaks!!! #73

Closed Setitch closed 7 years ago

Setitch commented 7 years ago

var x = 0; function doA() { ++x; var c = new Curl(); c.close(); delete c; }

function Application() { function doSchedule() { console.log(process.memoryUsage(), x); doA('a'); } setInterval(doSchedule, 1);

return this;

}

var app = new Application();

JCMais commented 7 years ago

I can see the memory grow, but I cannot see where is the issue. Snapshots are not showing any object being kept in memory.

Also, using only the Easy / Multi handle seems to keep the memory stable. This will stay open, since I don't have enough time look more into it for now.

Setitch commented 7 years ago

I think its due to some curl process data not cleaning. Its very huge leak.

I read log file on one machine, then spliced each line into object and send it to the machine that was saving the data - it was VERY fast using curl, but:

JCMais commented 7 years ago

Can you test if you have the same issues while using the Easy / Multi constructors directly?

There is an usage example at https://github.com/JCMais/node-libcurl/blob/develop/examples/multi-interface.js

druidvav commented 7 years ago

@Setitch looks like something wrong with your app, i've got no leaks besides natural nodejs GC lag.

bhyte commented 7 years ago

I'm actually seeing the same thing. We actually avoid "GC lag", by calling node with --expose-gc, and the global.gc() function. Using the Curl interface, the memory does indeed grow without limit (it will eventually hit an OOM). And as mentioned, simply switching from the Curl to Easy interface, memory stabilizes within a minute or so (with OP's original test code).

Setitch commented 7 years ago

@druidvav i wrote almost whole code for the application - if it looks like something wrong with it - please SHOW where. I have got leaks (huge ones) on the code i presented here - i even left it for some time to run - it crashed due to memory usage.

I also tested it with --expose-gc - nothing changes - still memory leaks are there and are not cleared after manually executing gc loop.

Therefore - this curl is broken, easy interface works - but i cannot use easy interface, i need curl interface - so library is not for usage (not with SO HUGE with real data).

JCMais commented 7 years ago

@Setitch I have a fix for that already, thanks to @bhyte.

Binding the _onData and _onHeader methods here: https://github.com/JCMais/node-libcurl/blob/develop/lib/Curl.js#L84, is making some object leak, probably the function that was bound, I don't know exactly what is leaking, and why.

I cannot release the fix right now because of other changes I did, but you can manually do it, add:

    this._handle.setOpt( _Curl.option.WRITEFUNCTION, null );
    this._handle.setOpt( _Curl.option.HEADERFUNCTION, null );

To line 1133 of lib/Curl.js

bhyte commented 7 years ago

Turns out that the memory leak I was trying to track down (that lead me here in the first place) is actually in libcurl+nss, the default on CentOS 7. With SSL_VERIFYPEER on (default), nss will leak a little bit of memory (and worse, won't show up in valgrind) with every HTTPS request; setting the value to 0 avoids the memory leak, but of course this is not practical. Calling curl_global_init() / curl_global_cleanup() more than once during runtime can workaround this issue (not exposed by node-libcurl...?). So, rolling our own libcurl is ideal in this case, however, maybe we could consider exposing global_init/cleanup to workaround the issue when changing the curl version isn't possible?

Setitch commented 7 years ago

Hmmm not good... But i wouldnt call it a bit - after 2-3k calls to curl i got 200-500mb more--_-- depending on data.... that is huuge memory leak.

bhyte commented 7 years ago

Not literally "a bit"; a lot more than a literal bit (was using a figure of speech :) Anyway, updating to the latest curl has been memory-stable for me since with thousands of requests over HTTPS.

JCMais commented 7 years ago

@Setitch so, just to confirm, your original issue is the same that @bhyte is demonstrating (with nss)?

Setitch commented 7 years ago

Sorry but im not really sure - temporary im out of time to test that:(

JCMais commented 7 years ago

This should be fixed on v1.1.0. It will be published to npm today.

tomrf commented 7 years ago

I think I'm still seeing this issue in v1.1.0 (node v6.9.4)

Doing thousands of requests per hour, heap is OK but RSS keeps growing.

Disabling SSL_VERIFYPEER seems to help, as I'm not seeing the same rate of growth in memory use then.

Have not tried using Easy or Multi interfaces yet, but will try that later today.

tomrf commented 7 years ago

Switched to using Easy + Multi interfaces, and there is no leaking.

Edit: I posted too soon -- still leaking, but at a slower / lower rate.

gsouf commented 4 years ago

Apparently still happening in recent versions.

JCMais commented 4 years ago

@gsouf are you able to post some code that can reproduce the memory leak you are having?

gsouf commented 4 years ago

@JCMais I think the issue might be somehow different to what I thought. It appears to be related to downloading large files see #245

AsifArko commented 3 years ago

For the request below I couldn't specify the -X BAN part with node-libcurl. Can anyone assist me please ? curl -is http://localhost:8080/ -X BAN -H "x-ban: ^/sports" -H "x-authorization: 123123123"

`const curl = new Curl(); const close = curl.close.bind(curl);

curl.setOpt('URL', http://${varnishHost}:${varnishPort}); curl.setOpt(Curl.option.HTTPHEADER, ['x-ban: ^/sports', x-authorization: ${token}]);

curl.on('end', function (statusCode, data, headers) { console.log(data) this.close(); }); curl.on('error', close); curl.perform();`

This is the snippet I am using. I don't exactly know how to set the -X param to BAN with libcurl. Thanks in advance