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: skip body and next message of CONNECT res #6279

Closed
wants to merge 3 commits into from
Closed

http: skip body and next message of CONNECT res #6279

wants to merge 3 commits into from

Conversation

indutny
Copy link
Member

@indutny indutny commented Apr 19, 2016

Checklist
  • tests and code linting passes
  • a test and/or benchmark is included
  • the commit message follows commit guidelines
Affected core subsystem(s)

http

Description of change
http: skip body and next message of CONNECT res

When handling a response to `CONNECT` request - skip message body
and do not attempt to parse the next message. `CONNECT` requests are
used in similar sense to HTTP Upgrade.

Fix: https://github.com/nodejs/node/pull/6198

cc @nodejs/http @mscdex @bnoordhuis

indutny and others added 3 commits April 19, 2016 10:47
@indutny
deps: update to http-parser 2.7.0
aebdf6f 
Adds `2` as a return value of `on_headers_complete`, this mode will be
used to fix handling responses to `CONNECT` requests.

See: nodejs#6198
@indutny
http: skip body and next message of CONNECT res
e411692 
When handling a response to `CONNECT` request - skip message body
and do not attempt to parse the next message. `CONNECT` requests are
used in similar sense to HTTP Upgrade.

Fix: nodejs#6198
@slushie @indutny
test: add test for responses to HTTP CONNECT req
3b07864 
See: nodejs#6198
@indutny indutny added http Issues or PRs related to the http subsystem. lts-watch-v4.x labels Apr 19, 2016
@indutny
Copy link
Member Author

indutny commented Apr 19, 2016

cc @nodejs/lts for discussion of backporting http-parser update to v4

@indutny
Copy link
Member Author

indutny commented Apr 19, 2016

CI: https://ci.nodejs.org/job/node-test-pull-request/2320/

@indutny indutny changed the title Fix/gh 6198 http: skip body and next message of CONNECT res Apr 19, 2016
@mscdex
Copy link
Contributor

mscdex commented Apr 19, 2016

Last CI failed because the certify box wasn't checked.

New CI: https://ci.nodejs.org/job/node-test-pull-request/2321/

@indutny
Copy link
Member Author

indutny commented Apr 19, 2016

Oops, my bad.

indutny
indutny reviewed Apr 19, 2016
View reviewed changes
lib/_http_common.js
@@ -105,7 +105,10 @@ function parserOnHeadersComplete(versionMajor, versionMinor, headers, method,
skipBody = parser.onIncoming(parser.incoming, shouldKeepAlive);
}

return skipBody;
if (typeof skipBody !== 'number')
Copy link
Member Author

Choose a reason for hiding this comment

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

If anyone is curious - this is for compatibility with external code. (like node-spdy)

@mscdex
Copy link
Contributor

mscdex commented Apr 19, 2016

CI is green. LGTM

@jasnell
Copy link
Member

jasnell commented Apr 19, 2016

LGTM

@indutny
Copy link
Member Author

indutny commented Apr 19, 2016

Landed in 7f14483, 7f14483, and 0ecc430. thank you!

indutny added a commit that referenced this pull request Apr 19, 2016
@indutny
deps: update to http-parser 2.7.0
7f14483 
Adds `2` as a return value of `on_headers_complete`, this mode will be
used to fix handling responses to `CONNECT` requests.

See: #6198
PR-URL: #6279
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
indutny added a commit that referenced this pull request Apr 19, 2016
@indutny
http: skip body and next message of CONNECT res
9d4d529 
When handling a response to `CONNECT` request - skip message body
and do not attempt to parse the next message. `CONNECT` requests are
used in similar sense to HTTP Upgrade.

Fix: #6198
PR-URL: #6279
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
indutny pushed a commit that referenced this pull request Apr 19, 2016
@slushie @indutny
test: add test for responses to HTTP CONNECT req
0ecc430 
See: #6198
PR-URL: #6279
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
@jasnell jasnell closed this Apr 19, 2016
MylesBorins pushed a commit that referenced this pull request Apr 20, 2016
@indutny
deps: update to http-parser 2.7.0
5305831 
Adds `2` as a return value of `on_headers_complete`, this mode will be
used to fix handling responses to `CONNECT` requests.

See: #6198
PR-URL: #6279
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Apr 20, 2016
@indutny
http: skip body and next message of CONNECT res
16b23b2 
When handling a response to `CONNECT` request - skip message body
and do not attempt to parse the next message. `CONNECT` requests are
used in similar sense to HTTP Upgrade.

Fix: #6198
PR-URL: #6279
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Apr 20, 2016
@slushie
test: add test for responses to HTTP CONNECT req
4e4efb7 
See: #6198
PR-URL: #6279
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Apr 21, 2016
2016-04-20, Version 5.11.0 (Stable) Release
e43735b 
Buffer:
  * Buffer.prototype.compare can now compare sub-ranges of two Buffers
   (James M Snell) #5880

deps:
  * update to http-parser 2.7.0
    (Fedor Indutny) #6279
  * update ESLint to 2.7.0
    (silverwind) #6132

net:
  * adds support for passing DNS lookup hints to createConnection()
    (Colin Ihrig) #6000

node:
  * Make the builtin libraries available for the --eval and --print
    CLI options
    (Anna Henningsen) #6207

npm:
  * upgrade npm to 3.8.6
    (Kat Marchán) #6153

repl:
  * Pressing enter in the repl will repeat the last command by default
    if no input has been received. This behaviour was in node
    previously and was not removed intentionally.
    (Rich Trott) #6090

src:
  * add SIGINFO to supported signals
    (James Reggio) #6093

streams:
  * Fix a regression that caused by net streams requesting multiple
    chunks synchronously when combined with cork/uncork
    (Matteo Collina) #6164

zlib:
  * The flushing flag is now configurable allowing for decompression
    of partial data
    (Anna Henningsen) #6069
This was referenced Apr 21, 2016
Closed
Closed
MylesBorins pushed a commit that referenced this pull request Apr 21, 2016
@indutny @MylesBorins
deps: update to http-parser 2.7.0
b7dd451 
Adds `2` as a return value of `on_headers_complete`, this mode will be
used to fix handling responses to `CONNECT` requests.

See: #6198
PR-URL: #6279
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Apr 21, 2016
@indutny @MylesBorins
http: skip body and next message of CONNECT res
f6969a1 
When handling a response to `CONNECT` request - skip message body
and do not attempt to parse the next message. `CONNECT` requests are
used in similar sense to HTTP Upgrade.

Fix: #6198
PR-URL: #6279
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Apr 21, 2016
@slushie @MylesBorins
test: add test for responses to HTTP CONNECT req
1ef18bd 
See: #6198
PR-URL: #6279
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Apr 21, 2016
@MylesBorins
2016-04-21, Version 5.11.0 (Stable) Release
8dc1d5e 
Buffer:
  * Buffer.prototype.compare can now compare sub-ranges of two Buffers
   (James M Snell) #5880

deps:
  * update to http-parser 2.7.0
    (Fedor Indutny) #6279
  * update ESLint to 2.7.0
    (silverwind) #6132

net:
  * adds support for passing DNS lookup hints to createConnection()
    (Colin Ihrig) #6000

node:
  * Make the builtin libraries available for the --eval and --print
    CLI options
    (Anna Henningsen) #6207

npm:
  * upgrade npm to 3.8.6
    (Kat Marchán) #6153

repl:
  * Pressing enter in the repl will repeat the last command by default
    if no input has been received. This behavior was in node
    previously and was not removed intentionally.
    (Rich Trott) #6090

src:
  * add SIGINFO to supported signals
    (James Reggio) #6093

streams:
  * Fix a regression that caused by net streams requesting multiple
    chunks synchronously when combined with cork/uncork
    (Matteo Collina) #6164

zlib:
  * The flushing flag is now configurable allowing for decompression
    of partial data
    (Anna Henningsen) #6069
@indutny
Copy link
Member Author

indutny commented Jun 1, 2016

@thealphanerd yep.

@indutny indutny deleted the fix/gh-6198 branch June 1, 2016 22:49
@MylesBorins
Copy link
Contributor

@indutny it looks like this uses a different http parser under the hood and is not merging cleanly... can you take a look at manually backporting?

@indutny
Copy link
Member Author

indutny commented Jun 8, 2016

@thealphanerd sorry for delay.

@indutny
Copy link
Member Author

indutny commented Jun 8, 2016

@thealphanerd Looking at this now.

@indutny indutny mentioned this pull request Jun 8, 2016
Closed
@indutny
Copy link
Member Author

indutny commented Jun 8, 2016

@thealphanerd done #7213

indutny added a commit to indutny/io.js that referenced this pull request Jun 28, 2016
@indutny
deps: update to http-parser 2.7.0
e2faad3 
Adds `2` as a return value of `on_headers_complete`, this mode will be
used to fix handling responses to `CONNECT` requests.

See: nodejs#6198
PR-URL: nodejs#6279
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
indutny added a commit to indutny/io.js that referenced this pull request Jun 28, 2016
@indutny
http: skip body and next message of CONNECT res
bfd1e18 
When handling a response to `CONNECT` request - skip message body
and do not attempt to parse the next message. `CONNECT` requests are
used in similar sense to HTTP Upgrade.

Fix: nodejs#6198
PR-URL: nodejs#6279
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
indutny pushed a commit to indutny/io.js that referenced this pull request Jun 28, 2016
@slushie @indutny
test: add test for responses to HTTP CONNECT req
f841ea9 
See: nodejs#6198
PR-URL: nodejs#6279
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit to MylesBorins/node that referenced this pull request Jul 5, 2016
@indutny
deps: update to http-parser 2.7.0
73a0e18 
Adds `2` as a return value of `on_headers_complete`, this mode will be
used to fix handling responses to `CONNECT` requests.

See: nodejs#6198
PR-URL: nodejs#6279
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit to MylesBorins/node that referenced this pull request Jul 5, 2016
@indutny
http: skip body and next message of CONNECT res
5503567 
When handling a response to `CONNECT` request - skip message body
and do not attempt to parse the next message. `CONNECT` requests are
used in similar sense to HTTP Upgrade.

Fix: nodejs#6198
PR-URL: nodejs#6279
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit to MylesBorins/node that referenced this pull request Jul 5, 2016
@slushie
test: add test for responses to HTTP CONNECT req
8fa2d07 
See: nodejs#6198
PR-URL: nodejs#6279
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jul 11, 2016
@indutny
deps: update to http-parser 2.7.0
36a7034 
Adds `2` as a return value of `on_headers_complete`, this mode will be
used to fix handling responses to `CONNECT` requests.

See: #6198
PR-URL: #6279
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jul 11, 2016
@indutny
http: skip body and next message of CONNECT res
a5d5d02 
When handling a response to `CONNECT` request - skip message body
and do not attempt to parse the next message. `CONNECT` requests are
used in similar sense to HTTP Upgrade.

Fix: #6198
PR-URL: #6279
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jul 11, 2016
@slushie
test: add test for responses to HTTP CONNECT req
fb88542 
See: #6198
PR-URL: #6279
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jul 12, 2016
@indutny
deps: update to http-parser 2.7.0
e06ab64 
Adds `2` as a return value of `on_headers_complete`, this mode will be
used to fix handling responses to `CONNECT` requests.

See: #6198
PR-URL: #6279
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jul 12, 2016
@indutny
http: skip body and next message of CONNECT res
07fd52e 
When handling a response to `CONNECT` request - skip message body
and do not attempt to parse the next message. `CONNECT` requests are
used in similar sense to HTTP Upgrade.

Fix: #6198
PR-URL: #6279
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jul 12, 2016
@slushie
test: add test for responses to HTTP CONNECT req
55f8689 
See: #6198
PR-URL: #6279
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Jul 12, 2016
Merged
@bnoordhuis bnoordhuis mentioned this pull request Jul 26, 2016
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
http Issues or PRs related to the http subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@indutny @mscdex @jasnell @MylesBorins @slushie