Repository navigation
HTTPParser inhibits garbage collection on keep-alive connections #9668
Description
- Version: v7.0.0
- Platform: Darwin hostname redacted 15.6.0 Darwin Kernel Version 15.6.0: Thu Sep 1 15:01:16 PDT 2016; root:xnu-3248.60.11~2/RELEASE_X86_64 x86_64
I'm not too familiar with node internals, so please correct any details that I may have gotten wrong.
HTTPParser objects appear to be created on a per-socket basis, and retain an incoming reference to an IncomingMessage. The incoming reference is kept around even after the message has been completely parsed and handled, making it un-gc-able for as long as the underlying socket remains open.
At Twitter, we've found that this can contribute to high memory usage when:
- node is running behind a large pool of proxies with keep-alive connections to our service
- using express locals to store a large chunk of application state on the request.
In this environment, already-handled messages are unable to be garbage collected until either a new request comes in on the existing socket or the connection is closed.
I put together a small demo of this issue at jbellenger/node-message-retention
Activity
addaleax commentedon Nov 17, 2016
@nodejs/http
sam-github commentedon Nov 18, 2016
Duplicate of #9268, PR #9440
targos commentedon Jan 8, 2017
#9440 landed.
misterdjules commentedon Jan 10, 2017
It seems that the original comment and repro code were describing a memory leak on the server side, not on the client side. However, it seems that #9440 fixes a leak in the HTTP client's implementation.
I would think that this issue is not a duplicate of #9628, and is not fixed by #9440. As a result, reopening it, my apologies if I'm missing something.
iagomls commentedon Mar 24, 2017
I don't know if it can be related, but i'm posting here.
We have tested several http libraries (request, superagent, got, node-fetch) pointing to a specific url. After running about ~20k requests in some minutes, we got this in our heapdump comparison:
HTTPPARSERseems to be allocating more and more memory, but it's not being released after we stop sending traffic. 10 minutes after stopping to send traffic, we still were using about ~250mb per cluster while we generally use about ~80mb without using any request library.While we keep sending traffic and using a request library, the memory keeps growing and growing until the app crashes. As the url we're using has a
keepaliveconnection, this may be related.Trott commentedon Jul 30, 2017
Should this remain open?
bnoordhuis commentedon Aug 1, 2017
I think this is still relevant. I'll take a look.
bnoordhuis commentedon Aug 2, 2017
I can confirm that it's still an issue but I suspect it cannot be fixed.
The request object is stored in the
state.incomingarray and bound to theresOnFinish()callback in order to clean up outstanding request body data when the response is finalized or aborted.What is wryly amusing is that it's a mitigation for resource leaks when people hold on to the request object but it evidently replaces one kind of leak with another.
mflash1 commentedon Sep 28, 2017
Node V8.2.1
we stream files using multer by sending http multipart requests.
we see the same memory leak on the server , HTTPParser objects are created per request and are not released after upload is ended. each one of them consumes
the memory consumption increasing and GC is unable to release the objects.
ischyron commentedon Nov 17, 2017
Node v6.10.3
@iagomelanias node-fetch does not support keep-alive. https://github.com/bitinn/node-fetch#class-request.
Wondering if this happens to even non keep-alive requets ?
I see that http-parser is allocated even after load stops and after manually triggering gc on a SIGINT.
mflash1 commentedon Nov 17, 2017
Necromos commentedon Feb 16, 2018
Is there any solution/idea how to solve this issue yet?
Also it looks like this issue is present in:
6.13.0
8.2.1
8.9.4
9.5.0
13 remaining items
addaleax commentedon Aug 24, 2019
#29297 should fix this and does not break the test added in #29263.
http: reset parser.incoming when server request is finished
paolomainardi commentedon May 31, 2020
Not sure if this fix has been backported to Node 12.x too
addaleax commentedon Jun 1, 2020
@paolomainardi It has been released in v12.9.1.
paolomainardi commentedon Jun 1, 2020
Thanks @addaleax
http: reset parser.incoming when server response is finished
http: reset parser.incoming when server request is finished