[8.x] Backport http2 changes from 9.x #20456

kjin · 2018-05-01T18:56:07Z

This is a backport of commits related to http2 changes from the 9.x branch to 8.x, as based off of the list of commits in this gist. I took the liberty of adding a few other changes which seemed like they needed to be backported as well as pre-requisites -- this fork of the earlier mentioned gist illustrates how I deviated from that list. Concerning this list:

A few were crossed out because I wasn't sure whether they should be added or not, for different reasons:
- It seemed clear to me that the version tag commits shouldn't be backported.
- 3914e97 seemed to depend on stream: complete migration to internal/errors #16589 and others, which didn't have a backport label.
- I wasn't sure about the first two (last two in the list) commits at all.

I guess the list is slightly outdated so this PR doesn't completely cover every commit, but I think it's a start.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
documentation is changed or added
commit message follows commit guidelines

/cc @MylesBorins @nodejs/http2 @jasnell

Instead of having per-request callbacks, always call a callback on the `StreamBase` instance itself for `WriteWrap` and `ShutdownWrap`. This makes `WriteWrap` cleanup consistent for all stream classes, since the after-write callback is always the same now. If special handling is needed for writes that happen to a sub-class, `AfterWrite` can be overridden by that class, rather than that class providing its own callback (e.g. updating the write queue size for libuv streams). If special handling is needed for writes that happen on another stream instance, the existing `after_write_cb()` callback is used for that (e.g. custom code after writing to the transport from a TLS stream). As a nice bonus, this also makes `WriteWrap` and `ShutdownWrap` instances slightly smaller. PR-URL: nodejs#17564 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: James M Snell <jasnell@gmail.com>

PR-URL: nodejs#17665 Fixes: nodejs#17636 Refs: nodejs#16482 Refs: nodejs#16860 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

Currently, when configured --without-ssl tests that use process.binding('crypto') fail with the following error: === release test-accessor-properties === Path: parallel/test-accessor-properties node/test/parallel/test-accessor-properties.js:16 const crypto = process.binding('crypto'); ^ Error: No such module: crypto at Object.<anonymous> (test-accessor-properties.js:16:24) at Module._compile (module.js:660:30) at Object.Module._extensions..js (module.js:671:10) at Module.load (module.js:577:32) at tryModuleLoad (module.js:517:12) at Function.Module._load (module.js:509:3) at Function.Module.runMain (module.js:701:10) at startup (bootstrap_node.js:194:16) at bootstrap_node.js:645:3 This commit adds a hasCrypto check. PR-URL: nodejs#17867 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

Remove reliance on V8-specific error messages in test/parallel/test-tls-external-accessor.js. Check that the error is a `TypeError`. The test should now be successful without modification using ChakraCore. PR-URL: nodejs#16272 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de>

Refactor and simplify the perf_hooks native internals. PR-URL: nodejs#17822 Reviewed-By: Anna Henningsen <anna@addaleax.net>

`OnCallbackPadding` on the native side already clamps the return value into the right range, so there’s not need to also do that on the JS side. Also, use `>>> 0` instead of `| 0` to get an uint32, since the communication with C++ land happens through an Uint32Array. PR-URL: nodejs#17717 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

PR-URL: nodejs#17908 Refs: nodejs#17746 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

Adds the possibility to keep a strong persistent reference to a JS object while a `SetImmediate()` call is in effect. Backport-PR-URL: nodejs#18050 PR-URL: nodejs#17183 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>

Calling into JS land from GC is not allowed, so delay the resolution of pending pings when a session is destroyed. Backport-PR-URL: nodejs#18050 PR-URL: nodejs#17183 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>

Introduce an `Http2Scope` class that, when it goes out of scope, checks whether a write to the network is desired by nghttp2. If that is the case, schedule a write using `SetImmediate()` rather than a custom per-session libuv handle. Backport-PR-URL: nodejs#18050 PR-URL: nodejs#17183 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>

PR-URL: nodejs#17406 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Backport-PR-URL: nodejs#18050

Backport-PR-URL: nodejs#18050 PR-URL: nodejs#17406 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> This is a significant cleanup and refactoring of the cleanup/close/destroy logic for Http2Stream and Http2Session. There are significant changes here in the timing and ordering of cleanup logic, JS apis. and various related necessary edits.

`nghttp2_stream_write_t` was not a necessary redirection layer and came with the cost of one additional allocation per stream write. Also, having both `nghttp2_stream_write` and `nghttp2_stream_write_t` as identifiers did not help with readability. Backport-PR-URL: nodejs#18050 PR-URL: nodejs#17718 Reviewed-By: James M Snell <jasnell@gmail.com>

- Only finish outgoing `WriteWrap`s once data has actually been passed to the underlying socket. - This makes HTTP2 streams respect backpressure - Use `DoTryWrite` as a shortcut for sending out as much of the data synchronously without blocking as possible - Use `NGHTTP2_DATA_FLAG_NO_COPY` to avoid copying DATA frame contents into nghttp2’s buffers before sending them out. Backport-PR-URL: nodejs#18050 PR-URL: nodejs#17718 Reviewed-By: James M Snell <jasnell@gmail.com>

Backport-PR-URL: nodejs#18050 PR-URL: nodejs#17763 Refs: nodejs#17746 Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

Backport-PR-URL: nodejs#18050 PR-URL: nodejs#17863 Fixes: nodejs#17840 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

Keep a local handle as a reference to the JS `Http2Session` object so that it will not be garbage collected when inside an `Http2Scope`, because the presence of the latter usually indicates that further actions on the session object are expected. Strictly speaking, storing the `session_handle_` as a property on the scope object is not necessary, but this is not very costly and makes the code more obviously correct. Fixes: nodejs#17840 Backport-PR-URL: nodejs#18050 PR-URL: nodejs#17863 Fixes: nodejs#17840 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

Backport-PR-URL: nodejs#18050 PR-URL: nodejs#17620 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

Collect and report basic timing information about `Http2Session` and `Http2Stream` instances. Backport-PR-URL: nodejs#18050 PR-URL: nodejs#17906 Refs: nodejs#17746 Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

This is the portion of be2cbcc that is not in dea44b9. Update performance_state to use AliasedBuffer and update usage sites. PR-URL: nodejs#18300 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com>

Strictly limit the number of concurrent streams based on the current setting of the MAX_CONCURRENT_STREAMS setting Backport-PR-URL: nodejs#18050 PR-URL: nodejs#16766 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Sebastiaan Deckers <sebdeckers83@gmail.com>

This commit also includes prerequisite error definitions from c75f87c and 1698c8e. Add support for sending and receiving ALTSVC frames. Backport-PR-URL: nodejs#18050 PR-URL: nodejs#17917 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

Backport-PR-URL: nodejs#18050 PR-URL: nodejs#17935 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Sebastiaan Deckers <sebdeckers83@gmail.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>

Add new properties to `Http2Session` to identify alpnProtocol, and indicator about whether the session is TLS or not, and initial support for origin set (preparinng for `ORIGIN` frame support and the client-side `Pool` implementation. The `originSet` is the set of origins for which an `Http2Session` may be considered authoritative. Per the `ORIGIN` frame spec, the originSet is only valid on TLS connections, so this is only exposed when using a `TLSSocket`. Backport-PR-URL: nodejs#18050 PR-URL: nodejs#17935 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Sebastiaan Deckers <sebdeckers83@gmail.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>

Add a padding strategy option that makes a best attempt to ensure that total frame length for DATA and HEADERS frames are aligned on multiples of 8-bytes. Backport-PR-URL: nodejs#18050 PR-URL: nodejs#17938 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

Backport-PR-URL: nodejs#18050 PR-URL: nodejs#17942 Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

Backport-PR-URL: nodejs#18050 PR-URL: nodejs#17954 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

The maxSessionMemory is a cap for the amount of memory an Http2Session is permitted to consume. If exceeded, new `Http2Stream` sessions will be rejected with an `ENHANCE_YOUR_CALM` error and existing `Http2Stream` instances that are still receiving headers will be terminated with an `ENHANCE_YOUR_CALM` error. Backport-PR-URL: nodejs#18050 PR-URL: nodejs#17967 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

Backport-PR-URL: nodejs#18050 PR-URL: nodejs#17968 Reviewed-By: Anna Henningsen <anna@addaleax.net>

Backport-PR-URL: #20456 PR-URL: #19451 Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

Previously, "HTTP/2" was strictly used to describe the protocol, and HTTP2 the module. This distinction is deemed unnecessary, and consistency between the two terms is enforced. Backport-PR-URL: #20456 PR-URL: #19603 Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: Chen Gang <gangc.cxy@foxmail.com> Reviewed-By: Shingo Inoue <leko.noor@gmail.com>

Backport-PR-URL: #19579 Backport-PR-URL: #20456 PR-URL: #18861 Reviewed-By: James M Snell <jasnell@gmail.com>

First steps towards #19060 Backport-PR-URL: #20456 PR-URL: #19389 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

Backport-PR-URL: #20456 PR-URL: #19647 Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Chen Gang <gangc.cxy@foxmail.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Backport-PR-URL: #19579 Backport-PR-URL: #20456 PR-URL: #18854 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

Do not close the request if callback is not a function, and throw ERR_INVALID_CALLBACK TypeError Backport-PR-URL: #19229 Backport-PR-URL: #20456 PR-URL: #19061 Fixes: #18855 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Shingo Inoue <leko.noor@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de>

Clarify current behavior of http2server.close() and http2secureServer.close() w.r.t. perceived differences when compared with httpServer.close(). Fixes: #19711 Backport-PR-URL: #20456 PR-URL: #19802 Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

Backport-PR-URL: #20456 PR-URL: #19842 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>

This adds the optional options argument to `http.createServer()`. It contains two options: the `IncomingMessage` and `ServerReponse` option. Backport-PR-URL: #20456 PR-URL: #15752 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Evan Lucas <evanlucas@me.com>

This adds the Http1IncomingMessage and Http1ServerReponse options to http2.createServer(). Backport-PR-URL: #20456 PR-URL: #15752 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Evan Lucas <evanlucas@me.com>

Add optional Http2ServerRequest and Http2ServerResponse options to createServer and createSecureServer. Allows custom req & res classes that extend the default ones to be used without overriding the prototype. Backport-PR-URL: #20456 PR-URL: #15560 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

Backport-PR-URL: #20456 PR-URL: #18872 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>