Skip to content
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

False successfully end of request #8102

Closed
RoobinGood opened this issue Aug 14, 2016 · 12 comments
Closed

False successfully end of request #8102

RoobinGood opened this issue Aug 14, 2016 · 12 comments
Labels
doc Issues and PRs related to the documentations. http Issues or PRs related to the http subsystem. test Issues and PRs related to the tests.

Comments

@RoobinGood
Copy link

  • Version: v4.4.0
  • Platform: Linux robin-master 3.19.0-15-generic #15-Ubuntu SMP Thu Apr 16 23:32:37 UTC 2015 x86_64 x86_64 x86_64 GNU/Linux
  • Subsystem: http

Long client requests ends successfully if data transfer closed for some reason, for example if server lost connectivity or suddenly fall.
For example I have one client Node.js app which make request to server Node.js app to receive a lot of chunks of data. Data transfer lasts for several time and server app falls before request completion.
I expect to get some error on client side to inform me that transfer was not successfully ended, but emits end event instead of error. So I have a situation when I receive only part of data but don't know about it and think that request successfully ended.

Example server send parts of JSON object for ~2 seconds. For test purpose I run following shell command:

timeout 1s node server.js & node client.js

Because data transfer lasts 2 seconds client app receive only part of data but ends with emit end handler without any error. May be problem on clientRequest part of Node.js cause the same trick with curl as client side return (18) transfer closed error.

Example of client app:

var http = require('http');

var makeRequest = function() {
    var req = http.request({
        host: '127.0.0.1',
        port: 8080,
        method: 'POST',
    }, function(res) {
        var data = '';
        res.on('data', function(chank) {
            data += chank;
        });

        res.on('end', function() {
            console.log('data', data);
            console.log('succesfull end');
        });

        res.on('error', function(err) {
            console.error(err);
        });
    });

    req.on('error', function(err) {
        console.error(err);
    });

    req.write('');
    req.end();
};

setTimeout(makeRequest, 100);

Example of server app

var http = require('http');

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

    res.write('{"result":[{"some": "object"}');

    // it must send data pieces for ~2 sec (20*100 ms = 2000 ms) 
    var times = 20;
    var delay = 100;

    var counter = 0;
    var timerId = setInterval(function() {
        res.write(',{"some": "object"}');

        counter++;
        if (counter >= times) {
            res.write(',{"some": "object"}');
            res.write(']}');
            res.end();
            clearInterval(timerId);
        }
    }, delay);
});

var port = 8080;
server.listen(port, function() {
    console.log('listen on 127.0.0.1:' + port);
});
@RoobinGood
Copy link
Author

Also tested on v4.4.7 with the same result.

@MylesBorins
Copy link
Contributor

/cc @bnoordhuis I seem to recall you had answered a similar question before

@mscdex mscdex added the http Issues or PRs related to the http subsystem. label Aug 14, 2016
@bnoordhuis bnoordhuis added doc Issues and PRs related to the documentations. test Issues and PRs related to the tests. docs-requested labels Aug 15, 2016
@bnoordhuis
Copy link
Member

The client sees an empty read when the server is killed. You can verify that with strace.

curl expects a zero-sized HTTP chunk because it sends Connection: keep-alive. The node.js client on the other hand sends Connection: close, in which case it's proper to end the response by simply closing the connection.

Node.js does not raise an error when keep-alive is enabled and the connection is closed before the final zero chunk but you can check if res.complete === true in your 'end' event listener.

res.complete apparently was never documented and we don't seem to have test coverage for it either.

It's been around for a long time though, it was added when support for trailing HTTP headers was implemented in node.js v0.3.0. Adding doc and test labels.

@okv
Copy link

okv commented Aug 15, 2016

Hi, there.

I encountered same problem. I'll try res.complete flag, thanks for explanation.

Node.js does not raise an error when keep-alive is enabled and the connection is closed before the final zero chunk

Why Node.js does not raise an error in such situation? It looks reasonable.

@RoobinGood
Copy link
Author

@bnoordhuis thanks for explanation, res.complete really works.

@bnoordhuis
Copy link
Member

Why Node.js does not raise an error in such situation? It looks reasonable.

I'm not 100% sure but I think it's for compatibility. Not all HTTP endpoints follow the spec as closely as they should.

You could argue, tongue in cheek, that closing the connection without sending the zero chunk is a performance optimization - saves a TCP round-trip!

@jasnell
Copy link
Member

jasnell commented Aug 15, 2016

Yep.. there are some implementations that have been rather pathological about not properly terminating a chunked stream. While throwing would make sense in theory, it ends up being a bit problematic in practice.

@okv
Copy link

okv commented Aug 15, 2016

got it, thanks

@sam-github sam-github added doc Issues and PRs related to the documentations. and removed doc Issues and PRs related to the documentations. docs-requested labels Dec 1, 2016
@Trott
Copy link
Member

Trott commented Jul 15, 2017

@nodejs/documentation @nodejs/testing

@Trott
Copy link
Member

Trott commented Jul 15, 2017

@nodejs/http

@refack
Copy link
Contributor

refack commented Jul 17, 2017

@jasnell does #14315 improve this situation?

@Redocram
Copy link

Redocram commented Nov 3, 2017

Hi everybody,
I have the same problem with my code, or I think so.

May I have a short example of the implementation of the if (res.complete === true) statement for a post request?

Thanks all!

jasnell added a commit to jasnell/node that referenced this issue Oct 26, 2018
targos pushed a commit that referenced this issue Nov 1, 2018
Fixes: #8102

PR-URL: #23914
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
MylesBorins pushed a commit that referenced this issue Nov 27, 2018
Fixes: #8102

PR-URL: #23914
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
MylesBorins pushed a commit that referenced this issue Nov 27, 2018
Fixes: #8102

PR-URL: #23914
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
rvagg pushed a commit that referenced this issue Nov 28, 2018
Fixes: #8102

PR-URL: #23914
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
rvagg pushed a commit that referenced this issue Nov 28, 2018
Fixes: #8102

PR-URL: #23914
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
MylesBorins pushed a commit that referenced this issue Nov 29, 2018
Fixes: #8102

PR-URL: #23914
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. http Issues or PRs related to the http subsystem. test Issues and PRs related to the tests.
Projects
None yet
Development

No branches or pull requests

10 participants