mjackson / remix-the-web

Open source tools for Remix (or any framework!)
MIT License
723 stars 15 forks source link

Thoughts on a Web APIs (fetch) based http server (replacing `node:http`) #18

Open cbnsndwch opened 2 months ago

cbnsndwch commented 2 months ago

After this tweet from @mjackson:

I started thinking about what it would take to actually replace node:http with an http server that directly implements Web APIs as described in README.md:

What exactly we mean by "web standards"? It means we use:

The Web Streams API instead of Node.js streams Uint8Array instead of Node.js Buffers The Web Crypto API instead of the Node.js crypto library Blob and File instead of some bespoke runtime-specific API

Took a deep dive into the source code of node:http and node:net to see what we would be working on and here are my thoughts:

No soup for you!

The files that make up node:http rely heavily on Node internal objects and functions that are not available to userland JS.

Replicating the functionality of node:http while still relying on node:net is then gonna require adding a new built-in module to Node and getting it merged. It won't be possible directly from userland

Enter the multi-(language)-verse

One alternative I thought of is building a native addon that bundles a different http server and exposes a Web API-based interface to JS, while staying as close to node:http as possible:

There are viable options in Rust (https://hyper.rs/, https://actix.rs/) and in Go (https://gofiber.io/, https://github.com/go-chi/chi, https://gorilla.github.io/) that can be compiled as Node native modules.

Encore.ts seems to be doing something like this but they've gone the "batteries included" route where the value prop only makes sense if you switch your entire runtime and app architecture to their library. Ideally whatever I would build would only aim to provide an alternative to node:http and not become an entire stack.

This path would be a lot of work so before I keep going down the rabbit hole your feedback would be greatly appreciated 🤓

cbnsndwch commented 2 months ago

PS: I know I just did the entire opposite of what the tweet said but I think there may be something worth exploring here. Sorry @mjackson 😁

mjackson commented 2 months ago

Sorry for the slowness on my reply here. I told you I didn't want to get sucked into this! 😅

Having said that, the path I was thinking about going was to use node:net together with milo for HTTP parsing. All server logic could be written in JavaScript/TypeScript.

I'd personally prefer to have the core server logic written in JavaScript/TypeScript (instead of e.g. Rust) to make it a little easier for JS devs to contribute.

I think there may be something worth exploring here

Yes, I definitely agree there is something worth exploring here. My current focus is making Remix and tools for Remix at the application layer that work with any JS runtime (Node.js, Deno, Bun, etc.), and we already have a thin wrapper around node:http that gives us good enough performance on Node.js, so I'm not very inclined to work on this myself. But if you've got the itch to contribute and you'd like to see something happen here I'd be happy to act as an advisor (and maybe even contribute some code as needed). But I'd need you to be the main driver.

cbnsndwch commented 2 months ago

Makes sense! Will take a look and post back

cbnsndwch commented 1 month ago

Oooh boy, it's been quite a journey so far haha!

Stopping over here to say I'm still looking into it every chance that I get. Keep the seat warm, if you will. Here's what I've learnt so far:

mjackson commented 1 month ago

Thanks for the report! I know very little about WASM, but it makes sense to me that there would be a perf hit there. AFAIK they are planning on using milo in node core sometime soon, but they're almost certainly not planning on using the WASM version.

Would it be worth it to do a spike with the WASM version just to get the ball rolling?

cbnsndwch commented 1 month ago

Did some more digging into node:http to try and suss out what else besides the parser would be needed and found out that it actually allows overriding the IncomingRequest constructor the server uses 🤓

That gave and idea of simplifying the adapter pattern you've got going on from IcomingMessage to Request

Just by skipping a lot of checks undici makes when instantiating a Request object that don't make sense for this use case (e.g. because the body would always be a stream) I was able to get about 5% more req/s than the base implementation here

My hypothesis is that by giving node:http an optimized constructor that fulfills undici's interface I can beat the typical perf from Express by avoiding all the header looping and the double copying of body stream, without having to get down to the native level.

Seems promising, will test some more and come back with results

mjackson commented 2 weeks ago

it actually allows overriding the IncomingRequest constructor the server uses

Just to make sure I understand... are you saying I can tell node:http how to construct request objects so I don't need to use createRequest in node-fetch-server?

cbnsndwch commented 2 weeks ago

Yep! Check out the option object accepted by http.createServer. It takes optional IncomingMessage and ServerResponse constructors that can be used to replace the default implementations:

https://nodejs.org/docs/latest-v20.x/api/http.html#httpcreateserveroptions-requestlistener

It feels like a hack but by passing in a class that extends IncomingMessage and that also matches the Request interface it could be used in a fetch-compliant listener.

ServerResponse is a different beast though, because node:http will construct it upfront and expect headers and body to be written to it so listeners that return new Response(...) would still need the copy-out mechanism

cbnsndwch commented 2 days ago

Welp definitely confirmed that passing in a custom request constructor to createServer does work, and that it avoids copying over the request data

Perf is still not great which may be due to still needing to copy the response back to the ServerResponse from node.

Will try to add tracking to different parts of the listener to confirm