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

No finish/close event on aborted http-response - race condition #1309

Closed
not-implemented opened this Issue Apr 1, 2015 · 8 comments

Comments

Projects
None yet
7 participants
@not-implemented
Contributor

not-implemented commented Apr 1, 2015

When tracking requests of a HTTP-Server - with server.on('request') and res.on('finish', ...) or res.on('close', ...) - we noticed inconsistent results over time (requests that are never finished or closed).

We tracked this race condition down to the following reproducable test case:

  1. connection is aborted from client side
  2. socket gets destroyed (but socket-"close"-event still not delivered)
  3. trying to send response
  4. socket-"close" event delivered

-> No "close" and no "finish" event on response!

var common = require('../common');
var assert = require('assert');
var http = require('http');

var clientRequest = null;
var serverResponseFinishedOrClosed = 0;

var server = http.createServer(function (req, res) {
    console.log('server: request');

    res.on('finish', function () {
        console.log('server: response finish');
        serverResponseFinishedOrClosed++;
    });
    res.on('close', function () {
        console.log('server: response close');
        serverResponseFinishedOrClosed++;
    });

    console.log('client: aborting request');
    clientRequest.abort();

    setImmediate(function() {
        console.log('server: tick 1' + (req.connection.destroyed ? ' (connection destroyed!)' : ''));

        setImmediate(function () {
            console.log('server: tick 2' + (req.connection.destroyed ? ' (connection destroyed!)' : ''));

            console.log('server: sending response');
            res.writeHead(200, {'Content-Type': 'text/plain'});
            res.end('Response\n');
            console.log('server: res.end() returned');

            setImmediate(function () {
                server.close();
            });
        });
    });
});

server.on('listening', function () {
    console.log('server: listening on port ' + common.PORT);
    console.log('client: starting request');

    var options = {port: common.PORT, path: '/'};
    clientRequest = http.get(options, function () {});
    clientRequest.on('error', function () {});
});

server.on('connection', function (connection) {
    console.log('server: connection');
    connection.on('close', function () { console.log('server: connection close'); });
});

server.on('close', function () {
    console.log('server: close');
    assert.equal(serverResponseFinishedOrClosed, 1, 'Expected either one "finish" or one "close" event on the response for aborted connections (got ' + serverResponseFinishedOrClosed + ')');
});

server.listen(common.PORT);
  • With one more setImmediate() call, you get a response close event (which is fine)
  • With one less setImmediate() call, you get a response finish event (which is fine)
  • Reproducable with io.js 1.6.3, io.js 1.3.0, io.js 1.1.0, node 0.12.0
  • In node 0.10 the race-condition was different: This test case works fine, but with one more setImmediate(), two events are delivered (finish AND close)
@not-implemented

This comment has been minimized.

Show comment
Hide comment
@not-implemented

not-implemented Apr 1, 2015

Contributor

The different behaviour between node 0.10 (<=0.11.5) and node 0.12 (>=0.11.6) may be related to the changes made by @isaacs ("http: Consistent 'finish' event semantics", 7c9b607, and "http: Add write()/end() callbacks" da93d6a).

Contributor

not-implemented commented Apr 1, 2015

The different behaviour between node 0.10 (<=0.11.5) and node 0.12 (>=0.11.6) may be related to the changes made by @isaacs ("http: Consistent 'finish' event semantics", 7c9b607, and "http: Add write()/end() callbacks" da93d6a).

@Olegas

This comment has been minimized.

Show comment
Hide comment
@Olegas

Olegas Apr 1, 2015

Contributor

+1
Can see this happens sometimes.
Neither finish nor close event is triggered on res. Stable reproduces in our environment, we can see this graphically (we count "active" connections with finish/close events).

Contributor

Olegas commented Apr 1, 2015

+1
Can see this happens sometimes.
Neither finish nor close event is triggered on res. Stable reproduces in our environment, we can see this graphically (we count "active" connections with finish/close events).

@not-implemented

This comment has been minimized.

Show comment
Hide comment
@not-implemented

not-implemented Apr 8, 2015

Contributor

Proposed fix see pull request #1373

Contributor

not-implemented commented Apr 8, 2015

Proposed fix see pull request #1373

@Fishrock123

This comment has been minimized.

Show comment
Hide comment
@Fishrock123
Member

Fishrock123 commented Apr 17, 2015

@brendanashworth

This comment has been minimized.

Show comment
Hide comment
@brendanashworth

brendanashworth May 14, 2015

Member

The fix for this (#1411, semver-major) is now on track for the 3.0.0 release.

Member

brendanashworth commented May 14, 2015

The fix for this (#1411, semver-major) is now on track for the 3.0.0 release.

@KoltesDigital

This comment has been minimized.

Show comment
Hide comment
@KoltesDigital

KoltesDigital Aug 13, 2015

Could it please be released earlier? I'm afraid 3.0.0 won't come soon.

KoltesDigital commented Aug 13, 2015

Could it please be released earlier? I'm afraid 3.0.0 won't come soon.

@Fishrock123

This comment has been minimized.

Show comment
Hide comment
@Fishrock123

Fishrock123 Aug 13, 2015

Member

3.0.0 is out and this landed in it. :) https://iojs.org/dist/v3.0.0/

Member

Fishrock123 commented Aug 13, 2015

3.0.0 is out and this landed in it. :) https://iojs.org/dist/v3.0.0/

danni pushed a commit to ask-izzy/ask-izzy that referenced this issue Oct 30, 2015

Danielle Madeley
Update NodeJS to 4.x
Fixes memory leak caused by nodejs/node#1309
@evantahler

This comment has been minimized.

Show comment
Hide comment
@evantahler

evantahler May 20, 2016

I've found something similar here, which you can emulate with node v4 and v5... just by sending the Content-Length header.

https://gist.github.com/evantahler/2f6c4241c47d7c89f5555d833475c8b8

evantahler commented May 20, 2016

I've found something similar here, which you can emulate with node v4 and v5... just by sending the Content-Length header.

https://gist.github.com/evantahler/2f6c4241c47d7c89f5555d833475c8b8

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment