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

[8.x] Backport http2 changes from 10.x #22850

Closed
wants to merge 67 commits into from
Closed

[8.x] Backport http2 changes from 10.x #22850

wants to merge 67 commits into from

Conversation

kjin
Copy link
Contributor

@kjin kjin commented Sep 13, 2018 •
edited

This PR tracks an effort to backport new HTTP/2 features from 10.x. It's a work in progress, so this PR is currently more of an FYI and shouldn't really be reviewed (except if it's clear that a commit doesn't really belong).

Note that I have some intermediate commits that currently skip some tests.

Edit: See my comments below.

cc/ @ofrobots @apapirovski @nodejs/http2

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

@nodejs-github-bot nodejs-github-bot added lib / src Issues and PRs related to general changes in the lib or src directory. v8.x labels Sep 13, 2018
@mcollina mcollina added the semver-minor PRs that contain new features and should be released in the next minor version. label Sep 14, 2018
@mcollina
Copy link
Member

Good work! I've tagged this as semver-minor because it adds a new feature to http as well.

@jasnell
Copy link
Member

jasnell commented Sep 14, 2018

Very nice! Thank you for taking this on

@addaleax
Copy link
Member

It actually looks like this already needs a rebase? :/

@kjin
Copy link
Contributor Author

kjin commented Sep 15, 2018

@addaleax Yep, I will be doing the rebase in a bit. Note that the commits as of right now represent maybe 50% of the commits that will be part of this PR... more to come.

@addaleax addaleax added the wip Issues and PRs that are still a work in progress. label Sep 15, 2018
@addaleax addaleax mentioned this pull request Sep 18, 2018
Closed
@kjin
Copy link
Contributor Author

kjin commented Sep 19, 2018 •
edited

Update: I've backported commits that have to do with HTTP/2 all the way up to August 15. Based on how many come afterward I'm thinking of just opening a new PR for remaining commits.

I created list of commits based on any commit the all matched the following criteria:

  • Has not been backported to 8.x before, and associated PR does not have the do-not-backport-v8.x label
  • Is on the branch v10.x-staging (as of August 15)
  • Satisfies at least one of the following:
    • Touches files that match at least one of these globs: lib/internal/http2, doc/api/http2.md, test/**/test-http2-*, src/node_http2*
    • Has http2 in the commit name
    • Is from a PR with the http2 label

I removed commits that I believed were too broad to be applied to a PR based around just backporting http2, which resulted in (1) the commits in the PR, and (2) a smaller list of commits that I did not add to this PR but are worth noting. I grouped these commits below based on what I think should be backported together in follow-up PRs:

@addaleax, it seems like you are the author of the majority of these changes, so any advice you might have on how to proceed with these would be appreciated.

@kjin kjin changed the title [WIP] [8.x] Backport http2 changes from 10.x [8.x] Backport http2 changes from 10.x Sep 20, 2018
@kjin
Copy link
Contributor Author

kjin commented Sep 25, 2018

Ping @MylesBorins @addaleax @nodejs/http2 -- Since Node 10 will be going to LTS soon, I want to make sure we have enough time to react to any requested changes/future PRs.

@ofrobots ofrobots removed the wip Issues and PRs that are still a work in progress. label Sep 25, 2018
@MylesBorins
Copy link
Member

@kjin thanks for the hard work! We should plan for another 8.x semver minor to accommodate these changes. It looks like there are semver minor changes outside of http2, but if they scoped only to http2 we would be able to land as a semver-patch (as it is not stable in 8.x)

@kjin
Copy link
Contributor Author

kjin commented Sep 25, 2018 •
edited

@MylesBorins With the exception of adding the ERR_TLS_RENEGOTIATE error, do you know what else would count as semver-minor that is outside of the H2 domain?

@MylesBorins
Copy link
Member

From a very quick look I'm thinking about #20094 which is a semver-minor which affects http.

TBH it probably doesn't make sense to just land those changes on http, we should eat the semver-minor on this

@MylesBorins MylesBorins mentioned this pull request Oct 2, 2018
Closed
@MylesBorins
Copy link
Member

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

@kjin
Copy link
Contributor Author

kjin commented Oct 3, 2018 •
edited

Update #2: I've added commits that were landed between 10.9 and 10.11. There were some in particular that I opted to change/skip, that might need additional attention:

I believe as it is, this PR passes tests. However, there are a few things I want to get confirmation for from these folks:

@addaleax
Copy link
Member

addaleax commented Oct 3, 2018

@kjin It’s okay not to backport 2a88731, yes

@kjin
Copy link
Contributor Author

kjin commented Oct 15, 2018

Ping @nodejs/lts -- I think this is ready to move forward (let me know if you need anything from my end)

@MylesBorins
Copy link
Member

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

@BethGriggs
Copy link
Member

The linter is failing, it seems to be this commit that is failing:

commit 019e127df96e2a53fa5830446a7c206a4eb3d4e1
Author: James M Snell <jasnell@gmail.com>
Date:   Sun Sep 16 19:13:11 2018 -0700

    http2: add origin frame support
    
    v8.x Backport Note -- as V8 doesn't expose an overload of String::WriteOneByte
    in Node 8 that accepts an isolate argument, the Origins constructor has been
    changed to not accept an isolate.
    
    PR-URL: https://github.com/nodejs/node/pull/22956
    Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

jasnell and others added 2 commits October 16, 2018 09:19
@jasnell @kjin
http2: fix ping duration calculation
141f166 
PR-URL: nodejs#19956
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
@indranil @kjin
doc: close event does not take arguments
f3f4ee6 
Explicitly added in the docs that the close event does not expect
any arguments when invoked.

Fixes: nodejs#20018

PR-URL: nodejs#20031
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
BethGriggs pushed a commit that referenced this pull request Oct 17, 2018
@jasnell @BethGriggs
http2: avoid race condition in OnHeaderCallback
5c3edd3 
Fixes: #21416

Backport-PR-URL: #22850
PR-URL: #22256
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: George Adams <george.adams@uk.ibm.com>
BethGriggs pushed a commit that referenced this pull request Oct 17, 2018
@jasnell @BethGriggs
test: add test-http2-large-file sequential test
b66cba0 
Refs: #19141

Backport-PR-URL: #22850
PR-URL: #22254
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: George Adams <george.adams@uk.ibm.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
BethGriggs pushed a commit that referenced this pull request Oct 17, 2018
@jasnell @BethGriggs
http2: explicitly disallow nested push streams
cc561cc 
Fixes: #19095

Backport-PR-URL: #22850
PR-URL: #22245
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
BethGriggs pushed a commit that referenced this pull request Oct 17, 2018
@jasnell @BethGriggs
http2: emit timeout on compat request and response
348cde0 
v8.x Backport Note: The timeout has been increased to 10ms.

Fixes: #20079

Backport-PR-URL: #22850
PR-URL: #22252
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
BethGriggs pushed a commit that referenced this pull request Oct 17, 2018
@BethGriggs
doc: make createPushResponse() more detailled
3fe9293 
We don't know what will return when successful or failure for
the callback of the function. So this commit makes it more detailled.

Backport-PR-URL: #22850
PR-URL: #22366
Refs: #22322
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
BethGriggs pushed a commit that referenced this pull request Oct 17, 2018
@Trott @BethGriggs
doc: simplify http2 wording and formatting
d0be932 
Remove `It is important to note that` and italics from `waitForTrailers`
sentences.

Backport-PR-URL: #22850
PR-URL: #22541
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
BethGriggs pushed a commit that referenced this pull request Oct 17, 2018
@jasnell @BethGriggs
http2: throw better error when accessing unbound socket proxy
6a396ff 
Fixes: #22268

Backport-PR-URL: #22850
PR-URL: #22486
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
BethGriggs pushed a commit that referenced this pull request Oct 17, 2018
@addaleax @BethGriggs
deps: update to nghttp2 1.33.0
60f7bfa 
Refs: https://github.com/nghttp2/nghttp2/releases/tag/v1.33.0

Backport-PR-URL: #22850
PR-URL: #22649
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
BethGriggs pushed a commit that referenced this pull request Oct 17, 2018
@szmarczak @BethGriggs
http2: don't expose the original socket through the socket proxy
805bf40 
Refs: #22486

Backport-PR-URL: #22850
PR-URL: #22650
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
BethGriggs pushed a commit that referenced this pull request Oct 17, 2018
@sagitsofan @BethGriggs
doc: document http2 timeouts
bae7c60 
New default timeout values of "2 minutes" were added into documentation
inside 2 classes under "Event: 'timeout'":
1) Class: Http2SecureServer
2) Class: Http2Server

New sections for `.setTimeout()` method were added inside
`Http2SecureServer` & `Http2Server` docs.

Backport-PR-URL: #22850
PR-URL: #22798
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
BethGriggs pushed a commit that referenced this pull request Oct 17, 2018
@jasnell @BethGriggs
http2: add http2stream.endAfterHeaders property
2de17ea 
Indicates is the END_STREAM flag was set on the received HEADERS frame

Backport-PR-URL: #22850
PR-URL: #22843
Fixes: #22497
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
BethGriggs pushed a commit that referenced this pull request Oct 17, 2018
@mcollina @BethGriggs
http2: check if stream is not destroyed before sending trailers
9f79341 
Fixes: #22855
Backport-PR-URL: #22850
PR-URL: #22896
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
BethGriggs pushed a commit that referenced this pull request Oct 17, 2018
@jasnell @BethGriggs
http2: add origin frame support
49f44f3 
v8.x Backport Note -- as V8 doesn't expose an overload of String::WriteOneByte
in Node 8 that accepts an isolate argument, the Origins constructor has been
changed to not accept an isolate.

Backport-PR-URL: #22850
PR-URL: #22956
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
BethGriggs pushed a commit that referenced this pull request Oct 17, 2018
@apapirovski @BethGriggs
http2: do not falsely emit 'aborted' on push
ca933ce 
A push stream should have its writable side closed upon receipt,
to avoid emitting the 'aborted' event when the readable side
is closed.

Backport-PR-URL: #22850
PR-URL: #22878
Fixes: #22851
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
BethGriggs pushed a commit that referenced this pull request Oct 17, 2018
@jasnell @BethGriggs
http2: add ping event
10576d6 
Add a `Http2Session` event whenever a non-ack `PING` is received.

Fixes: #18514

Backport-PR-URL: #22850
PR-URL: #23009
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
BethGriggs pushed a commit that referenced this pull request Oct 17, 2018
@Trott @BethGriggs
test: remove setImmediate from timeout test
c0f8e49 
In test-http2-session-timeout, setImmediate() is used to wrap makeReq().
makeReq() is asynchronous and setImmediate() is not necessary.

Backport-PR-URL: #22850
PR-URL: #23058
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
BethGriggs pushed a commit that referenced this pull request Oct 17, 2018
@Trott @BethGriggs
test: improve debugging information for http2 test
069fd79 
In test-http2-session-timeout, provide the number of requests that
occurred when the test fails.

Backport-PR-URL: #22850
PR-URL: #23058
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
BethGriggs pushed a commit that referenced this pull request Oct 17, 2018
@jasnell @BethGriggs
http2: graduate from experimental
18a2b3d 
Backport-PR-URL: #22850
PR-URL: #22466
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Yuta Hiroto <hello@hiroppy.me>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@BethGriggs
Copy link
Member

Landed on v8.x-staging in fe3836a...18a2b3d

@BethGriggs BethGriggs closed this Oct 17, 2018
@BethGriggs BethGriggs mentioned this pull request Oct 17, 2018
Closed
2 tasks
@BethGriggs BethGriggs mentioned this pull request Nov 20, 2018
Merged
BethGriggs added a commit that referenced this pull request Nov 20, 2018
@BethGriggs
2018-11-20, Version 8.13.0 'Carbon' (LTS)
20e68dd 
Notable changes:

* **assert**:
  - backport some assert commits (Ruben Bridgewater) [#23223](#23223)
* **deps**:
  - V8: cherry-pick 64-bit hash seed commits (Yang Guo) [#23274](#23274)
* **http**:
  - added aborted property to request (Robert Nagy) [#20094](#20094)
* **http2**:
  - backport http2 changes from 10.x (Kelvin Jin) [#22850](#22850)

PR-URL: #23974
@Flarna
Copy link
Member

Flarna commented Nov 21, 2018

Not sure if this is the right place to post this; please redirect me if needed.
I used 8.13.0 today and it seems that it behaves different compared to 10.12.0 regarding emitting error events.
e.g. if I pass a non existing file to respondWithFile() the error event is issued after the close event for 8.x; for 10.x error is emitted first.

@MylesBorins
Copy link
Member

@Flarna if you could open a new issue that documents how to recreate the observed behavior we can get to this ASAP

@Flarna Flarna mentioned this pull request Nov 21, 2018
Closed
@Flarna
Copy link
Member

Flarna commented Nov 21, 2018

@MylesBorins done: #24559

@Flarna Flarna mentioned this pull request Dec 2, 2018
Closed
3 tasks
BethGriggs pushed a commit that referenced this pull request Dec 11, 2018
@Flarna @BethGriggs
http2: fix sequence of error/close events
4c24a82 
Correct sequence of emitting `error` and `close` events for a
`Http2Stream`.

PR-URL: #24789
Refs: #22850
Refs: #24685
Fixes: #24559
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
@kasicka kasicka mentioned this pull request May 22, 2019
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
lib / src Issues and PRs related to general changes in the lib or src directory. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet