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
fix(bodyReader): Fix memory leak #1566
Conversation
lib/plugins/bodyReader.js
Outdated
@@ -172,6 +172,7 @@ function bodyReader(options) { | |||
}); | |||
|
|||
req.once('error', next); | |||
req.once('close', next); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please wrap the next function here in a once
? We're about to tidy up the router and server such that it's going to enforce strict next, and so we would want to make sure we're not potentially calling next here.
If you can also add some comments about why we're adding the extra listener here, given the context you have about the memory leak, that would be even more awesome.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We may have to handle the aborted
event as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I think handling aborted is a good idea. I was looking at the ReadableStream docs, but it appears ClientRequest (the req object) adds a couple of additional events.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should I wrap all the next calls in bodyReader w/ once?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can either do that, or when next fires deregister all the other listeners on the request that you've previously attached.
@daviande Awesome find on the memory leak. Any thoughts to adding that array we talked about to the request object which would make finding a problem like this easier in the future? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How feasible is it to add a test for this?
lib/plugins/bodyReader.js
Outdated
@@ -172,6 +172,7 @@ function bodyReader(options) { | |||
}); | |||
|
|||
req.once('error', next); | |||
req.once('close', next); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We may have to handle the aborted
event as well.
@yunong We already have req._currentHandler, so I'm not too convinced that adding an array adds much additional information for debugging memory leaks. Ultimately, I'd be most interested in which middleware I'm stuck in, when debugging future issues. |
I'll look into adding a test, not sure how the restify tests are structured. |
Without handling the req 'close' event, we will leak ClientRequest objects when the client suddently stops sending the request body.
aec11c3
to
aba77c6
Compare
Without handling the req 'close' event, we will leak ClientRequest objects when the client suddently stops sending the request body.
Without handling the req 'close' event, we will leak ClientRequest objects when the client suddently stops sending the request body.
Without handling the req 'close' event, we will leak ClientRequest objects when the client suddently stops sending the request body.
Without handling the req 'close' event, we will leak ClientRequest objects when the client suddently stops sending the request body.
Without handling the req 'close' event, we will leak ClientRequest objects when the client suddently stops sending the request body.
Without handling the req 'close' event, we will leak ClientRequest objects when the client suddently stops sending the request body.
Repro:
create a big file (1gb in this case) -- doesn't matter that its not actually json:
head -c 1073741824 </dev/urandom >myfile.json
create a smaller file:
echo '{"foo": "bar"}' > small_file.json
Create a node.js app.js:
var restify = require('restify');
var server = restify.createServer();
server.use(restify.plugins.bodyReader());
server.post('/mypost', function mypost(req, res, next) {
console.log(req.body);
console.log(server.inflightRequests());
console.log(req._currentHandler);
res.send(200);
return next();
});
server.listen(8080, function() {
console.log('%s listening at %s', server.name, server.url);
});
Start the node server:
node app.js
Curl w/ small_file.json:
curl -v -H "Content-Type: application/json" -X POST -d @small_file.json http://localhost:8080/mypost
Note that there is 1 in flight request (see stdout from app.js)
Curl w/ the larger file:
curl -v -H "Content-Type: application/json" -X POST -d @myfile.json http://localhost:8080/mypost
Before request completes, ctrl+c curl. Note, this ctrl+c has to happen once we've entered the bodyReader middleware. So, it can be helpful to add a print statement to the bodyReader just so you can see when you've started to consume the POST body. NOTE: the mypost middleware (from app.js) never runs, since we aren't calling .once('close', next) in bodyReader.js
If you curl w/ the small file again:
curl -v -H "Content-Type: application/json" -X POST -d @small_file.json http://localhost:8080/mypost
You'll see the inflight request is up to 2 now (since we've leaked the previous big file post)