Repository navigation
http: Memory Leak #26957
Description
Running this will cause the memory usage of the node process to infinitly grow...
- Version: 10.13.0
- Platform: OSX
- Subsystem:
const http = require('http')
const { spawn } = require('child_process')
const crypto = require('crypto')
const fs = require('fs')
const dst = fs.createWriteStream('./tmp')
http.createServer((req, res) => {
req
.on('data', buf => {
if (!dst.write(buf)) {
req.pause()
}
})
dst.on('drain', () => {
if (req.readable) {
req.resume()
}
})
}).listen(9988, () => {
spawn('cat', ['/dev/zero'])
.stdout
.pipe(http
.request({
hostname: 'localhost',
port: 9988,
path: `/file`,
method: 'POST'
})
)
})
Activity
[-]Memory Leak[/-][+]http: Memory Leak[/+]ronag commentedon Mar 28, 2019
Backpressure seems to work, however the
bufdoesn't seem to be released. They seem to be allocated using some sort of external memory since they don't seem to be part of the node heap.ronag commentedon Mar 28, 2019
Interestingly using
pipeinstead ofdata&draindoes not seem to have the same problem?ronag commentedon Mar 28, 2019
Seems like
IncomingMessagedoes some strange stuff thatpipehandles which is outside of the normal "stream protocol"?mcollina commentedon Mar 28, 2019
Does this happen on the client side or on the server side?
ronag commentedon Mar 28, 2019
@mcollina: server side
ronag commentedon Mar 28, 2019
Seems related to chunked encoding
mcollina commentedon Mar 28, 2019
@ronag are you working on a PR?
ronag commentedon Mar 28, 2019
@mcollina no, I’m trying to find the problem but so far I have no idea. This might be over my head.
ronag commentedon Mar 28, 2019
Our findings so far seem to indicate a difference between how
Readable.resumeandReadable.piperesumes/pauses streams in a way whereIncomingMessageandHttpServerdepends on. In particularpipeOnDrainandresumeseem to differ.e.g. if instead of calling
IncomingMessage.resume()I use the following code:I seem to no longer have any leaks. Though I don’t dare using such a workaround in production without understanding why that helps. My suspicion is the sync vs async resume behavior which is different between
resumeandpipeOnDrain.stream: do not unconditionally call `_read()` on `resume()`
addaleax commentedon Mar 28, 2019
@ronag Fwiw, I’ve opened a PR to address this in #26965. It would be great if you could verify that that fully works for you.
stream: do not unconditionally call `_read()` on `resume()`
stream: do not unconditionally call `_read()` on `resume()`
stream: do not unconditionally call `_read()` on `resume()`
stream: do not unconditionally call `_read()` on `resume()`
ronag commentedon Jun 1, 2019
@mcollina I'm a little surprised this wasn't fixed/merged in/into LTS? Is that on purpose?
mcollina commentedon Jun 3, 2019
I'm a little surprised of your question. There is no evidence of anybody blocking #26965 getting into Node 10. Likely, it seem they just need some help.
stream: do not unconditionally call `_read()` on `resume()`
stream: do not unconditionally call `_read()` on `resume()`
stream: do not unconditionally call `_read()` on `resume()`