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

http: do not emit upgrade on advertisement #4337

Closed
wants to merge 3 commits into from

Conversation

indutny
Copy link
Member

@indutny indutny commented Dec 17, 2015

Do not emit upgrade if the server is just advertising its protocols
support as per RFC 7230 Section 6.7.

A server MAY send an Upgrade header field in any other response
to advertise that it implements support for upgrading to the
listed protocols, in order of descending preference, when
appropriate for a future request.

Fix: #4334

cc @nodejs/http
R?=@mscdex or @bnoordhuis

Do not emit `upgrade` if the server is just advertising its protocols
support as per RFC 7230 Section 6.7.

    A server MAY send an Upgrade header field in any other response
    to advertise that it implements support for upgrading to the
    listed protocols, in order of descending preference, when
    appropriate for a future request.

Fix: nodejs#4334
@indutny
Copy link
Member Author

indutny commented Dec 17, 2015

@Fishrock123 Fishrock123 added the http Issues or PRs related to the http subsystem. label Dec 17, 2015
@Fishrock123
Copy link
Contributor

This probably needs to be backported to 0.10 and 0.12 if possible.

@indutny
Copy link
Member Author

indutny commented Dec 17, 2015

@Fishrock123 we should we very careful with this. As you have probably already seen in the tests - there is some behavior change, and we should be sure that this is necessary.

@jasnell
Copy link
Member

jasnell commented Dec 17, 2015

Hmm.. yeah we'll want to test that out. Use of Upgrade has been rare up until HTTP/2 so we're likely safe, but there could be some folks out there making undocumented and unknown uses that could be affected.

One question.. is this enough of a change of behavior to warrant a semver-minor or major? The old behavior may have been against spec, but it technically was working as written -- i.e. it wasn't a bug.

@indutny
Copy link
Member Author

indutny commented Dec 17, 2015

@jasnell I wouldn't do semver-minor for this, either patch or major. If we will decide to not backport it - let's call it major.

@jasnell
Copy link
Member

jasnell commented Dec 17, 2015

As much as I'd prefer to have this in v4 and earlier, I'm not sure I'm comfortable with doing so... currently leaning towards major then.

@dougwilson
Copy link
Member

As much as I'd prefer to have this in v4 and earlier, I'm not sure I'm comfortable with doing so... currently leaning towards major then.

I second that.

@indutny indutny added semver-major PRs that contain breaking changes and should be released in the next major version. and removed lts-watch-v0.10 labels Dec 18, 2015
@indutny
Copy link
Member Author

indutny commented Dec 18, 2015

Let it be it then!

if (upgrade &&
parser.outgoing !== null &&
(parser.outgoing._headers.upgrade === undefined ||
!/(^|\W)upgrade(\W|$)/i.test(parser.outgoing._headers.connection))) {
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 add a comment explaining that the regex is supposed to match Connection: headers containing words besides just the string 'upgrade'?

(At least, that's what I infer it does.)

'Upgrade: h2c\r\n\r\n' +
'ohai');
}).listen(common.PORT, function() {
fire();
Copy link
Member

Choose a reason for hiding this comment

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

Nit: you could just pass fire() directly.

@bnoordhuis
Copy link
Member

LGTM with two unimportant nits.

@indutny
Copy link
Member Author

indutny commented Dec 18, 2015

@bnoordhuis thank you!

@indutny
Copy link
Member Author

indutny commented Dec 18, 2015

Landed in 32ac376, thank you!

indutny added a commit that referenced this pull request Dec 18, 2015
Do not emit `upgrade` if the server is just advertising its protocols
support as per RFC 7230 Section 6.7.

    A server MAY send an Upgrade header field in any other response
    to advertise that it implements support for upgrading to the
    listed protocols, in order of descending preference, when
    appropriate for a future request.

Fix: #4334
PR-URL: #4337
Reviewed-By: Ben Noordhuis <[email protected]>
@mscdex mscdex closed this Dec 18, 2015
@umutm
Copy link

umutm commented Jan 20, 2016

When can we expect this feature to be added to the core?

@Trott
Copy link
Member

Trott commented Jan 20, 2016

@umutm It's tagged semver-major so it won't be available in a release until Node 6 which will be out in April. (See the graphic at the bottom of https://github.com/nodejs/LTS/.)

@MylesBorins
Copy link
Contributor

@silverwind good catch. Glad you saw that. Backing it out of staging

rvagg added a commit that referenced this pull request Feb 21, 2016
* buffer:
  - You can now supply an encoding argument when filling a
    Buffer Buffer#fill(string[, start[, end]][, encoding]), supplying
    an existing Buffer will also work with
    Buffer#fill(buffer[, start[, end]]). See the API documentation for
    details on how this works. (Trevor Norris) #4935
  - Buffer#indexOf() no longer requires a byteOffset argument if you
    also wish to specify an encoding:
    Buffer#indexOf(val[, byteOffset][, encoding]).
    (Trevor Norris) #4803
* child_process: spawn() and spawnSync() now support a 'shell' option
  to allow for optional execution of the given command inside a shell.
  If set to true, cmd.exe will be used on Windows and /bin/sh
  elsewhere. A path to a custom shell can also be passed to override
  these defaults. On Windows, this option allows .bat. and .cmd files
  to be executed with spawn() and spawnSync(). (Colin Ihrig) #4598
* http_parser: Update to http-parser 2.6.2 to fix an unintentionally
  strict limitation of allowable header characters.
  (James M Snell) #5237
* dgram: socket.send() now supports accepts an array of Buffers or
  Strings as the first argument. See the API docs for details on how
  this works. (Matteo Collina) #4374
* http: Fix a bug where handling headers will mistakenly trigger an
  'upgrade' event where the server is just advertising its protocols.
  This bug can prevent HTTP clients from communicating with HTTP/2
  enabled servers. (Fedor Indutny) #4337
* net: Added a listening Boolean property to net and http servers to
  indicate whether the server is listening for connections.
  (José Moreira) #4743
* node: The C++ node::MakeCallback() API is now reentrant and calling
  it from inside another MakeCallback() call no longer causes the
  nextTick queue or Promises microtask queue to be processed out of
  order. (Trevor Norris) #4507
* tls: Add a new tlsSocket.getProtocol() method to get the negotiated
  TLS protocol version of the current connection. (Brian White) #4995
* vm: Introduce new 'produceCachedData' and 'cachedData' options to
  new vm.Script() to interact with V8's code cache. When a new
  vm.Script object is created with the 'produceCachedData' set to true
  a Buffer with V8's code cache data will be produced and stored in
  cachedData property of the returned object. This data in turn may be
  supplied back to another vm.Script() object with a 'cachedData'
  option if the supplied source is the same. Successfully executing a
  script from cached data can speed up instantiation time. See the API
  docs for details. (Fedor Indutny) #4777
* performance: Improvements in:
  - process.nextTick() (Ruben Bridgewater) #5092
  - path module (Brian White) #5123
  - querystring module (Brian White) #5012
  - streams module when processing small chunks (Matteo Collina) #4354
rvagg added a commit that referenced this pull request Feb 21, 2016
* buffer:
  - You can now supply an encoding argument when filling a
    Buffer Buffer#fill(string[, start[, end]][, encoding]), supplying
    an existing Buffer will also work with
    Buffer#fill(buffer[, start[, end]]). See the API documentation for
    details on how this works. (Trevor Norris) #4935
  - Buffer#indexOf() no longer requires a byteOffset argument if you
    also wish to specify an encoding:
    Buffer#indexOf(val[, byteOffset][, encoding]).
    (Trevor Norris) #4803
* child_process: spawn() and spawnSync() now support a 'shell' option
  to allow for optional execution of the given command inside a shell.
  If set to true, cmd.exe will be used on Windows and /bin/sh
  elsewhere. A path to a custom shell can also be passed to override
  these defaults. On Windows, this option allows .bat. and .cmd files
  to be executed with spawn() and spawnSync(). (Colin Ihrig) #4598
* http_parser: Update to http-parser 2.6.2 to fix an unintentionally
  strict limitation of allowable header characters.
  (James M Snell) #5237
* dgram: socket.send() now supports accepts an array of Buffers or
  Strings as the first argument. See the API docs for details on how
  this works. (Matteo Collina) #4374
* http: Fix a bug where handling headers will mistakenly trigger an
  'upgrade' event where the server is just advertising its protocols.
  This bug can prevent HTTP clients from communicating with HTTP/2
  enabled servers. (Fedor Indutny) #4337
* net: Added a listening Boolean property to net and http servers to
  indicate whether the server is listening for connections.
  (José Moreira) #4743
* node: The C++ node::MakeCallback() API is now reentrant and calling
  it from inside another MakeCallback() call no longer causes the
  nextTick queue or Promises microtask queue to be processed out of
  order. (Trevor Norris) #4507
* tls: Add a new tlsSocket.getProtocol() method to get the negotiated
  TLS protocol version of the current connection. (Brian White) #4995
* vm: Introduce new 'produceCachedData' and 'cachedData' options to
  new vm.Script() to interact with V8's code cache. When a new
  vm.Script object is created with the 'produceCachedData' set to true
  a Buffer with V8's code cache data will be produced and stored in
  cachedData property of the returned object. This data in turn may be
  supplied back to another vm.Script() object with a 'cachedData'
  option if the supplied source is the same. Successfully executing a
  script from cached data can speed up instantiation time. See the API
  docs for details. (Fedor Indutny) #4777
* performance: Improvements in:
  - process.nextTick() (Ruben Bridgewater) #5092
  - path module (Brian White) #5123
  - querystring module (Brian White) #5012
  - streams module when processing small chunks (Matteo Collina) #4354
rvagg added a commit that referenced this pull request Feb 23, 2016
* buffer:
  - You can now supply an encoding argument when filling a
    Buffer Buffer#fill(string[, start[, end]][, encoding]), supplying
    an existing Buffer will also work with
    Buffer#fill(buffer[, start[, end]]). See the API documentation for
    details on how this works. (Trevor Norris) #4935
  - Buffer#indexOf() no longer requires a byteOffset argument if you
    also wish to specify an encoding:
    Buffer#indexOf(val[, byteOffset][, encoding]).
    (Trevor Norris) #4803
* child_process: spawn() and spawnSync() now support a 'shell' option
  to allow for optional execution of the given command inside a shell.
  If set to true, cmd.exe will be used on Windows and /bin/sh
  elsewhere. A path to a custom shell can also be passed to override
  these defaults. On Windows, this option allows .bat. and .cmd files
  to be executed with spawn() and spawnSync(). (Colin Ihrig) #4598
* http_parser: Update to http-parser 2.6.2 to fix an unintentionally
  strict limitation of allowable header characters.
  (James M Snell) #5237
* dgram: socket.send() now supports accepts an array of Buffers or
  Strings as the first argument. See the API docs for details on how
  this works. (Matteo Collina) #4374
* http: Fix a bug where handling headers will mistakenly trigger an
  'upgrade' event where the server is just advertising its protocols.
  This bug can prevent HTTP clients from communicating with HTTP/2
  enabled servers. (Fedor Indutny) #4337
* net: Added a listening Boolean property to net and http servers to
  indicate whether the server is listening for connections.
  (José Moreira) #4743
* node: The C++ node::MakeCallback() API is now reentrant and calling
  it from inside another MakeCallback() call no longer causes the
  nextTick queue or Promises microtask queue to be processed out of
  order. (Trevor Norris) #4507
* tls: Add a new tlsSocket.getProtocol() method to get the negotiated
  TLS protocol version of the current connection. (Brian White) #4995
* vm: Introduce new 'produceCachedData' and 'cachedData' options to
  new vm.Script() to interact with V8's code cache. When a new
  vm.Script object is created with the 'produceCachedData' set to true
  a Buffer with V8's code cache data will be produced and stored in
  cachedData property of the returned object. This data in turn may be
  supplied back to another vm.Script() object with a 'cachedData'
  option if the supplied source is the same. Successfully executing a
  script from cached data can speed up instantiation time. See the API
  docs for details. (Fedor Indutny) #4777
* performance: Improvements in:
  - process.nextTick() (Ruben Bridgewater) #5092
  - path module (Brian White) #5123
  - querystring module (Brian White) #5012
  - streams module when processing small chunks (Matteo Collina) #4354

PR-URL: #5295
rvagg added a commit that referenced this pull request Feb 24, 2016
* buffer:
  - You can now supply an encoding argument when filling a
    Buffer Buffer#fill(string[, start[, end]][, encoding]), supplying
    an existing Buffer will also work with
    Buffer#fill(buffer[, start[, end]]). See the API documentation for
    details on how this works. (Trevor Norris) #4935
  - Buffer#indexOf() no longer requires a byteOffset argument if you
    also wish to specify an encoding:
    Buffer#indexOf(val[, byteOffset][, encoding]).
    (Trevor Norris) #4803
* child_process: spawn() and spawnSync() now support a 'shell' option
  to allow for optional execution of the given command inside a shell.
  If set to true, cmd.exe will be used on Windows and /bin/sh
  elsewhere. A path to a custom shell can also be passed to override
  these defaults. On Windows, this option allows .bat. and .cmd files
  to be executed with spawn() and spawnSync(). (Colin Ihrig) #4598
* http_parser: Update to http-parser 2.6.2 to fix an unintentionally
  strict limitation of allowable header characters.
  (James M Snell) #5237
* dgram: socket.send() now supports accepts an array of Buffers or
  Strings as the first argument. See the API docs for details on how
  this works. (Matteo Collina) #4374
* http: Fix a bug where handling headers will mistakenly trigger an
  'upgrade' event where the server is just advertising its protocols.
  This bug can prevent HTTP clients from communicating with HTTP/2
  enabled servers. (Fedor Indutny) #4337
* net: Added a listening Boolean property to net and http servers to
  indicate whether the server is listening for connections.
  (José Moreira) #4743
* node: The C++ node::MakeCallback() API is now reentrant and calling
  it from inside another MakeCallback() call no longer causes the
  nextTick queue or Promises microtask queue to be processed out of
  order. (Trevor Norris) #4507
* tls: Add a new tlsSocket.getProtocol() method to get the negotiated
  TLS protocol version of the current connection. (Brian White) #4995
* vm: Introduce new 'produceCachedData' and 'cachedData' options to
  new vm.Script() to interact with V8's code cache. When a new
  vm.Script object is created with the 'produceCachedData' set to true
  a Buffer with V8's code cache data will be produced and stored in
  cachedData property of the returned object. This data in turn may be
  supplied back to another vm.Script() object with a 'cachedData'
  option if the supplied source is the same. Successfully executing a
  script from cached data can speed up instantiation time. See the API
  docs for details. (Fedor Indutny) #4777
* performance: Improvements in:
  - process.nextTick() (Ruben Bridgewater) #5092
  - path module (Brian White) #5123
  - querystring module (Brian White) #5012
  - streams module when processing small chunks (Matteo Collina) #4354

PR-URL: #5295
MylesBorins pushed a commit to MylesBorins/node that referenced this pull request Mar 1, 2016
Do not emit `upgrade` if the server is just advertising its protocols
support as per RFC 7230 Section 6.7.

    A server MAY send an Upgrade header field in any other response
    to advertise that it implements support for upgrading to the
    listed protocols, in order of descending preference, when
    appropriate for a future request.

Fix: nodejs#4334
PR-URL: nodejs#4337
Reviewed-By: Ben Noordhuis <[email protected]>
MylesBorins pushed a commit that referenced this pull request Mar 1, 2016
Do not emit `upgrade` if the server is just advertising its protocols
support as per RFC 7230 Section 6.7.

    A server MAY send an Upgrade header field in any other response
    to advertise that it implements support for upgrading to the
    listed protocols, in order of descending preference, when
    appropriate for a future request.

Fix: #4334
PR-URL: #4337
Reviewed-By: Ben Noordhuis <[email protected]>
MylesBorins pushed a commit that referenced this pull request Mar 2, 2016
Do not emit `upgrade` if the server is just advertising its protocols
support as per RFC 7230 Section 6.7.

    A server MAY send an Upgrade header field in any other response
    to advertise that it implements support for upgrading to the
    listed protocols, in order of descending preference, when
    appropriate for a future request.

Fix: #4334
PR-URL: #4337
Reviewed-By: Ben Noordhuis <[email protected]>
scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016
Do not emit `upgrade` if the server is just advertising its protocols
support as per RFC 7230 Section 6.7.

    A server MAY send an Upgrade header field in any other response
    to advertise that it implements support for upgrading to the
    listed protocols, in order of descending preference, when
    appropriate for a future request.

Fix: nodejs#4334
PR-URL: nodejs#4337
Reviewed-By: Ben Noordhuis <[email protected]>
@umutm
Copy link

umutm commented Apr 10, 2016

Any chance that this fix will be added to v0.10 to make the older Nodejsversions compatible with HTTP/2?

@bnoordhuis
Copy link
Member

@umutm I don't know if any collaborators are interested in back-porting it but you're welcome to give it a go; the v0.12-staging and v0.10-staging branches are what you want to target.

@umutm
Copy link

umutm commented Apr 11, 2016

@bnoordhuis Thanks very much for the update.

Although I'd love to, that is over my talent and had wondered if there was any official decision to make older versions HTTP\2 compatible. Thanks very much again.

@MylesBorins
Copy link
Contributor

afaik 0.10 and 0.12 are not going to be getting any non security upgrades.

/cc @nodejs/lts

@rvagg
Copy link
Member

rvagg commented Apr 11, 2016

0.12 entered Maintenance this month and joins 0.10, the bar is pretty high for getting changes in to those branches however given the importance of this change to supporting modern servers I think we might be at least open to have a discussion about if if someone were to put pull requests to those staging branches.

Your best bet is to migrate to v4+ tbh, there's so much additional work that's gone in to making v4 a really solid platform.

@indutny
Copy link
Member Author

indutny commented Apr 11, 2016

Please also note that this is a breaking change, some programs that work right now on v0.10/v0.12 may become broken after backporting it. Can we afford this?

@umutm
Copy link

umutm commented Apr 11, 2016

Also, strange enough, there is not much demand for this. It is either "big hosting companies didn't update their Apache versions yet so that HTTP requests work fine for many" or the majority uses v4+.

We have performed a switch to v4, experiencing an issue there (couldn't clarify yet) and wanted to double check if the v0.1+ would get this feature. An totally understand the difficulty of maintaining/updating such older versions.

Thanks you very much again for all the help.

@Fishrock123
Copy link
Contributor

wanted to double check if the v0.1+ would get this feature.

@umutm Probably not very immediately at the least, if at all.

If you need help upgrading to v4 we have a full list of breaking changes v0.10 -> v4.
There are also tools such as upgrade-ready that can help find outdated modules.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
http Issues or PRs related to the http subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.