Skip to content
This repository has been archived by the owner on Apr 22, 2023. It is now read-only.

Commit

Permalink
http: do not send 0\r\n\r\n in TE HEAD responses
Browse files Browse the repository at this point in the history
When replying to a HEAD request, do not attempt to send the trailers and
EOF sequence (`0\r\n\r\n`). The HEAD request MUST not have body.

Quote from RFC:

The presence of a message body in a response depends on both the
request method to which it is responding and the response status code
(Section 3.1.2).  Responses to the HEAD request method (Section 4.3.2
of [RFC7231]) never include a message body because the associated
response header fields (e.g., Transfer-Encoding, Content-Length,
etc.), if present, indicate only what their values would have been if
the request method had been GET (Section 4.3.1 of [RFC7231]).

fix #8361

Reviewed-By: Timothy J Fontaine <tjfontaine@gmail.com>
  • Loading branch information
indutny authored and tjfontaine committed Sep 16, 2014
1 parent 6e689ec commit 1fddc1f
Show file tree
Hide file tree
Showing 2 changed files with 40 additions and 24 deletions.
6 changes: 5 additions & 1 deletion lib/http.js
Original file line number Diff line number Diff line change
Expand Up @@ -945,6 +945,10 @@ OutgoingMessage.prototype.end = function(data, encoding) {
if (encoding === 'hex' || encoding === 'base64')
hot = false;

// Transfer-encoding: chunked responses to HEAD requests
if (this._hasBody && this.chunkedEncoding)

This comment has been minimized.

Copy link
@evantorrie

evantorrie Feb 9, 2015

Shouldn't this have been a if (!this._hasBody && this.chunkedEncoding) in order to match the comment? i.e. HEAD responses do not have a body.

This comment has been minimized.

Copy link
@trevnorris

trevnorris Feb 10, 2015

Good catch. @indutny mind giving some feedback?

This comment has been minimized.

Copy link
@indutny

indutny Feb 10, 2015

Author Member

I think @evantorrie is right!

This comment has been minimized.

Copy link
@trevnorris

trevnorris Feb 10, 2015

@indutny so should the comment change, or the code?

This comment has been minimized.

Copy link
@indutny

indutny Feb 10, 2015

Author Member

I'd go with code, assuming that the tests will continue to pass with it.

This comment has been minimized.

Copy link
@trevnorris

trevnorris Feb 10, 2015

Okay. Now I'd just like to figure out how to add a test for this...

This comment has been minimized.

Copy link
@trevnorris

trevnorris Feb 10, 2015

Actually, running the tests now. Causes test-http to hang.

This comment has been minimized.

Copy link
@evantorrie

evantorrie Feb 11, 2015

Is it the same type of hang that you experienced when this was originally committed? According to @tjfontaine, the tests slowed down tremendously due to the original patch.

My experience has been that this commit resulted in a wrk workload for a simple Hello World Linux server slowing down from 4K+ requests/second to only 50 requests/second (i.e. an 80x slowdown).

This comment has been minimized.

Copy link
@trevnorris

trevnorris Feb 11, 2015

@evantorrie it was also causing other tests to fail.

This comment has been minimized.

Copy link
@evantorrie

evantorrie Feb 12, 2015

@trevnorris FYI, you may want to see my comment regarding overall performance degradation that I see (admittedly in a very synthetic benchmark). #9026 (comment)

hot = false;

if (hot) {
// Hot path. They're doing
// res.writeHead();
Expand Down Expand Up @@ -982,7 +986,7 @@ OutgoingMessage.prototype.end = function(data, encoding) {
}

if (!hot) {
if (this.chunkedEncoding) {
if (this._hasBody && this.chunkedEncoding) {
ret = this._send('0\r\n' + this._trailer + '\r\n', 'ascii');
} else {
// Force a flush, HACK.
Expand Down
58 changes: 35 additions & 23 deletions test/simple/test-http-head-request.js
Original file line number Diff line number Diff line change
Expand Up @@ -26,32 +26,44 @@ var util = require('util');


var body = 'hello world\n';
var id = 0;

var server = http.createServer(function(req, res) {
common.error('req: ' + req.method);
res.writeHead(200, {'Content-Length': body.length});
res.end();
server.close();
});
function test(headers) {
var port = common.PORT + id++;

var server = http.createServer(function(req, res) {
console.error('req: %s headers: %j', req.method, headers);
res.writeHead(200, headers);
res.end();
server.close();
});

var gotEnd = false;

var gotEnd = false;

server.listen(common.PORT, function() {
var request = http.request({
port: common.PORT,
method: 'HEAD',
path: '/'
}, function(response) {
common.error('response start');
response.on('end', function() {
common.error('response end');
gotEnd = true;
server.listen(port, function() {
var request = http.request({
port: port,
method: 'HEAD',
path: '/'
}, function(response) {
console.error('response start');
response.on('end', function() {
console.error('response end');
gotEnd = true;
});
response.resume();
});
response.resume();
request.end();
});
request.end();
});

process.on('exit', function() {
assert.ok(gotEnd);
process.on('exit', function() {
assert.ok(gotEnd);
});
}

test({
'Transfer-Encoding': 'chunked'
});
test({
'Content-Length': body.length
});

7 comments on commit 1fddc1f

@trevnorris
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@indutny @tjfontaine This is causing test-http-many-keep-alive-connections.js to stall indefinitely. I'll try to take a look, but would one of you two mind also taking a look?

@indutny
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure that it is related to this patch? Also, I'm failing to reproduce it, but I'd suspect that it is related to creating 10000 connection, which could sometimes overflow ephemeral ports on the machine...

@trevnorris
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right now all I know for sure is that when I build 6e689ec (parent of this commit) the test completes successfully. And it doesn't on this commit.

@tjfontaine
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you have an environment configuration description such that we can reproduce it?

@tjfontaine
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

correction I see it

@tjfontaine
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right we're now hitting 1fddc1f#diff-1c0f1c434b17b7f8795d44a51a14320aR992 instead of the hot path case for writeHead(); end() and always forcing a flush, resulting in the test slowing down drastically

@trevnorris
Copy link

@trevnorris trevnorris commented on 1fddc1f Sep 24, 2014 via email

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please sign in to comment.