http: relax requirements on upgrade listener#19981
http: relax requirements on upgrade listener#19981apapirovski wants to merge 3 commits intonodejs:masterfrom
Conversation
The http spec does not say anything about Upgrade headers making protocol switch mandatory but Node.js implements them as if they are. Relax the requirements to only destroy the socket if no upgrade listener exists on the client when status code is 101.
apapirovski
added
http
|
Can you please run CIGTM? |
|
|
||
| var eventName = req.method === 'CONNECT' ? 'connect' : 'upgrade'; | ||
| if (server.listenerCount(eventName) > 0) { | ||
| if (eventName === 'upgrade' || server.listenerCount(eventName) > 0) { |
There was a problem hiding this comment.
can you combine this if with the ternary before somehow? With this change we truly only need to do server.listenerCount('connect') here.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
As I said, I'm not convinced. Removing the ternary might create code that is harder to read in the end.
There was a problem hiding this comment.
@mcollina ... are you ok with this landing without this change?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 {
// ...
}| req.res = res; | ||
|
|
||
| if (res.upgrade) | ||
| return 2; |
There was a problem hiding this comment.
Can you please add the comment:
// Skip body and treat as Upgrade.
It was there in _http_common.js
apapirovski
added
the
author ready
| test_upgrade_no_listener_ended = true; | ||
| conn.once('data', (data) => { | ||
| assert.strictEqual('string', typeof data); | ||
| assert.strictEqual('HTTP/1.1 200', data.substr(0, 12)); |
There was a problem hiding this comment.
Nit: please switch the arguments order to adhere to actual, expected.
|
CI before landing https://ci.nodejs.org/job/node-test-pull-request/14320/ |
|
Failure on CI is unrelated, landing. |
|
Landed in f7fbbee |
|
@jasnell can you have a look at adding this to 10? |
The http spec does not say anything about Upgrade headers making protocol switch mandatory but Node.js implements them as if they are. Relax the requirements to only destroy the socket if no upgrade listener exists on the client when status code is 101. PR-URL: #19981 Fixes: #11552 Refs: https://tools.ietf.org/html/rfc7230#section-6.7 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
The http spec does not say anything about Upgrade headers making protocol switch mandatory but Node.js implements them as if they are. Relax the requirements to only destroy the socket if no upgrade listener exists on the client when status code is 101. PR-URL: #19981 Fixes: #11552 Refs: https://tools.ietf.org/html/rfc7230#section-6.7 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
7 participants
The http spec does not say anything about Upgrade headers making
protocol switch mandatory but Node.js implements them as if they
are. Relax the requirements to only destroy the socket if no
upgrade listener exists on the client when status code is 101.
Refs: https://tools.ietf.org/html/rfc7230#section-6.7
Fixes: #11552
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes