Socket not returning to HTTP agent when exception thrown inside of event leading to backed up http pool

[vshlos]

I have found an issue where if there is an exception thrown in the handler of a http response, then this connection will not be returned to the pool until it times out a few minutes later. This causes the pool to backup and can lead to a denial of service attack or other issues in which node will not be able to talk to a host for a given amount of time.

Here is a simple proof of concept:

var http = require("http")

function test() {
    var options = {
        host: "google.com",
        port: 80,
        path: "/"
    }

    console.log("started")
    var req = http.request(options, function (res) {
        console.log("response")

        res.on("end", function () {
            throw new Error("HELLO")

        })
    })
    req.end()

}

process.on("uncaughtException", function () {
    console.log("uncaught");
})

for (var i = 0; i < 10; i++)
    test();

This code sample will display "started" 10 times and "response" 5 times (maxSockets is 5) then hang for a couple of minutes until those sockets timeout, then show "response" 5 more times.

This exception can be thrown on the response callback or in any of the emit events and will cause a similar issue.

This is especially noticeable when you are running a test framework like mocha and on the response you check the JSON object with an assert which throws an exception and causes the rest of the tests that talk to the same endpoint to fail or timeout.

For example if you have the following type of code

test.makeRequest(function (result) {
    assert(result.firstName != "")
})

After 5 of these tests fail, the rest will fail with a timeout even though the test might otherwise pass.

[sreuter]

To me it looks like this also happens on catched errors, like when a client request timeouts. This seems only to happen on 0.10.1, not on 0.6.19 or 0.8.21. I'll have a look if i can show off the behavior I see with a small test.

[bnoordhuis]

I'll refer you to the documentation for the 'uncaughtException' event:

Note that `uncaughtException` is a very crude mechanism for exception
handling and may be removed in the future.

Don't use it, use [domains](domain.html) instead. If you do use it, restart
your application after every unhandled exception!

Do *not* use it as the node.js equivalent of `On Error Resume Next`. An
unhandled exception means your application - and by extension node.js itself -
is in an undefined state. Blindly resuming means *anything* could happen.

Think of resuming as pulling the power cord when you are upgrading your system.
Nine out of ten times nothing happens - but the 10th time, your system is bust.

You have been warned.

[isaacs]

Reopening to address the underlying issue here: we should free the socket before emitting the response.end event, probably, not after.

19:03 <@isaacs> crabdude: well ,yeah, i mean, the one that threw timed out.
19:03 <@isaacs> crabdude: back to "make node core more stable" ok, fine.
19:04 <@isaacs> crabdude: we should probably clean up the socket *before* emitting end.
19:04 <@isaacs> crabdude: http is scheduled to be rewritten in 0.12
19:04 < crabdude> alright, so that's one response
19:04 <@isaacs> crabdude: that wasn't made clear in the original issue, clearly.  i apologize for missing that aspect
                of it.
19:05 < crabdude> but this is only one symptom of what I see as a bug in EE
19:05 <@isaacs> crabdude: a cursory reading of it seems like you're saying, "I did process.on('uncaughtException')
                but when an error happened, it still
                caused bad things to happen"
19:05 <@isaacs> crabdude: which is kind of like, yeah... um... can't really do much about that.

[bnoordhuis]

Won't that make it impossible to queue a new request in the 'end' listener that reuses the current connection?

[isaacs]

@bnoordhuis If the net result of this is "Can't reasonably fix without major changes" we can always put it off until 0.12. I just reopened because there is an actual issue here.

[isaacs]

Original issue fixed.