Node: [Tracking Issue]: Some http2 todos

[jasnell]

Hello @nodejs/http2 folks... now that the big destroy/cleanup refactor is finally done, there are a number of remaining items that need working on with an eye towards bringing http2 out of experimental in time for Node.js 10.0.0. We can use this tracking issue to keep organized.

Please leave a comment in the thread if you wish to take on one of the items. I've added a comment on the ones I'm definitely working on.

• [x] PR: https://github.com/nodejs/node/pull/17763 -- Convert Http2Settings into an Async handle object following the same pattern as Http2Ping (c++ and javascript). Currently, there is no async context tracking between sending SETTINGS and receiving the acknowledgement back. What I'd like to do is add a callback argument to Http2Session.prototype.settings() and turn the node::http2::Http2Settings object into an AsyncWrap following the same pattern Http2Ping uses. There are a couple of complicating points on this:

  • A SETTINGS frame is sent automatically at the start of the Http2Session and there is currently no way to pass a callback in for this. Currently a localSettings event is emitted on the Http2Session when it receives an acknowledgement, which is the only way we currently have for catching these. If someone wants to set a callback for that initial SETTINGS frame, then they'll need a way of setting it.
  • The localSettings and remoteSettings events would need to be refactored.

• [x] respondWithFile() and respondWithFD() should have an associated request handle and callback. Currently the only way to know when these operations are done is to watch of the close event on the Http2Stream. (c++ and javascript)

• [x] node::http2::Http2Stream::Provider:FD currently uses synchronous libuv fs calls to fill in the DATA frame buffer. While these are relatively small reads, it is still blocking file i/o. This bit needs to be refactored to use non-blocking. This will require quite a bit of work and requires quite a bit of C++ know how to get done. (all c++)

• [x] Performance metrics! Each Http2Session should record some basic metrics about the number of Http2Streams that have been used, the average duration, etc. These could also reasonably emit trace events in a number of places along with some perf_hooks integration. I'll be working on this in the coming few weeks. (mostly c++, some js) https://github.com/nodejs/node/pull/17906

• [x] Reviewing test coverage. Pretty self-explanatory.

• [x] Support unref() and ref() in HTTP2Session  and HTTP2Stream https://github.com/nodejs/node/issues/16617

• [ ] Add a http2.Pool object to maintain a pool of sessions open, one for every target host. This functionality is needed by everyone that wants to use the client. Requires https://github.com/nodejs/node/issues/16617 because the pool should automatically ref() and unref() the sessions based on usage. There is no global pool. (will depend largely on origin set support landing - https://github.com/nodejs/node/pull/17935)

• [x] Update nghttp2 library dependency (https://github.com/nodejs/node/pull/17908)

• [x] Support ALTSVC frames https://github.com/nodejs/node/pull/17917

• [x] Initial origin set support https://github.com/nodejs/node/pull/17935

• [ ] Stretch goal: rudimentary support for extension frames.

I'm currently working on the Http2Settings and perf metrics items, and will be looking at the extension frames bit later on once everything else is done.

[addaleax]

Regarding the file/fd handling … I still think these APIs shouldn’t exist. I can see that performance is the reason we have them, but that’s just a failure on Node’s streams API in general, and we shouldn’t have to give up orthogonality as a principle of good API design for that.

My current short-term goal (maybe that’s obvious from the PRs I’m opening) is to simplify and increase performance of our StreamBase streams. I’m having this specific use case in mind; what I want – and I know this might be a long shot – is to:

• Have fs streams be backed by StreamBase as well so we can make them interact with other C++ streams more easily
• Implement a fast-path for a.pipe(b) if a and b are both native streams that completely avoids calling into JS
  
  
  
  • This obviously won’t always work, so we need a bailout mechanism, but I am reasonably sure that it’s doable
  
  
  • I want to make sendfile() happen for FS → Network streams as part of that fast path – Also tricky, but again I’m reasonably sure that it’s doable
  
  

As a heads up, the next piece of that journey is that I’m trying to get rid of WriteWrap and ShutdownWrap, at least as JS APIs and presumably as C++ APIs as well.

[jasnell]

I'm very happy to see the improvements in StreamBase but I really don't want to lose the respondWithFile/respondWithFD APIs.

[addaleax]

@jasnell Why? There’s no reason for users to expect them to do anything different than fs.createReadStream(…).pipe(http2stream), right?

[jasnell]

if we can get a reasonable fast path with the current pipe model (your second main bullet) then ok. For me, the requirement is not so much the specific API surface as much as it is having the ability to send file purely at the c++ level without pulling any of the chunks into JS.

The new streams API work that @Fishrock123 and I have (slowly) been looking at will make this far more efficient once we get down that road further.

There's no reason for users to expect them to do anything different than...

Well, yes... (1) data transfer purely at the C++ level, (2) zero buffering, (3) no transforms, (4) no unnecessary C++-to-JS boundary crosses... Using a ReadStream and pipe brings along a certain range of assumptions and requirements that respondWithFD/File simply do not require, and that we do not strictly need.

[bacriom]

Hi @jasnell, I'm new so I wanted to collaborate on this issue, can you tell me one task to start?

[apapirovski]

@bacriom Writing tests is always a great first thing to work on. For HTTP2, here are the coverage reports: https://coverage.nodejs.org/coverage-06033695ed0bfca5/root/internal/http2/index.html — just find an area that has missing coverage and work on tweaking an existing test or creating a new one.

Or outside of HTTP2, check out issues labeled as "good first issue."

[bacriom]

@apapirovski thanks! :)

[kamesh95]

Hey @jasnell, I am new to this open source community and would like to contribute. Can you help with any task to start over here? I work with node js daily at work. So I can start with javascript as well as C, C++ related code. Thanks!

[mcollina]

@addaleax fs.createReadStream(…).pipe(http2stream) will never be as fast as respondWithFile. The first needs to support multiple destinations in javascript, while the later is all implemented in the lower layers. Serving files has historically being extremely slow, and this is brilliant solution.

@jasnell I think we should add an http2.Pool object, to maintain a pool of sessions open, one for every target host. This functionality is needed by everyone that wants to use the client, so we can as well add it ourselves. I don't think we should provide a global instance of that pool.

[jasnell]

@mcollina ... +1 on the http2.Pool ... Feel free to add that to the list in the original post above with a bit of text explaining it.

[jasnell]

@kamesh95 ... a fantastic first task would be to look at improving test coverage and documentation. That's a fantastic starting point to begin learning the implementation details and helps us with a piece that is both critically important and definitely underserved at the moment.

[kamesh95]

@jasnell Thanks! I will start looking into the existing test cases and their flow to get a better understanding and then start contributing. :)

[sebdeckers]

@jasnell wrote:

Add a http2.Pool object to maintain a pool of sessions open, one for every target host.

@mcollina wrote:

I think we should add an http2.Pool object, to maintain a pool of sessions open, one for every target host.

Not exactly one-to-one. For the design of this pool, do keep in mind that HTTP/2 already supports coalescing, and proposals like Alt Svc or Secondary Certs push in the same direction.

Note also that head of line blocking under TCP congestion still exists in HTTP/2. Explicitly creating multiple H2 sessions may still be useful.

Couple of ideas:

• Should the session pool be used transparently by any client that does not explicitly overwrite it?
• Should it overload the agent option, for compatibility, or should there be a new session (name?) option?

[bacriom]

@apapirovski hi ! I wrote some coverage tests as you said... and I want to know, can I create a pull request to extend the code coverage ?

[apapirovski]

@bacriom You should always feel free to open a PR for any changes you have, you don't need to check with anyone first :) If you need any more info, see our Contributing guide: https://github.com/nodejs/node/blob/master/CONTRIBUTING.md#pull-requests

[jasnell]

We definitely need to support multiple Http2Session instances per origin in the Pool, but it does add some complexity around how to select which one to give to the user. It might be that only the most recently created one should be used because the older one is in the process of being closed down; or it might be based on relative load; or it could be based on something more subtle such as what settings have been configured (e.g. there may be one with larger frame sizes and flow control tuned for large data transfers. A pluggable selection function should be allowed with a reasonable default in place.

I think the easiest will be to use the Pool transparently for all clients. With regard to agent compatibility, I'd say call it something different.

[mcollina]

I would leave http2.connect() as is, and build the pool as a facility on top. I’m very -1 on adding the same behavior of the agent property of http1, because none of the objects are compatible anyway.

Developing a compat version for the client is another topic, and not what I had in mind.

@apapirovski I wrote both sentences, they look skmilar to me, but it’s definitely not flashed out enough for a full implementation. It does not want to be a spec.

We need a pool concept because every time I see an http2 client library, they implemented their own pool: there is some complexity in all the events that they need to listen to and I think it’s something we can implement for good.

[apapirovski]

@mcollina I assume you were replying to @sebdeckers? or @addaleax?

[mcollina]

My bad, it was @sebdeckers.

[jasnell]

Thinking about the Pool implementation requirements more. Looking at https://tools.ietf.org/html/draft-ietf-httpbis-origin-frame-04, which is awaiting publication as an RFC now, every Http2Session has an origin set that defines the set of origins for which the connection may be used. There are strict requirements on matching the origins up with authenticated domains in the certificate. Given the potential security issues, I'm thinking that Pool should only work with secure ClientHttp2Session instances (https: and not http).

I've opened https://github.com/nodejs/node/pull/17935 to get things rolling. We do not have to wait for ORIGIN frame support to land, but this PR adds the necessary API surface that we'll need.

[mcollina]

@jasnell I would leave it open, but with a default for secure things. A non-secure Pool can be very handy for microservice communication.

[BridgeAR]

Should this be kept open? Most things got addressed and the rest could probably be dealt with on demand?

[mcollina]

@BridgeAR I think we _could_ open quite a few "good first issue" to increase code coverage of http2.
I'm ok for closing after https://github.com/nodejs/node/pull/22466 lands.

[jasnell]

Just note that quite a bit of the missing test coverage in that are for purely defensive branches that should be impossible to get to and would extremely difficult to construct a test for.

[pietermees]

Regarding http2.Pool: I did some preliminary work trying to come up with an Agent that can service both https and h2 requests: https://github.com/zentrick/alpn-agent

In many scenarios, you'll want a client that ideally uses h2 when available, but falls back to http/1.1 when not. This requires ALPN negotiation, and then using the negotiated connection either with https through an https.Agent, or transforming the negotiated socket to a ClientHttp2Session and perform an h2 request against it.

We've been discussing in https://github.com/szmarczak/http2-wrapper/issues/6 and https://github.com/sindresorhus/got/issues/167 to use this in https://github.com/sindresorhus/got

[mcollina]

@pietermees We are not really talking about a new module/function capable of doing both HTTP1 and HTTP2, but rather something that allows consumer to not worry about sessions, easily handle handle reconnects (there is an upper limit to the number of streams that can be created over a session) and maybe support some basic connection pooling. This is a basic piece of software to enable some work in the ecosystem.

Specifically our Agent implementation is _completely incompatible_ to what is not _plaintext_ HTTP over a Node.js Duplex. As an example, HTTP over quic would be problematic as well.

If we want to support a similar API, a good starting point could be to add HTTP2IncomingMessage and HTTP2ClientRequest to core, i.e. compatibility mode for the client as well. @szmarczak Would you like to send a PR with what you have in https://github.com/szmarczak/http2-wrapper? (Or somebody else)

[szmarczak]

Yeah, sure.

[jasnell]

Closing this. The remaining items can be handled individually if someone wishes to pursue them

[szmarczak]

Finally I got the time to release 1.0.0-alpha.0 of http2-wrapper. If you could give any feedback on:

• https://github.com/szmarczak/http2-wrapper/issues/16
• https://github.com/szmarczak/http2-wrapper/issues/17

that'd be great! I'll try to send a PR once 1.0.0 is released.

[pietermees]

@szmarczak added comments to both issues :)

[szmarczak]

@pietermees Replied back.

[szmarczak]

@jasnell Are you familiar with the Origin Set usage? If so, would you mind claryfying some things on https://github.com/szmarczak/http2-wrapper/issues/17? I really could use some help. problem solved