http2: major update to internals #17105

jasnell · 2017-11-17T20:58:02Z

This update does several significant things:

It eliminates the base Nghttp2* classes and folds those
in to node::http2::Http2Session and node::http2::Http2Stream
It makes node::http2::Http2Stream a StreamBase instance and
sends that out to JS-land to act as the [kHandle] for the
JavaScript Http2Stream class.
It shifts some of the callbacks from C++ off of the JavaScript
Http2Session class to the Http2Stream class.
It refactors the data provider structure for FD and Stream
based sending to help encapsulate those functions easier
It streamlines some of the functions at the C++ layer to
eliminate now unnecessary redirections
It cleans up node_http2.cc for better readability and
maintainability
It refactors some of the debug output
Because Http2Stream instances are now StreamBases, they are
now also trackable using async-hooks
The Stream::OnRead algorithm has been simplified with a
couple bugs fixed.
I've eliminated node_http2_core.h and node_http2_core-inl.h
Detect invalid handshake a report protocol error to session
Add Http2Session.prototype.ping

@mcollina @apapirovski @addaleax @sebdeckers @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

Affected core subsystem(s)

http2

addaleax · 2017-11-17T21:06:50Z

It makes node::http2::Http2Stream a StreamBase instance and
sends that out to JS-land to act as the [kHandle] for the
JavaScript Http2Stream class.

Yay for that!

It eliminates the base Nghttp2* classes and folds those in to node::http2::Http2Session and node::http2::Http2Stream

Tbh, I did like the distinction between the C++ wrapper for nghttp2 and our own Node code… I guess this is okay tho.

Also, the moving-code-around bits folded in with the rest of the changes into a single huge commit makes reviewing a bit harder than it could be ;)

addaleax · 2017-11-17T21:04:40Z

src/node_http2.cc

 using v8::String;
 using v8::Uint32;
 using v8::Uint32Array;
 using v8::Undefined;

 namespace http2 {

-Nghttp2Session::Callbacks Nghttp2Session::callback_struct_saved[2] = {
+Http2Session::Callbacks Http2Session::callback_struct_saved[2] = {


While you’re at it, could you make these const? Otherwise it’s technically global mutable state…

sebdeckers · 2017-11-18T10:17:31Z

@jasnell To prepare for an eventual QUIC layer, I had thought the "folding" of these classes would happen the other way around, i.e. retaining only the Nghttp2 classes and dropping the generic Http2Session/Http2Stream. I don't mean to bikeshed, the changes LGTM. Merely wondering if you had other future plans and considerations here.

sebdeckers

I only reviewed changes on the JS side.

sebdeckers · 2017-11-18T10:26:35Z

test/parallel/test-http2-ping.js

+      assert(ack);
+      assert.strictEqual(typeof duration, 'number');
+    })));
+    // Only one ping at a time


Why only one ping at a time? I see no such limitation in the http2 spec.

The protocol doesn't limit it, but PING is a potential DOS vector. The one at a time limit is intended to avoid the possibility of abuse.

What risk does PING pose over sending any other outgoing REQUEST/SETTINGS/PRIORITY frame?

Multiple outstanding PINGs could occur as keepalive heartbeat messages under high RTT conditions.

We also currently limit SETTINGS frame too, just not as strictly. The risk is in allowing node.js to be used inappropriately as an attack tool. It may be ok tho

We could allow a configurable upper limit on the number of outstanding pings, allowing more than one to be sent at a time, and using a stack for the Http2Ping callbacks

The FIFO approach should generally work, though, if I understand the spec correctly, there is a potential case where outstanding PING are not ACKed in the same order as the outgoing sequence. Methods for dealing with this would be limiting concurrency to 1, or matching the ACKed payload (assuming unique bytes).

sebdeckers · 2017-11-18T10:28:36Z

doc/api/http2.md

@@ -342,6 +342,31 @@ acknowledgement for a sent SETTINGS frame. Will be `true` after calling the
 `http2session.settings()` method. Will be `false` once all sent SETTINGS
 frames have been acknowledged.

+#### http2session.ping(callback)


Could ping expose an (optional) 8-octet payload param?

Yeah, I'm considering that, just not sure how useful it's going to be if we stick with the one at a time limit.

Some considerations:

Analysis/performance tools might want to send PING floods.

When RTT exceeds keepalive interval. (Ouch! 😵)

Avoid global state. Or at least do not expose it to the user.

Follow what the spec says. Implementation is no place for protocol design.

sebdeckers · 2017-11-18T10:32:58Z

test/parallel/test-http2-shutdown-errors.js

@@ -29,7 +29,7 @@ const tests = Object.getOwnPropertyNames(constants)
 let currentError;

 // mock submitGoaway because we only care about testing error handling
-Http2Session.prototype.submitGoaway = () => currentError.ngError;
+Http2Session.prototype.goaway = () => currentError.ngError;


nit: goAway to be consistent with pushPromise, rstStream.

The spec treats it as GOAWAY and PUSH_PROMISE / RST_STREAM, I was segmenting based on the presence of the _

sebdeckers · 2017-11-18T10:34:33Z

test/parallel/test-http2-client-unescaped-path.js

@@ -9,7 +9,8 @@ const server = http2.createServer();

 server.on('stream', common.mustNotCall());

-const count = 32;
+//const count = 32;


sebdeckers · 2017-11-18T10:35:22Z

test/parallel/test-http2-client-onconnect-errors.js

@@ -67,29 +56,21 @@ const genericTests = Object.getOwnPropertyNames(constants)
    type: 'session'
  }));

+


nit: does ESLint not complain about this extra blank line?

sebdeckers · 2017-11-18T10:39:26Z

lib/internal/http2/core.js

+  setNextStreamID(id) {
+    if (typeof id !== 'number')
+      throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'id', 'number');
+    if (id <= 0 || id > 2 ** 31 - 1)


Extract 0b111111... trick to a const to avoid repetitive calculation, or does V8 optimise that automatically?

I believe v8 optimizes these but it's still worthwhile avoid repetition

sebdeckers · 2017-11-18T10:41:23Z

lib/internal/http2/core.js

@@ -844,7 +778,7 @@ class Http2Session extends EventEmitter {

    const handle = this[kHandle];
    if (handle === undefined)
-      return Object.create(null);
+      return {};


Q: Why is this preferred? (IIRC we discussed this before so I'm curious why change it now.)

Under turbofan using the literal is more performant it turns out, and after evaluating, there's no real benefit to making these particular objects have that null prototype

IIRC, since about the time of TF being enabled (not sure it’s related), Object.create(null) acts as a hint to the engine that the object is in dictionary mode, i.e. kind of acts like a Map.

So not using a null-prototype object here seems like a good idea.

TIL 🌈✨ Thanks!

sebdeckers · 2017-11-18T10:49:11Z

Additional notable changelog:

Http2Session.priority() is removed. Only use Http2Stream.priority().

Do we need to worry about deprecating this API?

jasnell · 2017-11-18T13:50:34Z

While the code is still in experimental status, we shouldn't need to sorry about deprecating those, although we'll want to call it out as a notable change.

As for the possibility of introducing something like quic, we'd have to step back later and evaluate that. These changes shouldn't make it more difficult tho.

addaleax · 2017-11-18T14:10:37Z

doc/api/http2.md

+acknowledgement was received.
+
+```js
+session.ping((ack, duration) => {


If you want to deviate from errback style, it might be a good idea to add a custom util.promisify variant (or stick with errback)

Yeah, I'm debating back and forth on this one

Alternatively, this could accept a payload argument and simply send out a PING frame. Any received PING frames would emit a ping event with the payload as argument. That avoids state (leaking memory) and allows users to implement promises or callbacks as well as limit or allow parallel PINGs.

apapirovski

Amazing! 🥇 A lot here so will need to go through this in stages, but had a few nits/questions to start.

apapirovski · 2017-11-17T21:01:45Z

lib/internal/http2/core.js

    // then, on flip of the event loop, do the actual shutdown
-    setImmediate(doShutdown, this, options);
+    setImmediate(doShutdown.bind(this), options);


Is there a particular reason we do this? It's definitely faster without the bind & call. We could potentially rename self to something better if that's not very readable (like stream or session depending on the context). Especially since these are one-time use only, we're not storing the bound version or anything.

(There are a few other places this is the case.)

More of a preference than anything. Not opposed to switching it back

I'm fine with it either way. It's a super minor thing. Wanted to make sure I wasn't missing anything.

Can always open my own PR if it really bugs me one day... 😆

apapirovski · 2017-11-18T14:40:07Z

test/parallel/test-http2-client-onconnect-errors.js

@@ -40,7 +29,7 @@ const specificTests = [
      message: 'No stream ID is available because ' +
               'maximum stream ID has been reached'
    },
-    type: 'session'
+    type: 'stream'


Could you elaborate on this change? Maximum stream ID being reached means the whole session is probably in trouble, no?

More of an ergonomics thing, really. Yes it affects the session but it feels more natural to watch for the error on the steam itself. May be revisiting that tho

Makes sense 👍 I can definitely see arguments both ways.

apapirovski · 2017-11-18T15:06:41Z

lib/internal/http2/core.js

-      default:
-        if (ret <= 0) {
+    if (typeof ret === 'number') {
+      let target = session;


It seems like we could just not set the target by default since each case branch redefines it, including default.

Or if we don't anticipate it changing, maybe just remove target altogether and replace it with hard-coded this. Can always change later if a session error comes to exist in this branch.

apapirovski · 2017-11-18T15:13:20Z

lib/internal/http2/core.js

-  self.emit('destroy');
+function finishStreamDestroy() {
+  const id = this[kID];
+  this[kSession][kState].streams.delete(id);


Complete nit, but since id is now used only once, can we delete(this[kID]); instead?

Maybe I’m missing context, but delete is always going to change the map of the object whose property is being delete, so you don’t usually want that for performance reasons if it’s not necessary.

(And if you delete anything but the most recently added property, you’re going to switch the object into dictionary mode, which is even worse.)

Sorry, to be clear I just mean this: this[kSession][kState].streams.delete(this[kID]);. Avoid the unnecessary const declaration (which was there because id was reused in the earlier version).

@addaleax ... Keep in mind that this is Map, not a regular object.

ronkorving · 2017-11-20T10:47:11Z

Are there any performance implications to this many changes? If any, hopefully all good ones ;)

jasnell · 2017-11-20T13:46:07Z

I benchmarked carefully as I went through to make sure there were no regressions with perf. It's partly what took so long to get this going (change, bench, change, bench, change,... Lol). The most significant impact is on the Http2Stream class becoming the StreamBase and the change there was negligible.

jasnell · 2017-11-20T19:52:43Z

Quick note: there are a number of more substantial changes that need to be made to the Closing/Error state handling for both Http2Session and Http2Stream that will need to build on top of these changes. They are rather large and significant changes that I'd much prefer doing in a separate PR once this lands.

jasnell · 2017-11-20T19:55:06Z

~~CI: https://ci.nodejs.org/job/node-test-pull-request/11558/~~
New CI: https://ci.nodejs.org/job/node-test-pull-request/11560/

jasnell · 2017-11-20T21:37:04Z

Failed on one of the windows variants... passed on everything else... trying one more time:

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

jasnell · 2017-11-20T22:15:18Z

Ok, CI appears to be good on all platforms now (there are still a handful running at the moment but they passed on the previous run)

jasnell · 2017-11-20T23:52:46Z

CI is good. Will squash the commits.

@nodejs/http2 ... there are some other large changes that need to build on top of this so I'd appreciate some sign offs so I can get this landed :-)

This update does several significant things: 1. It eliminates the base Nghttp2* classes and folds those in to node::http2::Http2Session and node::http2::Http2Stream 2. It makes node::http2::Http2Stream a StreamBase instance and sends that out to JS-land to act as the [kHandle] for the JavaScript Http2Stream class. 3. It shifts some of the callbacks from C++ off of the JavaScript Http2Session class to the Http2Stream class. 4. It refactors the data provider structure for FD and Stream based sending to help encapsulate those functions easier 5. It streamlines some of the functions at the C++ layer to eliminate now unnecessary redirections 6. It cleans up node_http2.cc for better readability and maintainability 7. It refactors some of the debug output 8. Because Http2Stream instances are now StreamBases, they are now also trackable using async-hooks 9. The Stream::OnRead algorithm has been simplified with a couple bugs fixed. 10. I've eliminated node_http2_core.h and node_http2_core-inl.h 11. Detect invalid handshake a report protocol error to session 12. Refactor out of memory error, improve other errors 13. Add Http2Session.prototype.ping

addaleax · 2017-11-20T23:59:19Z

@jasnell Do you think you could split the commits so that moving code around is its separate thing? I know that’s quite a task, but it would really help a lot…

jasnell · 2017-11-21T00:02:45Z

umm... at this point, that would be extremely difficult to do, as moving the code around was not done as a separate task. It was done all at once while making the specific individual changes.

addaleax · 2017-11-21T00:22:23Z

doc/api/errors.md

+<a id="ERR_HTTP2_PING_LENGTH"></a>
+### ERR_HTTP2_PING_LENGTH
+
+HTTP/2 ping payloads must be exactly 8-bytes in length.


nit: 8 bytes

addaleax · 2017-11-21T00:22:53Z

doc/api/http2.md

+`maxOutstandingPings` configuration option. The default maximum is 10.
+
+If provided, the `payload` must be a `Buffer`, `TypedArray`, or `DataView`
+containing 8-bytes of data that will be transmitted with the `PING` and


nit: 8 bytes

addaleax · 2017-11-21T00:35:14Z

src/node_http2.cc

+  max_header_pairs_ =
+      type == NGHTTP2_SESSION_SERVER
+          ? std::max(maxHeaderPairs, 4)     // minimum # of request headers
+          : std::max(maxHeaderPairs, 1);    // minumum # of response headers


typo: minumum

addaleax · 2017-11-21T00:35:37Z

src/node_http2.cc

+                         const nghttp2_session_callbacks* callbacks,
+                         void* user_data,
+                         const nghttp2_option* options);
+  init_fn fn = type == NGHTTP2_SESSION_SERVER ?


mhhhhh… auto? 😄

yeah yeah... ;-) I haven't quite managed to get used to using auto too much. I'm working on it, I swear ;-)

addaleax · 2017-11-21T00:38:20Z

src/node_http2.cc

+// TODO(jasnell): Currently, this creates one uv_prepare_t per Http2Session,
+//                we should investigate to see if it's faster to create a
+//                single uv_prepare_t for all Http2Sessions, then iterate
+//                over each.


Just so you know what I’m having in mind for this: I’d keep a boolean flag on a each session that keeps track of whether a send callback is scheduled to be executed, and use the SetImmediate() from #17117 to schedule it once it becomes necessary (if that flag is not already set).

In particular I’d really like to avoid iterating over all sessions.

whether or not there is data to send is not always easily visible. nghttp has a want_write function that can be checked to see if a particular session has data to send.

nghttp2 doesn't do it's own I/O so it's always something that we do that makes it want to write something, right? We should use want_write after such an action to check.

(Fwiw, I would be up for trying to do this myself once this PR is merged, and if I prove myself wrong, so be it. ;))

I'm not saying it's a bad idea :-) ... just saying that it's not always immediately visible.

Btw... Currently, we are already calling SendPendingData() immediately after receiving and processing data on a socket. It's entirely possible that we could eliminate the prep_t call entirely, I just haven't worked on that yet.

To expand a bit... the underlying nghttp2_session will have data to write to the output stream in one of two cases:

We receive data from the socket and pass it in to nghttp2_session. This will trigger a series of synchronous callbacks to user code. In some cases, nghttp2 will push it's own data onto the send queue during this time, in other cases user code will push it's own data onto the send queue. Either way, when we're doing receiving the chunk of data, we call SendPendingData() to push out whatever was sent.

User-code takes other actions independent of receiving data from the socket. The uv_prep_t is really in place to handle this second set of actions. Rather than calling SendPendingData() multiple times, once after each action, the data is allowed to batch up in nghttp2 internal queue and is sent on each tick of the event loop.

So what you're suggesting is absolutely doable. First, keep the SendPendingData() in place for 1, then for 2, if any action occurring outside of the receive callback cycle puts data into the queue, schedule an env->SetImmediate() to call SendPendingData() on the next tick.

addaleax · 2017-11-21T00:49:16Z

src/node_http2.cc

-  Local<Array> headers = args[2].As<Array>();
+  Local<Context> context = env()->context();
+  Isolate* isolate = env()->isolate();
+  HandleScope scope(isolate);


addaleax · 2017-11-21T00:49:33Z

src/node_http2.cc

-  Local<Array> headers = args[1].As<Array>();
+  Local<Context> context = env()->context();
+  Isolate* isolate = env()->isolate();
+  HandleScope scope(isolate);


addaleax · 2017-11-21T00:50:20Z

src/node_http2.cc

+inline void Http2Session::HandleSettingsFrame(const nghttp2_frame* frame) {
+  Local<Context> context = env()->context();
+  Isolate* isolate = env()->isolate();
+  HandleScope scope(isolate);


addaleax · 2017-11-21T00:53:27Z

src/node_http2.cc

+  Environment* env = Environment::GetCurrent(args);
+  uint32_t val = args[0]->Uint32Value(env->context()).ToChecked();
+  args.GetReturnValue().Set(
+      OneByteString(env->isolate(), nghttp2_strerror(val)));


OneByteString() uses v8::NewStringType::kNormal – can you use kInternalized, since these are static strings (i.e. call NewFromOneByte directly with that flag)?

Good call :)

addaleax · 2017-11-21T01:01:47Z

src/node_http2.h

+ public:
+  Provider(Http2Stream* stream, int options);
+  explicit Provider(int options);
+  ~Provider();


Not making a base class destructor virtual is kind of dangerous… The small overhead might be worth avoiding the surprise of future crashes?

Yeah, good point :)

jasnell · 2017-11-21T03:16:56Z

that was the result of an over-zealous and lazy replace ;-)

jasnell · 2017-11-21T03:19:36Z

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

addaleax · 2017-11-21T11:48:59Z

Landed in 69e6c5a

Added semver-minor label for the new ping methods

This update does several significant things: 1. It eliminates the base Nghttp2* classes and folds those in to node::http2::Http2Session and node::http2::Http2Stream 2. It makes node::http2::Http2Stream a StreamBase instance and sends that out to JS-land to act as the [kHandle] for the JavaScript Http2Stream class. 3. It shifts some of the callbacks from C++ off of the JavaScript Http2Session class to the Http2Stream class. 4. It refactors the data provider structure for FD and Stream based sending to help encapsulate those functions easier 5. It streamlines some of the functions at the C++ layer to eliminate now unnecessary redirections 6. It cleans up node_http2.cc for better readability and maintainability 7. It refactors some of the debug output 8. Because Http2Stream instances are now StreamBases, they are now also trackable using async-hooks 9. The Stream::OnRead algorithm has been simplified with a couple bugs fixed. 10. I've eliminated node_http2_core.h and node_http2_core-inl.h 11. Detect invalid handshake a report protocol error to session 12. Refactor out of memory error, improve other errors 13. Add Http2Session.prototype.ping PR-URL: #17105 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Sebastiaan Deckers <[email protected]>

nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Nov 17, 2017

addaleax reviewed Nov 17, 2017

View reviewed changes

jasnell force-pushed the http2-rework-internals branch from c43fa6b to e1542d0 Compare November 17, 2017 23:05

mscdex added the http2 Issues or PRs related to the http2 subsystem. label Nov 18, 2017

sebdeckers reviewed Nov 18, 2017

View reviewed changes

addaleax reviewed Nov 18, 2017

View reviewed changes

addaleax mentioned this pull request Nov 18, 2017

src: introduce internal C++ SetImmediate() and remove async_hooks destroy timer #17117

Closed

2 tasks

apapirovski reviewed Nov 18, 2017

View reviewed changes

jasnell force-pushed the http2-rework-internals branch from 9c649e2 to 4c229db Compare November 20, 2017 19:54

jasnell force-pushed the http2-rework-internals branch from 72ec460 to a21dca0 Compare November 20, 2017 23:53

addaleax reviewed Nov 21, 2017

View reviewed changes

addaleax approved these changes Nov 21, 2017

View reviewed changes

[Squash] Fix nits

b05f1dc

jasnell force-pushed the http2-rework-internals branch from 6a7ca53 to b05f1dc Compare November 21, 2017 03:17

addaleax closed this Nov 21, 2017

MylesBorins mentioned this pull request Dec 7, 2017

v8.9.3 proposal #17532

Merged

		@@ -67,29 +56,21 @@ const genericTests = Object.getOwnPropertyNames(constants)
		type: 'session'
		}));

http2: major update to internals #17105

http2: major update to internals #17105

Conversation

jasnell commented Nov 17, 2017 • edited Loading

Checklist

Affected core subsystem(s)

addaleax commented Nov 17, 2017

Choose a reason for hiding this comment

sebdeckers commented Nov 18, 2017

sebdeckers left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sebdeckers commented Nov 18, 2017

jasnell commented Nov 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apapirovski left a comment

Choose a reason for hiding this comment

apapirovski Nov 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apapirovski Nov 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apapirovski Nov 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ronkorving commented Nov 20, 2017

jasnell commented Nov 20, 2017

jasnell commented Nov 20, 2017

jasnell commented Nov 20, 2017 • edited Loading

jasnell commented Nov 20, 2017

jasnell commented Nov 20, 2017

jasnell commented Nov 20, 2017

addaleax commented Nov 20, 2017

jasnell commented Nov 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasnell Nov 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasnell commented Nov 17, 2017 •

edited

Loading

apapirovski Nov 17, 2017 •

edited

Loading

apapirovski Nov 18, 2017 •

edited

Loading

apapirovski Nov 18, 2017 •

edited

Loading

jasnell commented Nov 20, 2017 •

edited

Loading

jasnell Nov 21, 2017 •

edited

Loading