Reuse allocated port/inspectorPort: 0 ports between setOptions()

Author: mrbbot

packages/miniflare/src/index.ts

@@ -81,6 +81,7 @@ import {
   Service,
   Socket,
   SocketIdentifier,
+  SocketPorts,
   Worker_Binding,
   Worker_Module,
   kInspectorSocket,
@@ -546,14 +547,15 @@ export function _initialiseInstanceRegistry() {
 }
 
 export class Miniflare {
+  #previousSharedOpts?: PluginSharedOptions;
   #sharedOpts: PluginSharedOptions;
   #workerOpts: PluginWorkerOptions[];
   #log: Log;
 
   readonly #runtime?: Runtime;
   readonly #removeRuntimeExitHook?: () => void;
   #runtimeEntryURL?: URL;
-  #socketPorts?: Map<SocketIdentifier, number>;
+  #socketPorts?: SocketPorts;
   #runtimeClient?: Client;
   #proxyClient?: ProxyClient;
 
@@ -877,6 +879,21 @@ export class Miniflare {
     });
   }
 
+  #getSocketAddress(
+    id: SocketIdentifier,
+    previousRequestedPort: number | undefined,
+    host = DEFAULT_HOST,
+    requestedPort?: number
+  ) {
+    // If `port` is set to `0`, was previously set to `0`, and we previously had
+    // a port for this socket, reuse that random port
+    if (requestedPort === 0 && previousRequestedPort === 0) {
+      requestedPort = this.#socketPorts?.get(id);
+    }
+    // Otherwise, default to a new random port
+    return `${host}:${requestedPort ?? 0}`;
+  }
+
   async #assembleConfig(loopbackPort: number): Promise<Config> {
     const allWorkerOpts = this.#workerOpts;
     const sharedOpts = this.#sharedOpts;
@@ -967,13 +984,24 @@ export class Miniflare {
 
       // Allow additional sockets to be opened directly to specific workers,
       // bypassing Miniflare's entry worker.
-      let { unsafeDirectHost, unsafeDirectPort } = workerOpts.core;
+      const { unsafeDirectHost, unsafeDirectPort } = workerOpts.core;
       if (unsafeDirectHost !== undefined || unsafeDirectPort !== undefined) {
-        unsafeDirectHost ??= DEFAULT_HOST;
-        unsafeDirectPort ??= 0;
+        const name = getDirectSocketName(i);
+        const address = this.#getSocketAddress(
+          name,
+          // We don't attempt to reuse allocated ports for `unsafeDirectPort: 0`
+          // as there's not always a clear mapping between current/previous
+          // worker options. We could do it by index, names, script, etc.
+          // This is an unsafe option primarily intended for Wrangler's
+          // inspector proxy, which will usually set this value to `9229`.
+          // We could consider changing this in the future.
+          /* previousRequestedPort */ undefined,
+          unsafeDirectHost,
+          unsafeDirectPort
+        );
         sockets.push({
-          name: getDirectSocketName(i),
-          address: `${unsafeDirectHost}:${unsafeDirectPort}`,
+          name,
+          address,
           service: { name: getUserServiceName(workerName) },
           http: {},
         });
@@ -1059,13 +1087,27 @@ export class Miniflare {
     const host = this.#sharedOpts.core.host ?? DEFAULT_HOST;
     const urlSafeHost = getURLSafeHost(host);
     const accessibleHost = getAccessibleHost(host);
+    const entryAddress = this.#getSocketAddress(
+      SOCKET_ENTRY,
+      this.#previousSharedOpts?.core.port,
+      host,
+      this.#sharedOpts.core.port
+    );
+    let inspectorAddress: string | undefined;
+    if (this.#sharedOpts.core.inspectorPort !== undefined) {
+      inspectorAddress = this.#getSocketAddress(
+        kInspectorSocket,
+        this.#previousSharedOpts?.core.inspectorPort,
+        "localhost",
+        this.#sharedOpts.core.inspectorPort
+      );
+    }
     const runtimeOpts: Abortable & RuntimeOptions = {
       signal: this.#disposeController.signal,
-      entryHost: urlSafeHost,
-      entryPort: this.#sharedOpts.core.port ?? 0,
+      entryAddress,
       loopbackPort,
       requiredSockets,
-      inspectorPort: this.#sharedOpts.core.inspectorPort,
+      inspectorAddress,
       verbose: this.#sharedOpts.core.verbose,
     };
     const maybeSocketPorts = await this.#runtime.updateConfig(
@@ -1223,6 +1265,7 @@ export class Miniflare {
 
     // Split and validate options
     const [sharedOpts, workerOpts] = validateOptions(opts);
+    this.#previousSharedOpts = this.#sharedOpts;
     this.#sharedOpts = sharedOpts;
     this.#workerOpts = workerOpts;
     this.#log = this.#sharedOpts.core.log ?? this.#log;

packages/miniflare/src/runtime/index.ts

@@ -25,20 +25,20 @@ const ControlMessageSchema = z.discriminatedUnion("event", [
 
 export const kInspectorSocket = Symbol("kInspectorSocket");
 export type SocketIdentifier = string | typeof kInspectorSocket;
+export type SocketPorts = Map<SocketIdentifier, number /* port */>;
 
 export interface RuntimeOptions {
-  entryHost: string;
-  entryPort: number;
+  entryAddress: string;
   loopbackPort: number;
   requiredSockets: SocketIdentifier[];
-  inspectorPort?: number;
+  inspectorAddress?: string;
   verbose?: boolean;
 }
 
 async function waitForPorts(
   stream: Readable,
   options: Abortable & Pick<RuntimeOptions, "requiredSockets">
-): Promise<Map<SocketIdentifier, number> | undefined> {
+): Promise<SocketPorts | undefined> {
   if (options?.signal?.aborted) return;
   const lines = rl.createInterface(stream);
   // Calling `close()` will end the async iterator below and return undefined
@@ -102,16 +102,16 @@ function getRuntimeArgs(options: RuntimeOptions) {
     // Required to use compatibility flags without a default-on date,
     // (e.g. "streams_enable_constructors"), see https://github.com/cloudflare/workerd/pull/21
     "--experimental",
-    `--socket-addr=${SOCKET_ENTRY}=${options.entryHost}:${options.entryPort}`,
+    `--socket-addr=${SOCKET_ENTRY}=${options.entryAddress}`,
     `--external-addr=${SERVICE_LOOPBACK}=localhost:${options.loopbackPort}`,
     // Configure extra pipe for receiving control messages (e.g. when ready)
     "--control-fd=3",
     // Read config from stdin
     "-",
   ];
-  if (options.inspectorPort !== undefined) {
+  if (options.inspectorAddress !== undefined) {
     // Required to enable the V8 inspector
-    args.push(`--inspector-addr=localhost:${options.inspectorPort}`);
+    args.push(`--inspector-addr=${options.inspectorAddress}`);
   }
   if (options.verbose) {
     args.push("--verbose");
@@ -127,7 +127,7 @@ export class Runtime {
   async updateConfig(
     configBuffer: Buffer,
     options: Abortable & RuntimeOptions
-  ): Promise<Map<SocketIdentifier, number /* port */> | undefined> {
+  ): Promise<SocketPorts | undefined> {
     // 1. Stop existing process (if any) and wait for exit
     await this.dispose();
     // TODO: what happens if runtime crashes?

packages/miniflare/test/index.spec.ts

@@ -130,6 +130,7 @@ test("Miniflare: setOptions: can update host/port", async (t) => {
 
   const opts: MiniflareOptions = {
     port: 0,
+    inspectorPort: 0,
     liveReload: true,
     script: `addEventListener("fetch", (event) => {
       event.respondWith(new Response("<p>👋</p>", {
@@ -140,24 +141,38 @@ test("Miniflare: setOptions: can update host/port", async (t) => {
   const mf = new Miniflare(opts);
   t.teardown(() => mf.dispose());
 
-  const initialURL = await mf.ready;
-  let res = await mf.dispatchFetch("http://localhost");
-  const initialLoopbackPort = loopbackPortRegexp.exec(await res.text())?.[1];
+  async function getState() {
+    const url = await mf.ready;
+    const inspectorUrl = await mf.getInspectorURL();
+    const res = await mf.dispatchFetch("http://localhost");
+    const loopbackPort = loopbackPortRegexp.exec(await res.text())?.[1];
+    return { url, inspectorUrl, loopbackPort };
+  }
 
+  const state1 = await getState();
   opts.host = "0.0.0.0";
   await mf.setOptions(opts);
-  const updatedURL = await mf.ready;
-  res = await mf.dispatchFetch("http://localhost");
-  const updatedLoopbackPort = loopbackPortRegexp.exec(await res.text())?.[1];
+  const state2 = await getState();
 
-  // Make sure a new port was allocated when `port: 0` was passed to `setOptions()`
-  t.not(initialURL.port, "0");
-  t.not(initialURL.port, updatedURL.port);
+  // Make sure ports were reused when `port: 0` passed to `setOptions()`
+  t.not(state1.url.port, "0");
+  t.is(state1.url.port, state2.url.port);
+  t.not(state1.inspectorUrl.port, "0");
+  t.is(state1.inspectorUrl.port, state2.inspectorUrl.port);
 
   // Make sure updating the host restarted the loopback server
-  t.not(initialLoopbackPort, undefined);
-  t.not(updatedLoopbackPort, undefined);
-  t.not(initialLoopbackPort, updatedLoopbackPort);
+  t.not(state1.loopbackPort, undefined);
+  t.not(state2.loopbackPort, undefined);
+  t.not(state1.loopbackPort, state2.loopbackPort);
+
+  // Make sure setting port to `undefined` always gives a new port, but keeps
+  // existing loopback server
+  opts.port = undefined;
+  await mf.setOptions(opts);
+  const state3 = await getState();
+  t.not(state3.url.port, "0");
+  t.not(state1.url.port, state3.url.port);
+  t.is(state2.loopbackPort, state3.loopbackPort);
 });
 
 test("Miniflare: routes to multiple workers with fallback", async (t) => {