Compatibility fixes and test cases

[sebdeckers]

Accessing the request's headers was returning undefined. This is a quick fix that converts the headers object to a Map. Probably not the best for performance, but I wasn't sure if you'd rather rewrite all the property getters that expect .has and .get methods.

[mcollina]

wooow, thanks! Can you add a quick unit test for this? You can see an example in: https://github.com/nodejs/http2/blob/master/test/parallel/test-http2-create-client-session.js.

I'm not 100% sure what was the problem in the first place, so the test would help as well.

[sebdeckers]

@mcollina The problem was that stream[kHeaders] as always undefined.

Actually I don't quite like the Map solution. I will rewrite this with Object.prototype.hasOwnProperty instead, so that the headers remain a plain object.

Will also work on a test case, sure. 😄

[jasnell]

Why not pass the headers object off directly so that it does not need conversion?

stream[kHeaders] = headers;

[sebdeckers]

@jasnell Yep, doing that now. Got confused because the property getters use get/has. Not sure why...?

[sebdeckers]

@jasnell Pushed a new commit that uses the plain object. No more conversion.

@mcollina Added a separate test script for compat. Made it extensible so I can also write test cases for other parts of that file (i.e. response).

[sebdeckers]

PS: I see some unfinished provisions for implementing trailers on Http2ServerRequest. I believe this would involve listening to the stream.trailers event and storing them as this[kTrailers]. Possibly need to validate and merge multiple trailers. Is this a WIP? Happy to take a look at implementing this.

[sebdeckers]

@mcollina @jasnell I did some more work over the weekend to fix the compatibility layer. Please take a look and let me know what you think.

My aim is to make it work similar to Node.js' http APIs. Also added more tests, as requested. Probably covers some of #42.

• Am I going in the right direction with this PR?
• Anything I should clean up or refactor?
• How do you typically communicate on this project (Gitter, Slack, Hangouts)?

[jasnell]

I'll take a more detailed look through in the morning.

[sebdeckers]

@mcollina Thank you for the feedback! 🙇 Have now separated the tests into separate files. Also linted the code.

Notes to self:

Run Eslint as per the Makefile:

./node tools/eslint/bin/eslint.js --cache --rulesdir=tools/eslint-rules "test/parallel/test-http2-*"

Quickly run the relevant test cases in bash:

for test in test/parallel/test-http2-*.js; do ./node $test; done

[sebdeckers]

WIP:

• Cookies are not currently supported the same way the http1 module handles them.

• createPushResponse does not yet follow the API design shown in the implementation nodes. For now it simply returns the ServerHttp2Stream.

[mcollina]

Cookies are not currently supported the same way the http1 module handles them.

can you please open a separate issue about cookies?

[sebdeckers]

In 3739695 I've implemented a minimalist API for push requests and push responses.

server.on('request', (request, response) => {
  const headers = {
    ':path': '/foobar.txt',
    'content-type': 'text/plain; charset=utf8'
  }

  const pushRequest = response.createPushRequest(headers)
  // createPushRequest returns an Http2PushRequest
  // Its argument is a plain Object with the PUSH_PROMISE headers (key/value).

  // Its only method is .push() which sends the PUSH_PROMISE frame,
  // and offers a callback to deal with the push response.
  pushRequest.push((error, pushResponse) => {
    // pushResponse is just an Http2ServerResponse,
    // so it can do all the usual tricks...
    pushResponse.writeHead(200, headers)
    pushResponse.end('Hello, World!')
  })
})

@jasnell Would you be okay with this design replacing the one from the implementation notes example? It omits the .path property and request callback argument. Also renamed it to createPushRequest since that aligns with the RFC terminology (push request -> push response).

@mcollina I'm struggling to test the push stream using the client API. It looks like the PUSH_PROMISE frame is received in onSessionHeaders as a NGHTTP2_HCAT_RESPONSE, with the stream ID of the parent stream, i.e. the ID of the stream that received the frame, instead of the promised stream ID. Either I'm missing something or this may need some work on the C++ side. I'm not so good at that yet so any help is welcome. (I manually tested the server side's createPushRequest code using nghttp client. Pretty sure the bug is in the client side code.) Update: Fixed, see #47 (comment)

[sebdeckers]

@mcollina @jasnell Please review the C++ changes in 7f8a5eb#diff-20934af26ea78010b56b52f2fc636a18

This commit adds support for promised stream ID when receiving headers. The test case in test-http2-compat-Http2ServerResponse-createPushRequest.js passes, so push streams can now be used in the server and client.

However I'm seeing this error on exit:

$ ./node test/parallel/test-http2-compat-Http2ServerResponse-createPushRequest.js
Assertion failed: (pending_callbacks_head_ == nullptr), function Free, file ../src/node-http2-core-inl.h, line 325.
Abort trap: 6

Any ideas? It's been 20 years since I touched C++. This has been another adventurous weekend. 😅

Update: Added a workaround inn 44cc0b8. By delaying the server's pushStream.end(), the client reads the data before closing its stream. I think this bug should be addressed in the context of #51 with a minimal test case, rather than this PR which is focused on server changes.

[sebdeckers]

@mcollina @jasnell Any chance to get this PR reviewed & merged/rejected? Happy to do a Hangout call and discuss ways to collaborate. I'm seeing the API changes in #51 and am happy to update this PR accordingly, either in this PR or after round 11 closes. Thanks!

[jasnell]

This PR will need to be reworked. I've updated the master against the latest core and squashed the numerous http2 related commits to clean things up and get us ready for the next phase of cleanup and adding tests, etc.

[sebdeckers]

@jasnell Great! I'll get to work on it. 🕺

[sebdeckers]

@jasnell I have reworked the code according to the new client API. Also rebased/squashed and then cherry-picked the patch as one commit onto the updated master branch. Hope this is acceptable.

😫 Apologies for losing the individual commits — I struggle with advanced git-fu.

[sebdeckers]

Thanks for the thorough reviews, @jasnell. ☺️ I've cleaned up the linting issue and rebased once more.

[mcollina]

@sebdeckers sorry this conflicted with @yosuke-furukawa change. Would any of you fix things?

[sebdeckers]

@yosuke-furukawa Does the change in 6765d00 work for you? (See also onSessionHeaders)

The stream event is emitted by both server and client for any incoming streams. E.g. server receives a request, or client receives a push request. This is where request headers can be handled.

The push event is now emitted on the client's push request stream. This allows matching of the push response to the corresponding push request.

[mcollina]

Landed as cee5479

[jasnell]

@sebdeckers ... thank you for bearing with us on this. I know it took a while to get landed.

[sebdeckers]

@jasnell I learned a lot by doing this and look forward to making more contributions. My heartfelt thanks to you and @mcollina for all the patience & guidance. 🥂