implement node:http server-side modules #4591

anonrig · 2025-07-22T18:26:09Z

Implements the remaining modules necessary to add http.createServer. Starting from today, the following code works without any issues:

import http from 'node:http';
import { strictEqual, ok } from 'node:assert';
import { nodeCompatHttpServerHandler } from 'cloudflare:workers';

export const testHttpServerMultiHeaders = {
  async test(_ctrl, env) {
    const { promise, resolve } = Promise.withResolvers();
    const server = http.createServer(function (req, res) {
      strictEqual(req.headers.host, 'yagiz');
      res.writeHead(200, { 'Content-Type': 'text/plain' });
      res.end('EOF');

      server.close();
    });

    server.listen(8080, async function () {
      const res = await env.SERVICE.fetch('https://cloudflare.com', {
        headers: [['host', 'yagiz']],
      });

      strictEqual(res.status, 200);
      strictEqual(res.headers.get('content-type'), 'text/plain');
      strictEqual(await res.text(), 'EOF');

      resolve();
    });

    await promise;
  },
};

export default nodeCompatHttpServerHandler({ port: 8080 });

Before landing this pull-request the following todos are required:

Add remaining http-server tests
Add node:https as well

src/cloudflare/workers.ts

src/node/internal/internal_http_incoming.ts

src/node/internal/internal_http_server.ts

src/workerd/api/node/node.h

src/cloudflare/workers.ts

src/workerd/api/node/tests/http-server-nodejs-test.js

jasnell

Good first pass. The server API should be gated behind an experimental compat flag that is separate from the one used to enable the http client APIs so we can roll those out separately. We likely won't be able to lift the experimental flag off these until more of the test coverage is filled in.

More importantly, I don't think the handling of the response stream here is adequate. I left comments explaining why, but the way it's structured I think there's far too much internal buffering of the data as opposed to streaming it out as it is written. I think that needs to be addressed before this can land.

src/node/internal/internal_http_server.ts

src/workerd/api/node/tests/http-server-nodejs-test.js

anonrig · 2025-07-25T21:23:05Z

@kentonv I've updated the public API as your recommendation (nodeCompatHttpServerHandler). just wanted to share before we land this next week.

jasnell · 2025-07-25T21:55:04Z

src/cloudflare/workers.ts

+    );
+  }
+  return {
+    async fetch(request: Request): Promise<Response> {


I would include a comment in here about the env and ctx arguments are intentionally omitted here and the expectation, at least for now, is to use importable env and importable waitUntil.

jasnell · 2025-07-25T21:56:02Z

src/cloudflare/workers.ts

+      const instance = portMapper.get(port);
+      if (!instance) {
+        throw new Error(
+          `Http server with port ${port} not found. This is likely a bug with your code.`


Some indication on how to fix this in the message would be nice. Something about referencing server.listen(...) or a link to docs.

jasnell · 2025-07-25T21:57:40Z

src/node/internal/compatibility-flags.d.ts

@@ -31,3 +31,6 @@ export const workerdExperimental: boolean;
 export const durableObjectGetExisting: boolean;
 export const vectorizeQueryMetadataOptional: boolean;
 export const nodeJsZlib: boolean;
+export const enableNodejsProcessV2: boolean;
+export const enableNodejsHttpServerModules: boolean;
+export const jsWeakRef: boolean;


fwiw, now that globalThis.Cloudflare exists, this is largely obsolete and can probably be replaced. Not critical for this PR tho.

danlapid · 2025-07-25T22:08:01Z

src/cloudflare/internal/http.ts

@@ -0,0 +1,4 @@
+export interface Fetcher {
+  fetch: (request: Request) => Promise<Response>;


why not

Suggested change

fetch: (request: Request) => Promise<Response>;

fetch: typeof fetch;

?

jasnell · 2025-07-25T22:38:52Z

src/node/internal/internal_http_incoming.ts

    }
+
+    this.#tryRead().catch(() => {
+      /* Ignore errors */


Why ignore errors? Specifically, from what I can determine tryRead() does not actually need to throw anything.

jasnell · 2025-07-25T22:44:00Z

src/node/internal/internal_http_incoming.ts

-          break;
-        }
-        this.push(value);
+      const data = await this.#reader.read();


I'd move the getReader() call into the try here. getReader() can throw and that should not be ignored and should lead to this being destroyed.

Suggested change

const data = await this.#reader.read();

this.#reader ??= this._stream.getReader();

const data = await this.#reader.read();

jasnell · 2025-07-26T01:33:14Z

src/node/internal/internal_http_server.ts

+    let body = null;
+
+    if (this._hasBody) {
+      body = new ReadableStream<Uint8Array>({


Suggested change

body = new ReadableStream<Uint8Array>({

body = new ReadableStream<Uint8Array>({

type: 'bytes',

jasnell · 2025-07-26T01:34:20Z

src/node/internal/internal_http_server.ts

+          this.on('error', (error) => {
+            controller.error(error);
+          });


Suggested change

this.on('error', (error) => {

controller.error(error);

});

this.once('error', controller.error.bind(controller));

jasnell · 2025-07-26T01:35:03Z

src/node/internal/internal_http_server.ts

+        cancel: (reason: unknown): void => {
+          this.destroy(reason);
+        },


Suggested change

cancel: (reason: unknown): void => {

this.destroy(reason);

},

cancel: this.destroy.bind(this),

jasnell · 2025-07-26T01:38:18Z

src/node/internal/internal_http_server.ts

+      return encoding == null || encoding === 'utf8' || encoding === 'utf-8'
+        ? this.#encoder.encode(data)
+        : Buffer.from(data, encoding);


Why not just use Buffer.from(data, encoding) for UTF8 also?

jasnell · 2025-07-26T01:40:55Z

src/node/internal/internal_http_server.ts

+      statusCode === 204 ||
+      statusCode === 304 ||
+      (statusCode >= 100 && statusCode <= 199)


Suggested change

statusCode === 204 ||

statusCode === 304 ||

(statusCode >= 100 && statusCode <= 199)

statusCode === 204 || statusCode === 304

jasnell · 2025-07-26T01:49:46Z

src/node/internal/internal_http_server.ts

+    this.emit('close');
+    queueMicrotask(() => {
+      callback?.();
+    });


The callback passed into close is registered as a 'close' event handler and the close event should be emitted in the queueMicrotask(...)

Suggested change

this.emit('close');

queueMicrotask(() => {

callback?.();

});

if (typeof callback === 'function') {

this.on('close', callback);

}

queueMicrotask(() => {

this.emit('close');

});

For instance, in Node.js:

jsnell@james-cloudflare-build:~/tmp$ node a after close closed 1 closed 2 jsnell@james-cloudflare-build:~/tmp$ cat a.js const { createServer } = require('node:http'); const server = createServer(); server.on('close', () => console.log('closed 1')); server.listen(8888); server.close(() => console.log('closed 2')); console.log('after close'); jsnell@james-cloudflare-build:~/tmp$

jasnell · 2025-07-26T01:53:44Z

src/node/internal/internal_http_server.ts

+    const { incoming, response } = this.#toReqRes(request);
+    this.emit('connection', this, incoming);
+    this.emit('request', incoming, response);
+    return getServerResponseFetchResponse(response);


Currently, if any errors are thrown synchronously here they will bubble up to the regular fetch handler, erroring the request, which is fine if that's what we want. However, I think we should consider wrapping this in a try/catch and erroring the server.

jasnell · 2025-07-26T01:55:50Z

src/node/internal/internal_http_server.ts

+    }
+    incoming._addHeaderLines(headers, headers.length);
+
+    // TODO(soon): It would be useful if there was a way to expose request.cf properties.


I'd say it's essential, not just useful. We should figure this out before we flip the switch making node:http available. Can be just as simple as adding a new own property to incoming

jasnell · 2025-07-26T01:57:45Z

src/node/internal/internal_http_server.ts

+    if (typeof options.port === 'number' || typeof options.port === 'string') {
+      validatePort(options.port, 'options.port');
+    }
+
+    if (!('port' in options)) {
+      throw new ERR_INVALID_ARG_VALUE(
+        'options',
+        options,
+        'must have the property "port"'
+      );
+    }


validatePort(...) already checks the typeof for the input. This just feels a bit awkward in terms of how these checks are structured.

jasnell · 2025-07-26T01:59:35Z

src/node/internal/internal_http_server.ts

+      );
+    }
+
+    if (this.port != null || portMapper.has(Number(options.port))) {


Suggested change

if (this.port != null || portMapper.has(Number(options.port))) {

if (this.port != null || portMapper.has(+options.port)) {

But again, this feels a bit awkward. validatePort(...) returns the port value converted to a number.

jasnell · 2025-07-26T02:02:36Z

src/node/internal/internal_http_server.ts

+                new Error(
+                  `Content-Length mismatch: wrote ${state.bytesWritten} bytes but Content-Length header was ${state.contentLength}`


Should this be a node.js-style error with a code?

vicb · 2025-07-27T10:20:09Z

src/workerd/api/node/tests/http-server-nodejs-test.wd-test

+          (name = "worker", esModule = embed "http-server-nodejs-test.js")
+        ],
+        compatibilityDate = "2025-01-15",
+        compatibilityFlags = ["nodejs_compat", "experimental", "enable_nodejs_http_modules", "enable_nodejs_http_server_modules"],


For here it looks like both "enable_nodejs_http_modules" and "enable_nodejs_http_server_modules" are required to enable the server.

Could you please clarify that in compatibility-date.capnp where there is no mention to that:

enableNodejsHttpServerModules @103 :Bool $compatEnableFlag("enable_nodejs_http_server_modules") $compatDisableFlag("disable_nodejs_http_server_modules"); # Enables Node.js http server related modules such as node:_http_server

What I would like to see documented:

What would happen if "enable_nodejs_http_server_modules" is used without "enable_nodejs_http_modules" - if that works we should document that.

If that doesn't work and both flags have to be enabled, you could change to say that "enable_nodejs_http_modules" is implied by "enable_nodejs_http_server_modules"

Eventially both of these would have impliedByAfter annotations such that they would just be on when nodejs_compat is enabled... but yeah, some expanded docs about the relationship of these flags is helpful.

vicb · 2025-07-27T10:21:21Z

src/node/internal/internal_https_server.ts

+import type { Server as _Server } from 'node:https';
+
+export class Server extends HttpServer implements _Server {


Suggested change

import type { Server as _Server } from 'node:https';

export class Server extends HttpServer implements _Server {

import type { Server as HttpsServer } from 'node:https';

export class Server extends HttpServer implements HttpsServer {

vicb · 2025-07-27T10:28:21Z

src/node/internal/internal_http_server.ts

+  Server as _Server,
+  ServerResponse as _ServerResponse,


Suggested change

Server as _Server,

ServerResponse as _ServerResponse,

Server as HttpServer,

ServerResponse as HttpServerResponse,

vicb · 2025-07-27T10:33:17Z

src/node/internal/internal_http_server.ts

+// Since finalization registry is only available under a specific compat flag,
+// let's check if it's enabled to preserve backward compatibility.


nits:

"jsWeakRef" is more specific than "a specific"

Not really "backward compatibility" but rather have to do with flags?

Suggested change

// Since finalization registry is only available under a specific compat flag,

// let's check if it's enabled to preserve backward compatibility.

// The finalization registry is only available under the `jsWeakRef` flag

vicb · 2025-07-27T10:51:42Z

src/node/internal/internal_http_server.ts

+      if (key === 'host') {
+        // By default fetch implementation will join "host" header values with a comma.
+        // But in order to be node.js compatible, we need to select the first if possible.
+        headers.push(key, value.split(', ').at(0) as string);


nit

Suggested change

headers.push(key, value.split(', ').at(0) as string);

headers.push(key, value.split(', ', 1).at(0) as string);

vicb · 2025-07-27T10:56:58Z

src/node/internal/internal_http_server.ts

+        // existing conflicts, then use appendHeader.
+
+        for (let n = 0; n < obj.length; n += 2) {
+          k = obj[n + 0];


Suggested change

k = obj[n + 0];

k = obj[n];

anonrig requested review from a team as code owners July 22, 2025 18:26