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

wants to merge 3 commits into from

Conversation

apapirovski
Copy link
Member

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), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@apapirovski
http: relax requirements on upgrade listener
5722ae2 
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 apapirovski added http Issues or PRs related to the http subsystem. semver-major PRs that contain breaking changes and should be released in the next major version. labels Apr 12, 2018
@apapirovski apapirovski added this to the 10.0.0 milestone Apr 12, 2018
@nodejs-github-bot nodejs-github-bot added the http Issues or PRs related to the http subsystem. label Apr 12, 2018
@apapirovski
Copy link
Member Author

CI: https://ci.nodejs.org/job/node-test-pull-request/14229/
CitGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1359/

@apapirovski
Copy link
Member Author

/cc @addaleax @lpinca @jasnell @nodejs/http @nodejs/tsc

@apapirovski apapirovski mentioned this pull request Apr 12, 2018
Closed
lpinca
lpinca approved these changes Apr 12, 2018
View reviewed changes
Copy link
Member

@lpinca lpinca left a comment

Choose a reason for hiding this comment

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

SGTM

@lpinca
Copy link
Member

lpinca commented Apr 12, 2018 •
edited
Loading

Can you please run CIGTM?
Edit: Done above ignore me.

mcollina
mcollina approved these changes Apr 12, 2018
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

LGTM with some minor nits.

lib/_http_server.js
@@ -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

lib/_http_client.js Outdated
@@ -492,6 +492,9 @@ function parserOnIncomingClient(res, shouldKeepAlive) {
}
req.res = res;

if (res.upgrade)
return 2;
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 please add the comment:

// Skip body and treat as Upgrade.

It was there in _http_common.js

@apapirovski apapirovski added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 13, 2018
BridgeAR
BridgeAR reviewed Apr 15, 2018
View reviewed changes
test/parallel/test-http-upgrade-server.js Outdated
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));
Copy link
Member

Choose a reason for hiding this comment

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

Nit: please switch the arguments order to adhere to actual, expected.

@BridgeAR
Copy link
Member

CI before landing https://ci.nodejs.org/job/node-test-pull-request/14320/

@mcollina
Copy link
Member

Failure on CI is unrelated, landing.

@mcollina
Copy link
Member

mcollina commented Apr 16, 2018 •
edited
Loading

Landed in f7fbbee

@mcollina mcollina closed this Apr 16, 2018
@mcollina
Copy link
Member

@jasnell can you have a look at adding this to 10?

@apapirovski apapirovski deleted the fix-http-upgrade branch April 16, 2018 09:03
mcollina pushed a commit that referenced this pull request Apr 16, 2018
@apapirovski @mcollina
http: relax requirements on upgrade listener
f7fbbee 
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>
jasnell pushed a commit that referenced this pull request Apr 16, 2018
@apapirovski @jasnell
http: relax requirements on upgrade listener
33ce9a6 
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>
@jancurn jancurn mentioned this pull request Sep 7, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BridgeAR BridgeAR BridgeAR approved these changes

@lpinca lpinca lpinca approved these changes

@jasnell jasnell jasnell approved these changes

@mcollina mcollina mcollina approved these changes

@trivikr trivikr trivikr approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. http Issues or PRs related to the http subsystem. semver-major PRs that contain breaking changes and should be released in the next major version.
Projects
None yet
Milestone
10.0.0
Development

Successfully merging this pull request may close these issues.

node servers should handle upgrade headers gracefully
7 participants
@apapirovski @lpinca @BridgeAR @mcollina @jasnell @trivikr @nodejs-github-bot