chore: add http2 support

[bjohansebas]

With this, we officially add support for HTTP/2, and it would fix any previous issues with HTTP/2 and unblock middleware and Express itself from supporting HTTP/2, if the failure was due to this module.

Important changes:

• The entire test suite is refactored to have:
  • test/general.js: tests run against both HTTP/1 and HTTP/2 for shared cases that don’t require version-specific changes.
  • test/http1.js: HTTP/1-specific tests that couldn’t be shared in test/general.js.
  • test/http2.js: HTTP/2-specific tests, covering both Node.js’s compatibility layer and the native implementation without the compatibility layer (extracted from Added HTTP/2 support for onFinished and isFinished functions #100, thanks @GroophyLifefor)

Important things to keep in mind with HTTP/2

• HTTP/2 does not have the upgrade event by specification; see RFC 9113, section 8.6
• HTTP/2 and Node.js’s implementation handle the connect event a bit differently. Unlike HTTP/1, it is not finalized immediately, and we don’t have the upgrade property that HTTP/1 had (which indicated at the moment of the connect event that it was already finalized). Here, it is finalized only once the request has read all the data and there’s nothing left to read, as IncomingMessage normally does.
• For Http2ServerResponse, which is the equivalent of ServerResponse in HTTP/1, we only consider the closed property, which will be true if the connection has already been closed. In testing, we could also check if the destroyed property is true, but in our tests both always had the same value, and using closed seems more appropriate.
• For Http2ServerRequest, which is the equivalent of IncomingMessage in HTTP/1, we similarly check if the closed property is true, and we also verify whether it’s no longer receiving data and has read everything it needed to (see chore: add http2 support #87 (comment)).

closes #100 closes #27

[jonchurch]

There's a race condition failing 5 tests. The request is racing the client destroy, so sometimes you'll get ERR_HTTP2_STREAM_CANCEL emitted on the client, and the server will never receive the request. aka the request is aborted before it reaches the server

Moving the destroy into the request handler will ensure they dont race each other:

    it('should fire with error', function (done) {
      var client
      var server = http.createServer(function (req, res) {
        onFinished(res, function (_err) {
          server.close(done)
        })
        // destroy here, inside the handler
        client.destroy()
      })

[bjohansebas]

Okay, I only have two tests left to finish, bring over the tests from #100, add some documentation, and update the PR description.

[bjohansebas]

I’ve updated the PR description and brought in the changes from #100 (squashed, to include only the tests).

[Phillip9587]

Nice work @bjohansebas 🎉

[bjohansebas]

There will probably be edge cases, but once the version is released they could be quickly caught in the middleware and in Express itself, since webpack-dev-server no longer uses SPDY, and we’ll know how HTTP/2 behaves in Express with that package.

[Phillip9587]

LGTM

[bjohansebas]

Okay, I think I found another edge case. I’m testing this with webpack-dev-server, and it looks like the failure comes from here, or possibly from finalhandler, I’m not sure, but I’m pretty confident it originates here. I don’t have a standalone example yet, since I’m testing it directly through webpack-dev-server tests.

edit: Okay, no, I didn’t find an edge case. I think it’s more about how properties are inherited. The thing is that by the time it reaches on-finished, neither the stream property nor the socket property exists. It’s weird and seems to be related to Express.

Confirmed: it’s Express. In https://github.com/expressjs/express/blob/a479419b16f5b97eb20f5dbae5848708ff30ce2d/lib/application.js#L169, setPrototypeOf causes neither stream nor socket to be accessibles

[UlisesGascon]

Great effort @bjohansebas! 👏 👏 👏

[UlisesGascon]

are we sure that this is considered as major and not minor? based on the changes on the index.js IMO is minor

[bjohansebas]

It’s more of a minor change, the major change was in #56. The logic of this pr could even be backported to the current line (HTTP/2 won’t work perfectly, but for isFinished it will, while for onFinished we won’t know)