fix(bodyReader): Fix memory leak

[daviande]

Without handling the req 'close' event, we will leak ClientRequest objects when the client suddently stops sending the request body.

Repro:

1. create a big file (1gb in this case) -- doesn't matter that its not actually json:
   head -c 1073741824 </dev/urandom >myfile.json

2. create a smaller file:
   echo '{"foo": "bar"}' > small_file.json

3. 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);
});

4. Start the node server:
   node app.js

5. Curl w/ small_file.json:
   curl -v -H "Content-Type: application/json" -X POST -d @small_file.json http://localhost:8080/mypost

6. Note that there is 1 in flight request (see stdout from app.js)

7. Curl w/ the larger file:
   curl -v -H "Content-Type: application/json" -X POST -d @myfile.json http://localhost:8080/mypost

8. 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

9. 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)

[yunong]

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.

[DonutEspresso]

We may have to handle the aborted event as well.

[daviande]

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.

[daviande]

Should I wrap all the next calls in bodyReader w/ once?

[yunong]

You can either do that, or when next fires deregister all the other listeners on the request that you've previously attached.

[yunong]

@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?

[yunong]

How feasible is it to add a test for this?

[DonutEspresso]

We may have to handle the aborted event as well.

[DonutEspresso]

@yunong which array? if similar to #1462 we might tackle it there instead.

[daviande]

@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.

[daviande]

I'll look into adding a test, not sure how the restify tests are structured.