http2: additional refinement of stream/session

[jasnell]

tightening up the Http2Stream/Http2Session lifecycle, destroy, and
error handling. Some simplification, and more new tests

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• commit message follows commit guidelines

Affected core subsystem(s)

http2

[jasnell]

@mcollina ... ok, take a look. I think most of the comments are addressed with the exception of the error code for an invalid / destroyed stream.

[jasnell]

Note that the stream: add destroy and _destroy methods commit will be dropped once that commit lands in master and I have a chance to rebase http2/master from core again.

[mcollina]

@jasnell I think this is the right place to address #143. If not, LGTM this one and do another PR.

[jasnell]

@mcollina ... following the rebase off master the other day, it looks like we now have some fairly significant issues on the internals due to the changes in async_wrap that landed as part of async_hooks. We're getting some check asserts that are caused by the stack not properly unwinding as expected. This is most likely caused by insufficient process.nextTick() and setImmediate() calls but I'm still investigating. I do think I have the cause for #143 figured out but I need to work up a fix.

[jasnell]

@mcollina ... just fyi... I'm going through the internals on the native side and after going through things for #143, I've decided that it needs a fairly significant internal refactor. I'm going to be working on that next. Hopefully it shouldn't take too long.

Basically, what I need to refactor is the part that buffers callbacks on nghttp2 receives frames. Right now, that buffering is (a) inefficient, (b) largely unnecessary, and (c) causes issues when a stream is destroyed on the same tick as other callbacks (buffered data chunks end up being dropped on the floor). It's a fairly significant internal refactoring but it should not have any impact on the JS level API.

I will do the refactoring as part of this PR

[sebdeckers]

@jasnell Not sure if it's entirely related but some async_wrap/TLS issues were fixed in the node repo since the rebase here in the http2 fork last week. Maybe do another rebase? E.g. NPM doesn't work using the fork but this PR landed 3 days ago and fixed it. nodejs/node#13092

[jasnell]

That could certainly be part of it, but there are a number of other weaknesses internally and improvements that can be made. Expect a fairly sizeable pr ;)

[jasnell]

mapToHeaders definitely needs a refactor. I would love for someone to tackle that. Here's the background on it:

1. nghttp2 expects headers to be passed in as an array of nghttp2_nv structs. Each struct includes the header name, value, and flags (more on the flags in a minute)

2. Node.js users are most familiar with using an object for headers.

3. mapToHeaders() takes an object like {foo: 'a', bar: ['b', 'c']} and produces an array like [['foo', 'a'], ['bar', 'b'], ['bar', 'c']], which is then passed down to the binding layer. Passing the headers to the binding layer makes it significantly faster to produce the appropriate array of nghttp2_nv structs.

4. The HTTP/2 spec requires us to perform some validation of headers. While nghttp2 takes care of at least part of this, it does not handle all of it. For instance:

   • HTTP/2 response pseudo-headers (specifically :status) is forbidden from being included in an HTTP/2 request message; likewise, request pseudo-headers (e.g. :path, :method, etc) must not appear within response messages; no pseudo-headers may appear within trailers.
   • HTTP/1 Connection specific headers (like Connection, Keep-Alive, etc are not permitted to be used at all. While nghttp2 will reject messages containing these that it receives, it is our responsibility to make sure they are not there in messages that we send.

5. These validations are expensive. Every one of them requires string comparisons for every header field name (and in the case of the TE header, we even have to look at the field value (TE is allowed only if the value is 'trailers' and nothing else).

6. HTTP/2 header compression allows the sender to decide whether specific header value pairs are indexed in the HPACK header compression mechanism. By default, we're indexing everything right now, but at some point we may need to expose the API so that users can decide this. This is set on a header by header basis. This is set per header by a flag.

So... these are the constraints that we need to work within. Like I said, I would love someone to take a look and improve this part.

[mcollina]

@jasnell I will be traveling for the next two weeks, so I cannot really help. I think opening up a separate issue with that content (after you land this) will help.

[kjin]

@jasnell Are you proposing that the logic in mapToHeaders be moved into the C++ layer (based on 1, 3, 4, 5)?

I'd be interested in helping out with this.

[sebdeckers]

@jasnell Just want to throw an idea out here: Move the validation logic out of mapToHeaders and into a separate helper or higher level API (like compat). The low-level API allows for better performance, while a higher level API is useful for compliance/convenience. And/or, allow passing headers as that already-optimised data structure to avoid the conversion and validation. E.g. A typical pattern for server push is lots of nearly-identical headers -- why waste CPU cycles re-validating?

FWIW I'm measuring ~40-50% rps drop from core to compat with a minimalist on('stream|request', (res) => res.end('hello world')) benchmark. Would be interesting to see the delta with an even lower-level API.

[jasnell]

@sebdeckers the one key constraint is that even the minimal core API needs to be completely compliant to the spec. I don't want to sacrifice any spec compliance for performance. We did that with the http/1 implementation and it has come back to bite us numerous times.

[jasnell]

@mcollina @nodejs/http2 ... all, this PR is updated with a fairly sizable and important refactoring of the http2 internals. I have eliminated the caching of the callbacks, improved the lifecycle state of stream and session, and fixed a range of bugs in the process. The data bug #143 should be fixed by this.

@mcollina ... we should check the performance impact of this change.

[sebdeckers]

@jasnell Great work! 🤠

Just a quick obersvation. I did a ./configure --debug-http2 --debug-nghttp2 build. Running pushserver.js & pushclient.js (from #143) to verify the fixed behaviour. I'm seeing these log lines repeated at incredible rate:

Client:

stream: adjusting kept idle streams num_idle_streams=0, max=100
Http2Session: sending data to the socket: size: 0
Nghttp2Session 1: receiving data from socket: 1

Server (this continues as long as the client is connected though idle):

Http2Session: sending data to the socket: size: 0
Nghttp2Session 0: emitting all pending data
stream: adjusting kept idle streams num_idle_streams=0, max=100

It completely floods the terminal in less than a second. Is this expected?

Edit: The bug does seem to be fixed, thank you! 👏

[jasnell]

Yeah, likely better not to print those debug statements then

[sebdeckers]

@jasnell Any specific goals for these perf benchmarks? Which metrics, protocol features, client/server setups, stream:session ratios, request/response types, etc.

@mcollina Is anyone planning to port autocannon to http2?

I've been using ngload which is great for basic, http1-like, load tests. However it lacks proper PUSH_PROMISE support.

[jasnell]

@sebdeckers ... specific goals right now are only to have a good idea of where we are at currently. I have had in my list for some time the need to set up proper benchmarks for http2 like we have with http in core. The http benchmarks require external tools to be installed, and the same would be true in this case. What I imagine, at first, is that we would initially just use nghttp2's own h2load benchmarking tool

[mcollina]

@mcollina Is anyone planning to port autocannon to http2?

After we release this, I would get it added. More likely, it would be a new tool as autocannon is really tied to HTTP/1.1

[jasnell]

@mcollina ... let me know if the refactor looks good to you. I'll be running a bunch of benchmarks this week but I'd like to get this landed.

[mcollina]

LGTM

[jasnell]

Rebased and updated. Landing!

[jasnell]

Landed!