Hybrid node:http + fetch approach to node-fetch-server #33

cbnsndwch · 2024-11-27T02:23:31Z

NOTE: all changes in this PR are non-breaking and implemented as separate, parallel exports except for:

added getter for the Host header to @mjackson/headers
updated platform field in packages/node-fetch-server/tsup.config.ts to 'node
implemented a custom IncomingMessage class that a node:http server can use as-is, and that implements the Request interface from undici-types
5-10% faster than the base implementation in node-fetch-server
15-20% faster than Express
added some basic unit tests to check parsing of sample requests works

…tch-hybrid-server

…n benchmark runner

… and hybrid FetchIncomingMessage constructpor for node:http

mjackson

I think this is a good start, and the results from the benchmark are encouraging! I requested a few changes.

mjackson · 2024-12-18T01:25:40Z

packages/node-fetch-server/src/lib/fetch-incoming-message/index.ts

+  /**
+   * **DO NOT USE**, will always throw. Kept only for compatibility with type definitions.
+   *
+   * @deprecated Use `Request.formDataAsync()` instead which returns an `AsyncGenerator<MultipartPart>`


Where is Request.formDataAsync defined? Is that a new spec? Or are you using the form-data-parser package?

I'm aiming to use the other packages in the repo as much as possible overall so yes, the goal is to use form-data-parser.

I think I meant to use it here but left it for a future iteration. I'd like to confirm this first though, as simplifying the Request model in use would allow me to handle this differently:

#33 (comment)

mjackson · 2024-12-18T01:27:02Z

packages/node-fetch-server/src/lib/fetch-incoming-message/index.ts

+    // const ac = new AbortController();
+    // this.on('close', () => {
+    //   ac.abort();
+    // });
+
+    // let signal = ac.signal;
+    // this.#signal = ac.signal;
+
+    // if (signal.aborted) {
+    //   ac.abort(signal.reason);
+    // } else {
+    //   // Keep a strong ref to ac while request object
+    //   // is alive. This is needed to prevent AbortController
+    //   // from being prematurely garbage collected.
+    //   // See, https://github.com/nodejs/undici/issues/1926.
+    //   this[kAbortController] = ac;
+    // }


Is this supposed to be uncommented? Seems like it, otherwise request.signal will be undefined.

There's A LOT of code in Undici that doesn't make sense in a server setting because fetch is a browser-centric spec. I may have been too liberal in cutting things out, will double check.

QQ: would using the actual Request class from Undici be a hard requirement or explicit goal here? There's further perf improvements to be had by using plain objects or a simpler class.

A non exhaustive list:

AbortController / Signals

AbortController, AbortSignal, finalization logic, dependentControllerMap, buildAbort, etc.

❓Under which circumstances would server-side code need to abort an incoming request?

Referrer / ReferrerPolicy / Mode / Credentials / Cache / Redirect / Integrity / Keepalive

teh implementation in the PR builds an instance of SuperHeaders from parser events. The additional logic in Undici is client-side specific.

Environment Settings Objects / sameOrigin Checks

Again, these are client or spec-level concerns not relevant on the server.

makeRequest(), cloneRequest(), fromInnerRequest()

Methods used internally by Undici to construct new Request objects with spec-defined state.

the implementation in the PR builds incoming requests from events emitted by the node:http parser, not from a “clone” or “init” object.

webidl.converters.RequestInfo / webidl.converters.RequestInit

similar as previous point, request is built bit by bit from parser events

mjackson · 2024-12-18T01:32:10Z

packages/node-fetch-server/src/lib/create-fetch-server.ts

+  return http.createServer<IncomingMessage>(
+    { IncomingMessage },


Interesting! I had no idea the createServer API could use a custom IncomingMessage class. Very cool find 👍

Thanks! Perusing the source code taught me a bunch of interesting bits about Node internals 😁

mjackson · 2024-12-18T01:34:44Z

packages/node-fetch-server/package.json

  },
  "scripts": {
    "bench": "bash ./bench/runner.sh",
    "build": "tsup",
-    "test": "node --import @swc-node/register/esm-register --test ./src/**/*.spec.ts",
+    "debug": "node --import @swc-node/register/esm-register ./src/test.ts",


It looks like ./src/test.ts is missing?

mjackson · 2024-12-18T01:37:21Z

packages/node-fetch-server/src/node-fetch-server.ts

+
+// hybrid magic
+export { FetchIncomingMessage } from './lib/fetch-incoming-message/index.js';
+export { createFetchServer } from './lib/create-fetch-server.js';


Note: Ideally instead of creating two APIs we'd stick with a single API, createRequestListenener. Although I understand that you're exporting createFetchServer here under a different name so you can benchmark them both together.

Exactly, and to allow iteration without breaking the existing implementation. The ultimate goal is of course to merge it into a single export once ready

mjackson · 2024-12-18T01:38:09Z

packages/headers/src/lib/super-headers.ts

+  get host() {
+    return this.get('host');
+  }


Can we please put this in a separate PR, and add a setter as well? Thanks 🙏

I think you've made this not needed in a recent release. I can remove this from the PR in favor of what's in the headers package now.

cbnsndwch added 6 commits November 24, 2024 15:28

feat(headers): add explicit getter for Host header to support node-fe…

f95e069

…tch-hybrid-server

ci(node-fetch-server): allow up to 5 seconds for server to start up i…

fc1edf6

…n benchmark runner

feat(node-fetch-server): [EXPERIMENTAL] add createFetchServer factory…

9d7ab01

… and hybrid FetchIncomingMessage constructpor for node:http

ci(node-fetch-server): update tsup config to target node explicitly

fc13bdb

perf(node-fetch-server): add bench entry for createFetchServer

63b0090

test(node-fetch-server): add some more tests for FetchIncomingMessage

153540b

cbnsndwch mentioned this pull request Nov 27, 2024

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

Open

mjackson force-pushed the main branch from 7500c3d to f2bf387 Compare December 4, 2024 00:15

mjackson requested changes Dec 18, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Hybrid node:http + fetch approach to node-fetch-server #33

Hybrid node:http + fetch approach to node-fetch-server #33

cbnsndwch commented Nov 27, 2024

mjackson left a comment

mjackson Dec 18, 2024

cbnsndwch Jan 11, 2025

mjackson Dec 18, 2024

cbnsndwch Jan 11, 2025 •

edited

Loading

cbnsndwch Jan 11, 2025

mjackson Dec 18, 2024

cbnsndwch Jan 11, 2025

mjackson Dec 18, 2024

mjackson Dec 18, 2024

cbnsndwch Jan 11, 2025

mjackson Dec 18, 2024

cbnsndwch Jan 11, 2025 •

edited

Loading

		return http.createServer<IncomingMessage>(
		{ IncomingMessage },

Hybrid node:http + fetch approach to node-fetch-server #33

Are you sure you want to change the base?

Hybrid node:http + fetch approach to node-fetch-server #33

Conversation

cbnsndwch commented Nov 27, 2024

mjackson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cbnsndwch Jan 11, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cbnsndwch Jan 11, 2025 • edited Loading

Choose a reason for hiding this comment

cbnsndwch Jan 11, 2025 •

edited

Loading

cbnsndwch Jan 11, 2025 •

edited

Loading