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.

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

http2: backport changes to v9.x-staging #18050

Closed
wants to merge 27 commits into from

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.