-
Notifications
You must be signed in to change notification settings - Fork 31
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. Weβll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: http2 support #106
base: main
Are you sure you want to change the base?
feat: http2 support #106
Conversation
test/index.test.ts
Outdated
}); | ||
|
||
// see https://http2.github.io/faq/#does-http2-require-encryption | ||
test("listen (http2)", async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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! |
945bec7
to
164aa2e
Compare
Just rebased the main branch for better mergability |
84c4880
to
b064d7c
Compare
Hi @pi0. can you check this? It seems to work well enough to consider merging the code π. |
b064d7c
to
ea19891
Compare
Just rebased main another time |
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. |
e6b2868
to
187216b
Compare
@joshmossas the newest commit adds a HTTP 1.x and HTTP 2 (both with TLS encryption) are working fine. When starting listhen with HTTP 1.x and the 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 The Commit also introduces a new differentiation in the
|
279fbcb
to
8373424
Compare
8373424
to
129a3f8
Compare
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:
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 I also added |
@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 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 ReportAttention: Patch coverage is
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. |
b8b1608
to
35fe367
Compare
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 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 The other approach I looked at was to use the 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: It's also worth noting that in addition to creating and emitting an |
With the recent websocket support passing both # 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 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. |
The The With How to handle the |
π Linked issue
Closes #10
β Type of 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
andhttp2
request and response.Since the
NodeListener
interface fromunjs/h3
is for HTTP1.x only, it is defined indev.ts
. Maybe theNodeListener
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