Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

HTTP servers do not have a reliable way of being notified when clients prematurely disconnect #7065

Open
mcavage opened this Issue · 3 comments

4 participants

@mcavage

This has pretty much always been true in all version of node historically, but it has become a burning production issue, and still happens in 0.10/0.11. Basically http.js has a clientError event on the server object, however this doesn't contain a request/response pair, and so it's essentially useless to a user of figuring out what do if there were any backend resources that need to be cleaned up as a result of the disconnect (I realize there are "secret" ways to traverse from a socket object, but it's not documented/guaranteed). The only reliable way that I can see to get this now is by listening for req.on("close"), but that doesn't account for half-open sockets, and so I see behavior of our web servers shutting down clients in this state and then 2 minutes later node's "socket reaper" kicks in and closes the socket (meaning the socket + request emitted "close" but didn't actually close the socket).

This behavior happens on both uploads and downloads, and I've attached a self-contained test case to demonstrate this. If you find problems with the test case, please do not just close this issue as this behavior is easily reproducible using say curl and ctrl-c'ing the client mid-datastream. The test cases uses a "passthrough" stream only to mimic what applications do if piping to multiple things (I do).

I'm not really sure from the docs or code what the intended way of an application being notified is, but I hope this issue serves to refine that and document it (for example, maybe the "pipeline" should emit "error", maybe it should emit "close" - I can see a number of possibilities there).

Lastly, there's yet another bug visible in the test case - at least on the upload path if the client.abort() is not wrapped in a (long) setTimeout, then the client request will emit an "error", even though a supported "cancellation" API was used. To repro this, just replace setTimeout with process.nextTick.

To run the test case: node foo.js will reproduce download failures, node foo.js post will reproduce the upload failures

var assert = require('assert');
var fs = require('fs');
var http = require('http');
var PassThrough = require('stream').PassThrough;

var PORT = 1234;


function clientGet(cb) {
    var p = 'http://127.0.0.1:' + PORT + '/';
    var req = http.get(p, function (res) {
        res.destroy();
        cb();
    });
}


function clientPost(cb) {
    var req = http.request({
        hostname: '127.0.0.1',
        port: PORT,
        path: '/',
        method: 'POST'
    });

    req.write(fs.readFileSync(__filename));
    setTimeout(function () {
        req.abort();
    }, 100);

    req.on('error', function (err) {
        console.error('WTF error: ' + err.toString());
        cb();
    });
}


function handleGetRequest(req, res) {
    var reader = fs.createReadStream(__filename, {
        highWaterMark: 1
    });
    var my_stream = new PassThrough({
        highWaterMark: 1
    });

    reader.once('open', function () {
        res.writeHead(200);
        reader.pipe(my_stream).pipe(res);
    });

    var saw_end = false;
    req.once('close', function () {
        assert.ok(saw_end);
    });

    my_stream.once('end', function () {
        saw_end = true;
    });
}


function handlePostRequest(req, res) {
    var my_stream = new PassThrough({
        highWaterMark: 1
    });
    var writer = fs.createWriteStream('/dev/null', {
        highWaterMark: 1
    });

    var saw_end = false;
    var saw_error = false;

    req.pipe(my_stream).pipe(writer).on('error', function () {
        saw_error = true;
    });

    req.on('end', function () {
        saw_end = true;
    });

    req.on('close', function () {
        assert.ok(saw_end || saw_error);
    });
}


(function main() {
    var server = http.createServer(function (req, res) {
        if (req.method === 'POST') {
            handlePostRequest(req, res);
        } else {
            handleGetRequest(req, res);
        }
    });

    server.listen(PORT, function () {
        function close() {
            server.close();
        }

        if (process.argv.length > 2) {
            clientPost(function () {});
        } else {
            clientGet(close);
        }
    });
})();
@tjfontaine
Owner

Those issues @OrangeDog mentions are valid scenarios for the need of 'clientError', we've accepted the connection, have a valid socket, but failed to accurately parse the headers or some other protocol level bug that prevented us from firing the request handler.

What's unacceptable about this current behavior, is that once we have fired the request handler a 'clientError' ceases to contain the appropriate context to make useful decisions.

What we have is a duplex socket, split into a readable (the request) and a writable (the response), and when the underlying socket hits an error, we emit on the socket, catch in the http layer, re-emit on the server via 'clientError' and then the socket initiates the destroy process itself resulting in the close notification -- even though we won't likely close because more often than not we're in allowHalfOpen so we're still capable of writing and probably stuck in a needDrain mechanism.

What should be happening here, after we've fired the request handler, we de-register the 'clientError' wrapper, and instead for socket level errors with a 'syscall' of 'read' we emit that error on the readable (request) object, and for 'write' we emit that error on the writable (response) object.

Unfortunately this is probably too much of a change in behavior, such that we will need to accommodate people relying on the 'clientError' path, we'll just leave that existing just also do the right thing as well.

@tjfontaine
Owner

see also #5134

@trevnorris trevnorris added the http label
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.