Skip to content

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Jan 9, 2018

Backport of multiple http2 related commits to v9.x-staging.

Currently blocked by #18049 and whatever else is currently causing v9.x-staging to fail. Will be able to finish once v9.x-staging is functioning again.

Note: have not fully tested this yet because of the v9.x-staging breakage.

Ping @nodejs/http2 @mcollina

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
Affected core subsystem(s)

http2

@nodejs-github-bot nodejs-github-bot added lib / src Issues and PRs related to general changes in the lib or src directory. v9.x labels Jan 9, 2018
@jasnell jasnell changed the title Backport http2 v9.x http2: backport changes to v9.x-staging Jan 9, 2018
@jasnell jasnell added dont-land-on-v4.x http2 Issues or PRs related to the http2 subsystem. blocked PRs that are blocked by other issues or PRs. labels Jan 9, 2018
@targos
Copy link
Member

targos commented Jan 9, 2018

v9.x-staging fails because #17704 is not in it. Is it possible to include it in this PR?

@jasnell
Copy link
Member Author

jasnell commented Jan 9, 2018

Not sure. That's a semver-major pr so would need a semver-major minor backport that omitted the breaking bits. That would best be done in a separate PR.

@mcollina
Copy link
Member

mcollina commented Jan 9, 2018

Could this commit be further backported to 8 in case?

@jasnell
Copy link
Member Author

jasnell commented Jan 9, 2018

That's going to require a bit more work. Planning to do it later this week

@MylesBorins
Copy link
Contributor

I've rebased against the new v9.x-staging to remove a commit that was breaking the build, it looks like another commit may be breaking stuff so i'll likely rebase one more time 😄

@targos
Copy link
Member

targos commented Jan 9, 2018

@MylesBorins you are looking for #17800 which depends on #17704

@MylesBorins
Copy link
Contributor

@targos you are a saint! I was just digging in

addaleax and others added 12 commits January 9, 2018 05:09
Adds the possibility to keep a strong persistent reference to
a JS object while a `SetImmediate()` call is in effect.

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.

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.

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

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.

PR-URL: nodejs#17718
Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: nodejs#17763
Refs: nodejs#17746
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
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

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>
PR-URL: nodejs#17620
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
PR-URL: nodejs#17939
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: James M Snell <jasnell@gmail.com>
kjin pushed a commit to kjin/node that referenced this pull request May 1, 2018
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>
kjin pushed a commit to kjin/node that referenced this pull request May 1, 2018
Backport-PR-URL: nodejs#18050
PR-URL: nodejs#17968
Reviewed-By: Anna Henningsen <anna@addaleax.net>
kjin pushed a commit to kjin/node that referenced this pull request May 1, 2018
Backport-PR-URL: nodejs#18050
PR-URL: nodejs#17972
Reviewed-By: Weijia Wang <starkwang@126.com>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
kjin pushed a commit to kjin/node that referenced this pull request May 1, 2018
* verify protections against ping and settings flooding
* Strictly handle and verify handling of unsolicited ping and
  settings frame acks.

Backport-PR-URL: nodejs#18050
PR-URL: nodejs#17969
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
kjin pushed a commit to kjin/node that referenced this pull request May 1, 2018
Backport-PR-URL: nodejs#18050
PR-URL: nodejs#17911
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Weijia Wang <starkwang@126.com>
MylesBorins pushed a commit that referenced this pull request May 2, 2018
Adds the possibility to keep a strong persistent reference to
a JS object while a `SetImmediate()` call is in effect.

Backport-PR-URL: #18050
Backport-PR-URL: #20456
PR-URL: #17183
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
MylesBorins pushed a commit that referenced this pull request May 2, 2018
Calling into JS land from GC is not allowed, so delay
the resolution of pending pings when a session is destroyed.

Backport-PR-URL: #18050
Backport-PR-URL: #20456
PR-URL: #17183
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
MylesBorins pushed a commit that referenced this pull request May 2, 2018
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: #18050
Backport-PR-URL: #20456
PR-URL: #17183
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
MylesBorins pushed a commit that referenced this pull request May 2, 2018
Backport-PR-URL: #20456
PR-URL: #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: #18050
MylesBorins pushed a commit that referenced this pull request May 2, 2018
Backport-PR-URL: #18050
Backport-PR-URL: #20456
PR-URL: #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.
MylesBorins pushed a commit that referenced this pull request May 2, 2018
`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: #18050
Backport-PR-URL: #20456
PR-URL: #17718
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request May 2, 2018
- 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: #18050
Backport-PR-URL: #20456
PR-URL: #17718
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request May 2, 2018
Backport-PR-URL: #18050
Backport-PR-URL: #20456
PR-URL: #17763
Refs: #17746
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
MylesBorins pushed a commit that referenced this pull request May 2, 2018
Backport-PR-URL: #18050
Backport-PR-URL: #20456
PR-URL: #17863
Fixes: #17840
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit that referenced this pull request May 2, 2018
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: #17840

Backport-PR-URL: #18050
Backport-PR-URL: #20456
PR-URL: #17863
Fixes: #17840
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit that referenced this pull request May 2, 2018
Backport-PR-URL: #18050
Backport-PR-URL: #20456
PR-URL: #17620
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
MylesBorins pushed a commit that referenced this pull request May 2, 2018
Collect and report basic timing information about `Http2Session`
and `Http2Stream` instances.

Backport-PR-URL: #18050
Backport-PR-URL: #20456
PR-URL: #17906
Refs: #17746
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
MylesBorins pushed a commit that referenced this pull request May 2, 2018
Strictly limit the number of concurrent streams based on the
current setting of the MAX_CONCURRENT_STREAMS setting

Backport-PR-URL: #18050
Backport-PR-URL: #20456
PR-URL: #16766
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Sebastiaan Deckers <sebdeckers83@gmail.com>
MylesBorins pushed a commit that referenced this pull request May 2, 2018
This commit also includes prerequisite error definitions
from c75f87c and 1698c8e.

Add support for sending and receiving ALTSVC frames.

Backport-PR-URL: #18050
Backport-PR-URL: #20456
PR-URL: #17917
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
MylesBorins pushed a commit that referenced this pull request May 2, 2018
Backport-PR-URL: #18050
Backport-PR-URL: #20456
PR-URL: #17935
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Sebastiaan Deckers <sebdeckers83@gmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
MylesBorins pushed a commit that referenced this pull request May 2, 2018
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: #18050
Backport-PR-URL: #20456
PR-URL: #17935
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Sebastiaan Deckers <sebdeckers83@gmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
MylesBorins pushed a commit that referenced this pull request May 2, 2018
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: #18050
Backport-PR-URL: #20456
PR-URL: #17938
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
MylesBorins pushed a commit that referenced this pull request May 2, 2018
Backport-PR-URL: #18050
Backport-PR-URL: #20456
PR-URL: #17942
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
MylesBorins pushed a commit that referenced this pull request May 2, 2018
Backport-PR-URL: #18050
Backport-PR-URL: #20456
PR-URL: #17942
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
MylesBorins pushed a commit that referenced this pull request May 2, 2018
Backport-PR-URL: #18050
Backport-PR-URL: #20456
PR-URL: #17954
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
MylesBorins pushed a commit that referenced this pull request May 2, 2018
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: #18050
Backport-PR-URL: #20456
PR-URL: #17967
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
MylesBorins pushed a commit that referenced this pull request May 2, 2018
Backport-PR-URL: #18050
Backport-PR-URL: #20456
PR-URL: #17968
Reviewed-By: Anna Henningsen <anna@addaleax.net>
MylesBorins pushed a commit that referenced this pull request May 2, 2018
Backport-PR-URL: #18050
Backport-PR-URL: #20456
PR-URL: #17972
Reviewed-By: Weijia Wang <starkwang@126.com>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
MylesBorins pushed a commit that referenced this pull request May 2, 2018
* verify protections against ping and settings flooding
* Strictly handle and verify handling of unsolicited ping and
  settings frame acks.

Backport-PR-URL: #18050
Backport-PR-URL: #20456
PR-URL: #17969
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
MylesBorins pushed a commit that referenced this pull request May 2, 2018
Backport-PR-URL: #18050
Backport-PR-URL: #20456
PR-URL: #17911
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Weijia Wang <starkwang@126.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
http2 Issues or PRs related to the http2 subsystem. lib / src Issues and PRs related to general changes in the lib or src directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.