Skip to content

http: do not emit upgrade on advertisement #4337

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

Closed
indutny wants to merge 3 commits into from
Closed

http: do not emit upgrade on advertisement #4337

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
01fc2e9
http: do not emit `upgrade` on advertisement
indutny Dec 17, 2015
f64bf32
add comment
indutny Dec 18, 2015
a48ed32
test
indutny Dec 18, 2015
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
Prev Previous commit
Next Next commit
add comment
  • Loading branch information
@indutny
indutny committed Dec 18, 2015
commit f64bf3282115163def1b7a2a495a3e43ff23204b
3 changes: 3 additions & 0 deletions lib/_http_common.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,9 @@ function parserOnHeadersComplete(versionMajor, versionMinor, headers, method,
// supported protocols.
//
// See RFC7230 Section 6.7
//
// NOTE: RegExp below matches `upgrade` in `Connection: abc, upgrade, def`
// header.
if (upgrade &&
Copy link
Member

@rvagg rvagg Jan 28, 2016

Choose a reason for hiding this comment

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

seems like a kludge that we are doing this here, isn't this more appropriate for http_parser if it's getting it wrong?

Copy link
Contributor

@silverwind silverwind Jan 28, 2016

Choose a reason for hiding this comment

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

I'm not sure. Is http-parser aware of the outgoing request? If not, I think the fix here is valid.

Copy link
Member Author

@indutny indutny Jan 28, 2016

Choose a reason for hiding this comment

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

@silverwind has valid point

Copy link
Contributor

@silverwind silverwind Jan 29, 2016

Choose a reason for hiding this comment

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

@indutny what do you think about only emitting 'upgrade' if both headers are present in the response? That way the decision could be based purely on the response and could be done in http-parser. All three use cases of the upgrade header require both headers to be present in both the request and the response.

Copy link
Member Author

@indutny indutny Jan 29, 2016

Choose a reason for hiding this comment

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

Wait, how does http-parser know about response headers? Also, I think I require both headers now, am I not?

Copy link
Contributor

@silverwind silverwind Jan 29, 2016

Choose a reason for hiding this comment

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

Isn't it http-parser which sets the upgrade argument based on the response header here?

And yeah, you require both header on the request. I still think the upgrade decision could be solely based on the response headers.

Copy link
Contributor

@silverwind silverwind Jan 29, 2016

Choose a reason for hiding this comment

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

Or rather, scrap that. I re-read that rfc7230 part, and the advertisement can include both headers, so knowledge of request and response is necessary.

Copy link
Member Author

@indutny indutny Jan 29, 2016

Choose a reason for hiding this comment

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

Of course 😉

parser.outgoing !== null &&
(parser.outgoing._headers.upgrade === undefined ||
Expand Down