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

• PR: http2: convert Http2Settings to an AsyncWrap #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.

• 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)

• 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++)

• 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) http2: perf_hooks integration #17906

• Reviewing test coverage. Pretty self-explanatory.

• Support unref() and ref() in HTTP2Session  and HTTP2Stream http2: implement ref() and unref() on client sessions  #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 http2: implement ref() and unref() on client sessions  #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 - http2: initial support for originSet #17935)

• Update nghttp2 library dependency (deps: update nghttp2 to 1.29.0 #17908)

• Support ALTSVC frames http2: add altsvc support #17917

• Initial origin set support http2: initial support for originSet #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. :)