feat: http2 support

[Mastercuber]

🔗 Linked issue

Closes #10

❓ Type of change

• 📖 Documentation (updates to the documentation, readme, or JSdoc annotations)
• 🐞 Bug fix (a non-breaking change that fixes an issue)
• 👌 Enhancement (improving an existing functionality like performance)
• ✨ New feature (a non-breaking change that adds functionality)
• 🧹 Chore (updates to the build process or auxiliary tools and libraries)
• ⚠️ Breaking change (fix or feature that would cause existing functionality to change)

📚 Description

Here is a node.js native http2 support using the http2 module.

RequestListener's are now self defined since it seems, there is no type/interface including both http and http2 request and response.

Since the NodeListener interface from unjs/h3 is for HTTP1.x only, it is defined in dev.ts. Maybe the NodeListener type should be moved to h3.

The secureServer is started with http1 enabled, so it's possible to connect to the server with http1.x and http2. This is shown by the added test.

It isn't possible to connect to a non encrypted HTTP2 server (see HTTP2 FAQ). This is also shown by a test.

Also a server type is created.

📝 Checklist

• I have linked an issue or discussion.
• I have updated the documentation accordingly.

[Mastercuber]

This test throws the 2 unhandled errors, though all tests - including this one - are successfully executed. I don't know how to handle this. The test is not needed, but will show, that a http2 server (without TLS) is throwing an error (Protocol Error) when requesting a resource

[Mastercuber]

The CI check shows another error (Promise resolves instead of rejects). Locally the promise rejects, but the 2 errors still listed at the end of the test run

[Mastercuber]

This (closed) issue describes the async throws error.

Now the assertion is like in this comment, means: No extra anonymous arrow function as argument to expect, an await in front of it and behind a rejects, since sendHttp2Request returns a promise and is therefore an async function.

I've also tried to catch the error manually - without success.

[pi0]

Hi. Sorry for checking on this late. I need little bit more time to be able focus and properly review and release this (amazing) PR!

[Mastercuber]

Just rebased the main branch for better mergability

[harrytran998]

Hi @pi0. can you check this? It seems to work well enough to consider merging the code 😆.

[Mastercuber]

Just rebased main another time

[joshmossas]

Hey I've been testing out these changes and I have a couple suggestions. To start, I think http2 is something developers should be able to manually toggle on/off. I also don't think it should be automatically switched on if https options are present.

The first reason being is that there are incompatibilities when going from http1.1 to http2. One such example is the Transfer-Encoding header which will work fine http1.1 but is disallowed in http2. If listhen auto switches to http2 when using SSL this is the kind of bug that would work fine on a devs machine and then create issues when actually deployed to a production server.

Additionally even though browsers force you to use SSL when using http2 there are actually use cases for using http2 without encryption. For example gRPC requires http2 but doesn't require encryption. So it's pretty common to send unencrypted http2 traffic between gRPC services especially in dev environments. Also there are some services that require you to send unencrypted traffic from your server when using http2. Google Cloud Run is one example. When enabling http2 support in Cloud Run your server must handle requests in HTTP/2 cleartext.

[Mastercuber]

@joshmossas the newest commit adds a --http2 switch.

HTTP 1.x and HTTP 2 (both with TLS encryption) are working fine. When starting listhen with HTTP 1.x and the http module, there is no problem; but when using HTTP 2, a HTTP 1.x Client can communicate with the Server, but not a HTTP 2 Client. When requesting with an HTTP 2 client, the server is responding with HTTP Version 0.9...

If you know how to handle it, to create a HTTP 2 Server without TLS encryption and be able to connect with a HTTP 1.x and HTTP 2 client (with the appropriate HTTP Versions), feel free to make these changes.

This was also the reason, why the Server type in src/types.ts had no Http2Server added to union; since the combination http2 (unencrypted) and HTTP 2 Client gives a HTTP 0.9 answer.

The Commit also introduces a new differentiation in the index.test.ts file:

• listen (http2): http1 client
• listen (http2): http2 client

[joshmossas]

Hey @Mastercuber thanks so much for making this change. I tested it out and it works for my use case (cleartext http2 from a server to a proxy sitting in front of it). You can actually connect to the unencrypted http2 server with an http2 client but it has to be a client that doesn't make an upgrade request. So basically:

# this doesn't work because it tries to make an upgrade request
curl --http2 http://localhost:3000

# this works
curl --http2-prior-knowledge http://localhost:3000

I did look into making a server that supports both http1.1 and http2 without SSL, but it might be more trouble than it's worth since NodeJS doesn't support upgrading non-encrypted http1.1 out of the box.

From the Node docs:

In order to create a mixed HTTPS and HTTP/2 server, refer to the ALPN negotiation section. Upgrading from non-tls HTTP/1 servers is not supported.

Honestly I think the way you have it now will probably cover the majority of use cases.

That being said I did get a basic implementation working, which you can view in my fork at the "http2-support" branch (https://github.com/joshmossas/listhen/tree/http2-support). It doesn't support upgrade requests yet as I haven't quite figured out how to handle it properly yet, but it does handle both http1.1 and http2.

Basically how it works is you create 3 servers one with node:net, one with node:http, and one with node:http2. Then inside the handler for the node:net server you determine who to pass the connection to.

I also added http2: boolean to ListenOptions in ./src/types.ts

[Mastercuber]

@joshmossas I have incorporated your changes into the PR. Now, on an Upgrade request, the 101 switching protocol header is responded to the client like you can see in the screenshot below:

The problem is, that curl hangs for me at this moment (doesn't matter if using flushHeaders() or end().

Is this a client handling problem? Since the headers are received by curl.

But your right, it's still a lot. To also be able to upgrade a connection is probably a more optional feature since http2 is working with TLS, and also with plain HTTP and --http2-prior-knowledge.

[codecov]

Codecov Report

Attention: Patch coverage is 70.08547% with 35 lines in your changes are missing coverage. Please review.

Project coverage is 52.67%. Comparing base (d115045) to head (b8b1608).
Report is 14 commits behind head on main.

❗ Current head b8b1608 differs from pull request most recent head f6beab5. Consider uploading reports for the commit f6beab5 to get more accurate results

Files	Patch %	Lines
src/listen.ts	68.88%	28 Missing ⚠️
src/cli.ts	0.00%	6 Missing ⚠️
src/server/dev.ts	87.50%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #106      +/-   ##
==========================================
+ Coverage   49.42%   52.67%   +3.25%     
==========================================
  Files          17       17              
  Lines        1819     1775      -44     
  Branches      147      148       +1     
==========================================
+ Hits          899      935      +36     
+ Misses        915      835      -80     
  Partials        5        5              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[joshmossas]

@Mastercuber

Okay so I spent quite a bit of time trying to figure out how to get it working and I came to the conclusion that it's not worth handling those upgrade requests when it's not https. It's not a client problem, the issue is Node doesn't give us the tools to easily convert a HTTP1.1 request to a HTTP2 request.

From what I can tell we have to take the req and res and convert them to Http2ServerRequest and Http2ServerResponse respectively and then emit the request event to the H2 server after emitting the connection event. So basically:

createHttpServer((req, res) => {
  if (res.headers.upgrade === 'h2c') {
     res.writeHead(101, {
          Upgrade: "HTTP/2",
          Connection: "Upgrade",
        });
      res.flushHeaders();
      h2Server.emit('connection', res.socket);
      // somehow create a `Http2ServerRequest` and `Http2ServerResponse` to pass here
      h2Server.emit('request', ...);
      return;
  }
  ...
})

However, Node doesn't actually provide a way to do this. To start node doesn't export the Http2ServerRequest and Http2ServerResponse classes. It only exports the interfaces so in order to recreated it I would need to re-implement every property and method on both of those classes. I did start doing that just to see if it works and it looks like the request event actually does get emitted and starts being processed, but obviously it couldn't complete since I didn't actually implement the whole interface. (There's also no guarantee that it would actually work if I did re-implement everything haha)

The other approach I looked at was to use the h1Server.on("upgrade") event listener which gives you the IncomingMessage, Duplex, and Buffer for that request. However you run into the same issues as you can't create an Http2ServerRequest from a Duplex either.

h1Server.on('upgrade', (req, socket, head) => {
  socket.write(`HTTP/1.1 101 Switching Protocols\r\nUpgrade: HTTP/2\r\nConnection: Upgrade\r\n\r\n`,);
  socket.pipe(socket);
  h2Server.emit('connection', socket);
  // still no way to create a `Http2ServerRequest` or `Http2ServerResponse`
  h2Server.emit('request', ...);
})

I may be overlooking something as there might be a better way to do this, but my current impression is that any successful implementation would be hacky and brittle, so it wouldn't be worth implementing into listhen.

Related issue:
nodejs/node#46152

It's also worth noting that in addition to creating and emitting an Http2ServerRequest we may also need to create and emit an Http2Session to get things to actually work properly, but I haven't actually gotten that far - so that's just speculation.

[joshmossas]

With the recent websocket support passing both --ws and --http2 causes websocket connections to fail. It only works if you also have --https added as well. So basically:

# websockets don't work
listhen . --ws --http2

# websockets work
listhen . --ws --http2 --https

I've made some changes here, which fixes the issue. I'll outline the changes below.

Basically the server created by createRawTcpIpcServer never fires an "upgrade" event. Since we register that raw server to the server variable server.on('upgrade') never fires for the websocket connection. In order to get around this we need to save the http1Server to a variable that we can access when registering the upgrade handler later.

The block where we create the http1 and http2 servers

const h1Server = createHttpServer(handle as RequestListenerHttp1x);
const h2Server = createHttp2Server(handle as RequestListenerHttp2);
server = createRawTcpIpcServer(async (socket) => {
  const chunk = await new Promise((resolve) =>
    socket.once("data", resolve),
  );
  // @ts-expect-error
  socket._readableState.flowing = undefined;
  socket.unshift(chunk);
  if ((chunk as any).toString("utf8", 0, 3) === "PRI") {
    h2Server.emit("connection", socket);
    return;
  }
  h1Server.emit("connection", socket);
});

// store this http1 server to a variable we can access later
wsTargetServer = h1Server;

addShutdown(server);
await bind();

The block where we register the web-socket upgrade handlers

 // --- WebSocket ---
if (listhenOptions.ws) {
  if (typeof listhenOptions.ws === "function") {
    // Check if the variable has been assigned. If not register upgrade handler on the default server.
    if (wsTargetServer) {
      wsTargetServer.on("upgrade", listhenOptions.ws);
    } else {
      server.on("upgrade", listhenOptions.ws);
    }
  } else {
    consola.warn(
      "[listhen] Using experimental websocket API. Learn more: `https://crossws.unjs.io`",
    );
    const nodeWSAdapter = await import("crossws/adapters/node").then(
      (r) => r.default || r,
    );
    const { handleUpgrade } = (nodeWSAdapter as any)({
      ...(listhenOptions.ws as CrossWSOptions<any, any>),
    });
    // Check if the variable has been assigned. If not register upgrade handler on the default server.
    if (wsTargetServer) {
      wsTargetServer.on("upgrade", handleUpgrade);
    } else {
      server.on("upgrade", handleUpgrade);
    }
  }
}

Anywho that's my quick solution. We can prolly clean it up a bit, but now we know how to handle this issue.

[Mastercuber]

The ws:// version is now working, also demonstrated with an example at /ws. Thanks for that @joshmossas!

The wss:// versions (tls enabled) seems not to work.
What I can see crossws and the provided nodejs adapter is used in conjunction with ws. Since here noServer is passed to the WebSocketServer from ws, it is not possible to pass a server as argument from listhen (either noServer or server, not both).

With noServer this is the way to go, what should be abstracted away in crossws with the handleUpgrade like you have incorporated.

How to handle the wss:// correctly?