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.

Sign up for GitHub

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

Jump to bottom

http: relax requirements on upgrade listener #19981

Closed
wants to merge 3 commits into from
Closed

http: relax requirements on upgrade listener #19981

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5722ae2
http: relax requirements on upgrade listener
apapirovski Apr 12, 2018
e0cebc4
fixup: add requested comment
apapirovski Apr 12, 2018
b1e5ebd
fixup: reverse args order in assert
apapirovski Apr 15, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 10 additions & 5 deletions doc/api/http.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -471,8 +471,9 @@ added: v0.1.94
* `head` {Buffer}

Emitted each time a server responds to a request with an upgrade. If this
event is not being listened for, clients receiving an upgrade header will have
their connections closed.
event is not being listened for and the response status code is 101 Switching
Protocols, clients receiving an upgrade header will have their connections
closed.

A client server pair demonstrating how to listen for the `'upgrade'` event.

Expand Down Expand Up @@ -873,16 +874,20 @@ per connection (in the case of HTTP Keep-Alive connections).
### Event: 'upgrade'
<!-- YAML
added: v0.1.94
changes:
- version: REPLACEME
pr-url: REPLACEME
description: Not listening to this event no longer causes the socket
to be destroyed if a client sends an Upgrade header.
-->

* `request` {http.IncomingMessage} Arguments for the HTTP request, as it is in
the [`'request'`][] event
* `socket` {net.Socket} Network socket between the server and client
* `head` {Buffer} The first packet of the upgraded stream (may be empty)

Emitted each time a client requests an HTTP upgrade. If this event is not
listened for, then clients requesting an upgrade will have their connections
closed.
Emitted each time a client requests an HTTP upgrade. Listening to this event
is optional and clients cannot insist on a protocol change.

After this event is emitted, the request's socket will not have a `'data'`
event listener, meaning it will need to be bound in order to handle data
Expand Down
8 changes: 6 additions & 2 deletions lib/_http_client.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -426,7 +426,7 @@ function socketOnData(d) {
req.socket._hadError = true;
req.emit('error', ret);
} else if (parser.incoming && parser.incoming.upgrade) {
// Upgrade or CONNECT
// Upgrade (if status code 101) or CONNECT
var bytesParsed = ret;
var res = parser.incoming;
req.res = res;
Expand All @@ -453,7 +453,7 @@ function socketOnData(d) {
req.emit(eventName, res, socket, bodyHead);
req.emit('close');
} else {
// Got Upgrade header or CONNECT method, but have no handler.
// Requested Upgrade or used CONNECT method, but have no handler.
socket.destroy();
}
} else if (parser.incoming && parser.incoming.complete &&
Expand Down Expand Up @@ -492,6 +492,10 @@ function parserOnIncomingClient(res, shouldKeepAlive) {
}
req.res = res;

// Skip body and treat as Upgrade.
if (res.upgrade)
return 2;

// Responses to CONNECT request is handled as Upgrade.
const method = req.method;
if (method === 'CONNECT') {
Expand Down
14 changes: 1 addition & 13 deletions lib/_http_common.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,7 @@ function parserOnHeadersComplete(versionMajor, versionMinor, headers, method,
parser.incoming.httpVersionMinor = versionMinor;
parser.incoming.httpVersion = `${versionMajor}.${versionMinor}`;
parser.incoming.url = url;
parser.incoming.upgrade = upgrade;

var n = headers.length;

Expand All @@ -101,19 +102,6 @@ function parserOnHeadersComplete(versionMajor, versionMinor, headers, method,
parser.incoming.statusMessage = statusMessage;
}

if (upgrade && parser.outgoing !== null && !parser.outgoing.upgrading) {
// The client made non-upgrade request, and server is just advertising
// supported protocols.
//
// See RFC7230 Section 6.7
upgrade = false;
}

parser.incoming.upgrade = upgrade;

if (upgrade)
return 2; // Skip body and treat as Upgrade.

return parser.onIncoming(parser.incoming, shouldKeepAlive);
}

Expand Down
8 changes: 0 additions & 8 deletions lib/_http_outgoing.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,6 @@ function OutgoingMessage() {
this.writable = true;

this._last = false;
this.upgrading = false;
this.chunkedEncoding = false;
this.shouldKeepAlive = true;
this.useChunkedEncodingByDefault = true;
Expand Down Expand Up @@ -304,7 +303,6 @@ function _storeHeader(firstLine, headers) {
// in the case of response it is: 'HTTP/1.1 200 OK\r\n'
var state = {
connection: false,
connUpgrade: false,
contLen: false,
te: false,
date: false,
Expand Down Expand Up @@ -366,10 +364,6 @@ function _storeHeader(firstLine, headers) {
}
}

// Are we upgrading the connection?
if (state.connUpgrade && state.upgrade)
this.upgrading = true;

// Date header
if (this.sendDate && !state.date) {
state.header += 'Date: ' + utcDate() + CRLF;
Expand Down Expand Up @@ -460,8 +454,6 @@ function matchConnValue(self, state, value) {
while (m) {
if (m[0].length === 5)
sawClose = true;
else
state.connUpgrade = true;
m = RE_CONN_VALUES.exec(value);
}
if (sawClose)
Expand Down
11 changes: 9 additions & 2 deletions lib/_http_server.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -530,14 +530,14 @@ function onParserExecuteCommon(server, socket, parser, state, ret, d) {
parser = null;

var eventName = req.method === 'CONNECT' ? 'connect' : 'upgrade';
if (server.listenerCount(eventName) > 0) {
if (eventName === 'upgrade' || server.listenerCount(eventName) > 0) {
Copy link
Member

Choose a reason for hiding this comment

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

can you combine this if with the ternary before somehow? With this change we truly only need to do server.listenerCount('connect') here.

Copy link
Member Author

@apapirovski apapirovski Apr 12, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I don't think we can combine it with the ternary since the eventName is also used inside the if clause when emitting. We could change server.listenerCount(eventName) to be server.listenerCount('connect') but I'm not sure that's truly better... I suppose a bit more descriptive?

Copy link
Member

Choose a reason for hiding this comment

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

As I said, I'm not convinced. Removing the ternary might create code that is harder to read in the end.

Copy link
Member

Choose a reason for hiding this comment

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

@mcollina ... are you ok with this landing without this change?

Copy link
Member Author

Choose a reason for hiding this comment

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

To be clear, I would be happy to make a change but I'm not sure what it would be. I don't think there's a way to simplify this since eventName is used inside the body of the if statement.

Copy link
Member

Choose a reason for hiding this comment

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

The only thing I can think of is reversing the condition but it doesn't change anything.

if (eventName === 'connect' && server.listenerCount('connect') === 0) {
  socket.destroy();
} else {
  // ...
}

Copy link
Member

Choose a reason for hiding this comment

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

Yes definitely.

Copy link
Member

Choose a reason for hiding this comment

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

This can land

debug('SERVER have listener for %s', eventName);
var bodyHead = d.slice(bytesParsed, d.length);

socket.readableFlowing = null;
server.emit(eventName, req, socket, bodyHead);
} else {
// Got upgrade header or CONNECT method, but have no handler.
// Got CONNECT method, but have no handler.
socket.destroy();
}
}
Expand Down Expand Up @@ -592,6 +592,13 @@ function resOnFinish(req, res, socket, state, server) {
function parserOnIncoming(server, socket, state, req, keepAlive) {
resetSocketTimeout(server, socket, state);

if (req.upgrade) {
req.upgrade = req.method === 'CONNECT' ||
server.listenerCount('upgrade') > 0;
if (req.upgrade)
return 2;
}

state.incoming.push(req);

// If the writable end isn't consuming, then stop reading
Expand Down
19 changes: 14 additions & 5 deletions test/parallel/test-http-upgrade-advertise.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,9 @@ const tests = [
{ headers: {}, expected: 'regular' },
{ headers: { upgrade: 'h2c' }, expected: 'regular' },
{ headers: { connection: 'upgrade' }, expected: 'regular' },
{ headers: { connection: 'upgrade', upgrade: 'h2c' }, expected: 'upgrade' }
{ headers: { connection: 'upgrade', upgrade: 'h2c' }, expected: 'upgrade' },
{ headers: { connection: 'upgrade', upgrade: 'h2c' }, expected: 'destroy' },
{ headers: { connection: 'upgrade', upgrade: 'h2c' }, expected: 'regular' },
];

function fire() {
Expand All @@ -32,10 +34,17 @@ function fire() {
done('regular');
});

req.on('upgrade', function onUpgrade(res, socket) {
socket.destroy();
done('upgrade');
});
if (test.expected === 'destroy') {
req.on('socket', () => req.socket.on('close', () => {
server.removeAllListeners('upgrade');
done('destroy');
}));
} else {
req.on('upgrade', function onUpgrade(res, socket) {
socket.destroy();
done('upgrade');
});
}

req.end();
}
Expand Down
8 changes: 3 additions & 5 deletions test/parallel/test-http-upgrade-server.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,8 +121,6 @@ function test_upgrade_with_listener() {
/*-----------------------------------------------
connection: Upgrade, no listener
-----------------------------------------------*/
let test_upgrade_no_listener_ended = false;

function test_upgrade_no_listener() {
const conn = net.createConnection(server.address().port);
conn.setEncoding('utf8');
Expand All @@ -135,8 +133,9 @@ function test_upgrade_no_listener() {
'\r\n');
});

conn.on('end', function() {
test_upgrade_no_listener_ended = true;
conn.once('data', (data) => {
assert.strictEqual(typeof data, 'string');
assert.strictEqual(data.substr(0, 12), 'HTTP/1.1 200');
conn.end();
});

Expand Down Expand Up @@ -182,5 +181,4 @@ server.listen(0, function() {
process.on('exit', function() {
assert.strictEqual(3, requests_recv);
assert.strictEqual(3, requests_sent);
assert.ok(test_upgrade_no_listener_ended);
});