Reuse primary PHP also for queued requests, reuse secondary PHP insances

JanJakes · JanJakes · commit f8d7d386c857 · 2026-01-29T16:55:12.000+01:00
diff --git a/packages/php-wasm/node/src/test/php-process-manager.spec.ts b/packages/php-wasm/node/src/test/php-process-manager.spec.ts
@@ -73,7 +73,7 @@ describe('PHPProcessManager', () => {
 		);
 	});
 
-	it('should not start a second PHP instance until the first getInstance() call when the primary instance is busy', async () => {
+	it('should reuse idle instances and only spawn when needed', async () => {
 		const phpFactory = vitest.fn(
 			async () => new PHP(await loadNodeRuntime(RecommendedPHPVersion))
 		);
@@ -83,39 +83,45 @@ describe('PHPProcessManager', () => {
 		});
 
 		expect(phpFactory).not.toHaveBeenCalled();
+
+		// First acquire spawns primary instance
 		const php1 = await mgr.acquirePHPInstance();
 		expect(phpFactory).toHaveBeenCalledTimes(1);
 		php1.reap();
 
+		// Second acquire reuses the now-idle primary
 		const php2 = await mgr.acquirePHPInstance();
 		expect(phpFactory).toHaveBeenCalledTimes(1);
 		php2.reap();
 
-		await mgr.acquirePHPInstance();
-		await mgr.acquirePHPInstance();
-		expect(phpFactory).toHaveBeenCalledTimes(3);
+		// Third acquire reuses primary again
+		const php3 = await mgr.acquirePHPInstance();
+		expect(phpFactory).toHaveBeenCalledTimes(1);
+
+		// Fourth acquire needs a new instance (primary is busy)
+		const php4 = await mgr.acquirePHPInstance();
+		expect(phpFactory).toHaveBeenCalledTimes(2);
+
+		php3.reap();
+		php4.reap();
 	});
 
-	it('should refuse to spawn two primary PHP instances', async () => {
+	it('should not spawn duplicate primary instances when called concurrently', async () => {
+		const phpFactory = vitest.fn(
+			async () => new PHP(await loadNodeRuntime(RecommendedPHPVersion))
+		);
 		const mgr = new PHPProcessManager({
-			phpFactory: async () =>
-				new PHP(await loadNodeRuntime(RecommendedPHPVersion)),
+			phpFactory,
 			maxPhpInstances: 5,
 		});
 
-		// A synchronous call. Do not await this promise on purpose.
-		mgr.getPrimaryPhp();
-
-		// No await here, because we want to check if a second,
-		// synchronous call throws an error if issued before
-		// the first call completes asynchronously.
-		try {
-			mgr.getPrimaryPhp();
-		} catch (e) {
-			expect(e).toBeInstanceOf(Error);
-			expect((e as Error).message).toContain(
-				'Requested spawning a primary PHP instance'
-			);
-		}
+		// Call getPrimaryPhp() twice concurrently - both should return the same instance
+		const [php1, php2] = await Promise.all([
+			mgr.getPrimaryPhp(),
+			mgr.getPrimaryPhp(),
+		]);
+
+		expect(php1).toBe(php2);
+		expect(phpFactory).toHaveBeenCalledTimes(1);
 	});
 });
diff --git a/packages/php-wasm/universal/src/lib/php-process-manager.ts b/packages/php-wasm/universal/src/lib/php-process-manager.ts
@@ -10,24 +10,18 @@ export type PHPFactory = (options: PHPFactoryOptions) => Promise<PHP>;
 
 export interface ProcessManagerOptions {
 	/**
-	 * The maximum number of PHP instances that can exist at
+	 * The maximum number of PHP instances that can be in use at
 	 * the same time.
 	 */
 	maxPhpInstances?: number;
 	/**
 	 * The number of milliseconds to wait for a PHP instance when
-	 * we have reached the maximum number of PHP instances and
-	 * cannot spawn a new one. If the timeout is reached, we assume
-	 * all the PHP instances are deadlocked and a throw MaxPhpInstancesError.
+	 * all instances are busy. If the timeout is reached, we assume
+	 * all the PHP instances are deadlocked and throw MaxPhpInstancesError.
 	 *
 	 * Default: 30000
 	 */
 	timeout?: number;
-	/**
-	 * The primary PHP instance that's never killed. This instance
-	 * contains the reference filesystem used by all other PHP instances.
-	 */
-	primaryPhp?: PHP;
 	/**
 	 * A factory function used for spawning new PHP instances.
 	 */
@@ -44,208 +38,117 @@ export class MaxPhpInstancesError extends Error {
 }
 
 /**
- * A PHP Process manager.
- *
- * Maintains:
- * * A single "primary" PHP instance that's never killed – it contains the
- *   reference filesystem used by all other PHP instances.
- * * A pool of disposable PHP instances that are spawned to handle a single
- *   request and reaped immediately after.
+ * A PHP Process manager that maintains a pool of reusable PHP instances.
  *
- * When a new request comes in, PHPProcessManager yields the idle instance to
- * handle it, and immediately starts initializing a new idle instance. In other
- * words, for n concurrent requests, there are at most n+1 PHP instances
- * running at the same time.
+ * Instances are spawned on demand up to `maxPhpInstances` and reused across
+ * requests. The first instance spawned is the "primary" instance which
+ * contains the reference filesystem used by all other instances.
  *
- * A slight nuance is that the first idle instance is not initialized until the
- * first concurrent request comes in. This is because many use-cases won't
- * involve parallel requests and, for those, we can avoid eagerly spinning up a
- * second PHP instance.
- *
- * This strategy is inspired by Cowboy, an Erlang HTTP server. Handling a
- * single extra request can happen immediately, while handling multiple extra
- * requests requires extra time to spin up a few PHP instances. This is a more
- * resource-friendly tradeoff than keeping 5 idle instances at all times.
+ * The semaphore controls how many requests can be processed concurrently.
+ * When all instances are busy, new requests wait in a queue until an
+ * instance becomes available or the timeout is reached.
  */
 export class PHPProcessManager implements PHPInstanceManager {
-	private primaryPhp?: PHP;
-	private primaryPhpPromise?: Promise<AcquiredPHP>;
-	private primaryIdle = true;
-	private nextInstance: Promise<AcquiredPHP> | null = null;
-	/**
-	 * All spawned PHP instances, including the primary PHP instance.
-	 * Used for bookkeeping and reaping all instances on dispose.
-	 */
-	private allInstances: Promise<AcquiredPHP>[] = [];
+	/** All PHP instances that have been spawned. */
+	private instances: PHP[] = [];
+	/** Instances that are currently idle and available for use. */
+	private idleInstances: PHP[] = [];
 	private phpFactory?: PHPFactory;
 	private maxPhpInstances: number;
 	private semaphore: Semaphore;
+	/** Promise guard to prevent spawning multiple primary instances. */
+	private primaryPhpPromise?: Promise<PHP>;
 
 	constructor(options?: ProcessManagerOptions) {
 		this.maxPhpInstances = options?.maxPhpInstances ?? 2;
 		this.phpFactory = options?.phpFactory;
-		this.primaryPhp = options?.primaryPhp;
 		this.semaphore = new Semaphore({
 			concurrency: this.maxPhpInstances,
-			/**
-			 * Wait up to 5 seconds for resources to become available
-			 * before assuming that all the PHP instances are deadlocked.
-			 */
 			timeout: options?.timeout || 30000,
 		});
 	}
 
 	/**
-	 * Get the primary PHP instance.
-	 *
-	 * If the primary PHP instance is not set, it will be spawned
-	 * using the provided phpFactory.
-	 *
-	 * @throws {Error} when called twice before the first call is resolved.
+	 * Get the primary PHP instance (the first one spawned).
+	 * If no instance exists yet, one will be spawned.
 	 */
-	async getPrimaryPhp() {
-		if (!this.phpFactory && !this.primaryPhp) {
-			throw new Error(
-				'phpFactory or primaryPhp must be set before calling getPrimaryPhp().'
-			);
-		} else if (!this.primaryPhp) {
+	async getPrimaryPhp(): Promise<PHP> {
+		if (this.instances.length === 0) {
 			if (!this.primaryPhpPromise) {
-				this.primaryPhpPromise = this.spawn({ isPrimary: true });
+				this.primaryPhpPromise = this.spawnInstance(true);
+				this.primaryPhpPromise.finally(() => {
+					this.primaryPhpPromise = undefined;
+				});
 			}
-			this.primaryPhp = (await this.primaryPhpPromise).php;
-			this.primaryPhpPromise = undefined;
+			await this.primaryPhpPromise;
 		}
-		return this.primaryPhp!;
+		return this.instances[0];
 	}
 
 	/**
-	 * Get a PHP instance.
+	 * Acquire a PHP instance for processing a request.
 	 *
-	 * It could be either the primary PHP instance, an idle disposable PHP
-	 * instance, or a newly spawned PHP instance – depending on the resource
-	 * availability.
+	 * Returns an idle instance from the pool, or spawns a new one if
+	 * the pool isn't at capacity. If all instances are busy, waits
+	 * until one becomes available.
 	 *
-	 * @throws {MaxPhpInstancesError} when the maximum number of PHP instances is reached
-	 *                                and the waiting timeout is exceeded.
+	 * @throws {MaxPhpInstancesError} when the timeout is reached waiting
+	 *                                for an available instance.
 	 */
 	async acquirePHPInstance(): Promise<AcquiredPHP> {
-		/**
-		 * First and foremost, make sure we have the primary PHP instance in place.
-		 * We may not actually acquire it. We just need it to exist.
-		 *
-		 * @TODO: Re-evaluate why we need it to exist. Should spawn() be just more
-		 *        lenient with its "another primary instance already started spawning"
-		 *        check?
-		 */
-		if (!this.primaryPhp) {
-			await this.getPrimaryPhp();
-		}
-
-		if (this.primaryIdle) {
-			this.primaryIdle = false;
-			return {
-				php: await this.getPrimaryPhp(),
-				reap: () => {
-					this.primaryIdle = true;
-				},
-			};
+		let releaseSemaphore: () => void;
+		try {
+			releaseSemaphore = await this.semaphore.acquire();
+		} catch (error) {
+			if (error instanceof AcquireTimeoutError) {
+				throw new MaxPhpInstancesError(this.maxPhpInstances);
+			}
+			throw error;
 		}
 
-		/**
-		 * nextInstance is null:
-		 *
-		 * * Before the first concurrent getInstance() call
-		 * * When the last getInstance() call did not have enough
-		 *   budget left to optimistically start spawning the next
-		 *   instance.
-		 */
-		const acquiredPHP =
-			this.nextInstance || this.spawn({ isPrimary: false });
+		const php = await this.getOrSpawnInstance();
 
-		/**
-		 * Start spawning the next instance if there's still room. We can't
-		 * just always spawn the next instance because spawn() can fail
-		 * asynchronously and then we'll get an unhandled promise rejection.
-		 */
-		if (this.semaphore.remaining > 0) {
-			this.nextInstance = this.spawn({ isPrimary: false });
-		} else {
-			this.nextInstance = null;
-		}
-		return await acquiredPHP;
+		return {
+			php,
+			reap: () => {
+				this.idleInstances.push(php);
+				releaseSemaphore();
+			},
+		};
 	}
 
 	/**
-	 * Initiated spawning of a new PHP instance.
-	 * This function is synchronous on purpose – it needs to synchronously
-	 * add the spawn promise to the allInstances array without waiting
-	 * for PHP to spawn.
+	 * Get an idle instance or spawn a new one if under capacity.
 	 */
-	private spawn(factoryArgs: PHPFactoryOptions): Promise<AcquiredPHP> {
-		if (factoryArgs.isPrimary && this.allInstances.length > 0) {
-			throw new Error(
-				'Requested spawning a primary PHP instance when another primary instance already started spawning.'
-			);
+	private async getOrSpawnInstance(): Promise<PHP> {
+		if (this.idleInstances.length > 0) {
+			return this.idleInstances.pop()!;
 		}
-		const spawned = this.doSpawn(factoryArgs);
-		this.allInstances.push(spawned);
-		const pop = () => {
-			this.allInstances = this.allInstances.filter(
-				(instance) => instance !== spawned
-			);
-		};
-		return spawned
-			.catch((rejection) => {
-				pop();
-				throw rejection;
-			})
-			.then((result) => ({
-				...result,
-				reap: () => {
-					pop();
-					result.reap();
-				},
-			}));
+		if (this.instances.length === 0) {
+			return await this.getPrimaryPhp();
+		}
+		return await this.spawnInstance(false);
 	}
 
 	/**
-	 * Actually acquires the lock and spawns a new PHP instance.
+	 * Spawn a new PHP instance.
 	 */
-	private async doSpawn(
-		factoryArgs: PHPFactoryOptions
-	): Promise<AcquiredPHP> {
-		let release: () => void;
-		try {
-			release = await this.semaphore.acquire();
-		} catch (error) {
-			if (error instanceof AcquireTimeoutError) {
-				throw new MaxPhpInstancesError(this.maxPhpInstances);
-			}
-			throw error;
-		}
-		try {
-			const php = await this.phpFactory!(factoryArgs);
-			return {
-				php,
-				reap() {
-					php.exit();
-					release();
-				},
-			};
-		} catch (e) {
-			release();
-			throw e;
+	private async spawnInstance(isPrimary: boolean): Promise<PHP> {
+		if (!this.phpFactory) {
+			throw new Error(
+				'phpFactory must be set before spawning instances.'
+			);
 		}
+		const php = await this.phpFactory({ isPrimary });
+		this.instances.push(php);
+		return php;
 	}
 
 	async [Symbol.asyncDispose]() {
-		if (this.primaryPhp) {
-			this.primaryPhp.exit();
+		for (const php of this.instances) {
+			php.exit();
 		}
-		await Promise.all(
-			this.allInstances.map((instance) =>
-				instance.then(({ reap }) => reap())
-			)
-		);
+		this.instances = [];
+		this.idleInstances = [];
 	}
 }
diff --git a/packages/playground/wordpress/src/boot.ts b/packages/playground/wordpress/src/boot.ts
@@ -474,7 +474,7 @@ export async function bootRequestHandler(options: BootRequestHandlerOptions) {
 		/**
 		 * If maxPhpInstances is not 1, the PHPRequestHandler constructor needs
 		 * a PHP factory function. Internally, it creates a PHPProcessManager that
-		 * dynamically starts new PHP instances and reaps them after they're used.
+		 * maintains a pool of reusable PHP instances.
 		 */
 		phpFactory:
 			options.maxPhpInstances !== 1
