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

kjin · 2018-09-13T22:03:06Z

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

mcollina · 2018-09-14T09:14:49Z

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

jasnell · 2018-09-14T09:35:41Z

Very nice! Thank you for taking this on

addaleax · 2018-09-14T15:41:07Z

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

kjin · 2018-09-15T00:04:15Z

@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.

kjin · 2018-09-19T22:57:12Z

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:

This seems to be part of a "StreamBase refactor" which I wasn't sure whether it belonged in v8.x
- e85c20b net,http2: merge write error handling & property names (net,http2: merge write error handling & property names #19734)
Changes to errors.js seem independent of H2 changes
- d28211e errors: validate input arguments (errors: validate input arguments #19924)
- b6fbe16 errors: alter ERR_HTTP2_INVALID_CONNECTION_HEADERS (errors: alter ERR_HTTP2_INVALID_CONNECTION_HEADERS #19807)
- f086354 errors: alter ERR_HTTP2_PSEUDOHEADER_NOT_ALLOWED (errors: alter ERR_HTTP2_PSEUDOHEADER_NOT_ALLOWED #19958)
Runtime-Controlled Debugging System -- this seems fairly independent despite the change being mostly in HTTP2
- 182c73b http2: switch to new runtime-controlled debugging system (src,http2: refactor + generalize http2 debugging #20987)
These are a dependent chain of commits that didn't seem to merge cleanly
- 0b3c80c http2: fix issues with aborted respondWithFile()s (http2: fix issues with aborted respondWithFile()s #21561)
- eea199b test: fix http2 connection abort test (test,doc: address http2 connection abort/network error handling #21861)
- c3d9000 doc: document http2 network error behaviour (test,doc: address http2 connection abort/network error handling #21861)
- 793a5bb test: don't fail http2 abort test if 'data' is called multiple times (test: don't fail http2 abort test if 'data' is called multiple times #21925)

@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 · 2018-09-25T18:00:40Z

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.

MylesBorins · 2018-09-25T19:42:28Z

@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 · 2018-09-25T22:36:39Z

@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 · 2018-09-26T03:22:28Z

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 · 2018-10-02T15:33:52Z

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

kjin · 2018-10-03T21:39:37Z

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:

d3ceaa1 (http2: emit timeout on compat request and response #22252): I changed tests related to timeouts. More info in http2: emit timeout on compat request and response #22252 (comment).
2a88731 (lib: merge onread handlers for http2 streams & net.Socket #22449): This PR has a lot of non-http2 stuff going on.
24675a4 (http2: add origin frame support #22956): This change makes use of a V8 API that's not available in Node 8. I use the closest approximation of the API (StringOneByte overload) instead. (The corresponding commit in this PR has a similar note.)
d1826fe (http2: close fd in doSendFileFD() #23047) This PR appears to fix an issue which might be the result of a PR that wasn't backported earlier (src,http2: use native-layer pipe mechanism from FileHandle instead of synchronous I/O #18936 -- contains do-not-backport-8.x tag).

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

~~@apapirovski http2: fix responses to long payload reqs #20084 (comment)~~ (talked in person, this seems OK)
@jasnell ~~http2: emit timeout on compat request and response #22252 (comment)~~ as well as -- does the third note in the list above make sense to you? (Edit -- I looked and it seems safe to me, the Isolate being used should be the same in both implementations)
@addaleax [8.x] Backport http2 changes from 10.x #22850 (comment) as well as -- Does it make sense for me not to backport the second commit listed above?

addaleax · 2018-10-03T21:46:43Z

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

kjin · 2018-10-15T19:02:19Z

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

MylesBorins · 2018-10-16T00:57:31Z

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

BethGriggs · 2018-10-16T15:20:14Z

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>

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>

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>

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>

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>

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>

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>

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>

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>

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>

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>

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>

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>

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>

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>

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>

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>

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>

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>

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>

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 · 2018-10-17T00:27:28Z

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

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 · 2018-11-21T13:24:14Z

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 · 2018-11-21T16:11:19Z

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

Flarna · 2018-11-21T20:04:42Z

@MylesBorins done: #24559

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>

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 added the semver-minor PRs that contain new features and should be released in the next minor version. label Sep 14, 2018

kjin force-pushed the o0003 branch from a9c0205 to 43eadca Compare September 15, 2018 01:24

addaleax added the wip Issues and PRs that are still a work in progress. label Sep 15, 2018

kjin force-pushed the o0003 branch from 43eadca to cc11604 Compare September 17, 2018 20:21

addaleax mentioned this pull request Sep 18, 2018

nodejs v8.11.3 aborts in http2 with TLS if client closes session #22923

Closed

kjin force-pushed the o0003 branch from 409f176 to 60d16b4 Compare September 19, 2018 22:56

kjin changed the title ~~[WIP] [8.x] Backport http2 changes from 10.x~~ [8.x] Backport http2 changes from 10.x Sep 20, 2018

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

kjin force-pushed the o0003 branch from 60d16b4 to 68e5ef9 Compare September 25, 2018 22:41

MylesBorins mentioned this pull request Oct 2, 2018

Doing another Semver Minor on 8.x nodejs/Release#364

Closed

kjin force-pushed the o0003 branch from ee7e9d1 to 02048d5 Compare October 4, 2018 18:09

jasnell and others added 2 commits October 16, 2018 09:19

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>

BethGriggs pushed a commit that referenced this pull request Oct 17, 2018

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 closed this Oct 17, 2018

BethGriggs mentioned this pull request Oct 17, 2018

http2: simplify timeout tracking #19206

Closed

2 tasks

BethGriggs mentioned this pull request Nov 20, 2018

v8.13.0 proposal #23974

Merged

Flarna mentioned this pull request Nov 21, 2018

http2: [8.x] server stream emits error after close #24559

Closed

Flarna mentioned this pull request Dec 2, 2018

[v8.x] http2: fix sequence of error/close events #24789

Closed

3 tasks

kasicka mentioned this pull request May 22, 2019

--debug-http2 fails build #27824

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

kjin commented Sep 13, 2018 •

edited

Loading

mcollina commented Sep 14, 2018

jasnell commented Sep 14, 2018

addaleax commented Sep 14, 2018

kjin commented Sep 15, 2018

kjin commented Sep 19, 2018 •

edited

Loading

kjin commented Sep 25, 2018

MylesBorins commented Sep 25, 2018

kjin commented Sep 25, 2018 •

edited

Loading

MylesBorins commented Sep 26, 2018

MylesBorins commented Oct 2, 2018

kjin commented Oct 3, 2018 •

edited

Loading

addaleax commented Oct 3, 2018

kjin commented Oct 15, 2018

MylesBorins commented Oct 16, 2018

BethGriggs commented Oct 16, 2018

BethGriggs commented Oct 17, 2018

Flarna commented Nov 21, 2018

MylesBorins commented Nov 21, 2018

Flarna commented Nov 21, 2018

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

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

Conversation

kjin commented Sep 13, 2018 • edited Loading

Checklist

mcollina commented Sep 14, 2018

jasnell commented Sep 14, 2018

addaleax commented Sep 14, 2018

kjin commented Sep 15, 2018

kjin commented Sep 19, 2018 • edited Loading

kjin commented Sep 25, 2018

MylesBorins commented Sep 25, 2018

kjin commented Sep 25, 2018 • edited Loading

MylesBorins commented Sep 26, 2018

MylesBorins commented Oct 2, 2018

kjin commented Oct 3, 2018 • edited Loading

addaleax commented Oct 3, 2018

kjin commented Oct 15, 2018

MylesBorins commented Oct 16, 2018

BethGriggs commented Oct 16, 2018

BethGriggs commented Oct 17, 2018

Flarna commented Nov 21, 2018

MylesBorins commented Nov 21, 2018

Flarna commented Nov 21, 2018

kjin commented Sep 13, 2018 •

edited

Loading

kjin commented Sep 19, 2018 •

edited

Loading

kjin commented Sep 25, 2018 •

edited

Loading

kjin commented Oct 3, 2018 •

edited

Loading