diff --git a/packages/php-wasm/node/src/test/php-instance-manager.spec.ts b/packages/php-wasm/node/src/test/php-instance-manager.spec.ts index fc3ff2dbce..90efd07b1c 100644 --- a/packages/php-wasm/node/src/test/php-instance-manager.spec.ts +++ b/packages/php-wasm/node/src/test/php-instance-manager.spec.ts @@ -159,7 +159,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)) ); @@ -169,39 +169,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/node/src/test/php-process-manager.spec.ts b/packages/php-wasm/node/src/test/php-process-manager.spec.ts index 64f6d995a7..00ed0d84fc 100644 --- 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 index 12ea8fcf96..aae55b435b 100644 --- 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; 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: 5000 + * 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,127 @@ 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. - * - * 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. + * A PHP Process manager that maintains a pool of reusable PHP 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. + * 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. * - * 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; - private primaryIdle = true; - private nextInstance: Promise | null = null; - /** - * All spawned PHP instances, including the primary PHP instance. - * Used for bookkeeping and reaping all instances on dispose. - */ - private allInstances: Promise[] = []; - private phpFactory?: PHPFactory; + /** All PHP instances that have been spawned. */ + private instances: PHP[] = []; + + /** Instances that are currently idle and available for use. */ + private idleInstances: PHP[] = []; + + /** Maximum number of concurrent PHP instances allowed. */ private maxPhpInstances: number; + + /** Factory function for creating new PHP instances. */ + private phpFactory?: PHPFactory; + + /** Controls concurrent access to PHP instances. */ private semaphore: Semaphore; + /** Prevents spawning duplicate primary instances during concurrent calls. */ + private primaryPhpPromise?: Promise; + 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 || 5000, + 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 and marked as idle. */ - async getPrimaryPhp() { - if (!this.phpFactory && !this.primaryPhp) { - throw new Error( - 'phpFactory or primaryPhp must be set before calling getPrimaryPhp().' - ); - } else if (!this.primaryPhp) { - if (!this.primaryPhpPromise) { - this.primaryPhpPromise = this.spawn({ isPrimary: true }); - } - this.primaryPhp = (await this.primaryPhpPromise).php; + async getPrimaryPhp(): Promise { + if (this.instances.length > 0) { + return this.instances[0]; + } + + if (!this.primaryPhpPromise) { + this.primaryPhpPromise = this.spawnInstance(true); + } + try { + return await this.primaryPhpPromise; + } finally { this.primaryPhpPromise = undefined; } - return this.primaryPhp!; } /** - * 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 { - /** - * 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 }); - - /** - * 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; + const php = await this.getOrSpawnInstance(); + 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. */ - private spawn(factoryArgs: PHPFactoryOptions): Promise { - 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 { + if (this.instances.length === 0) { + await this.getPrimaryPhp(); } - 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.idleInstances.length === 0) { + await this.spawnInstance(false); + } + return this.idleInstances.pop()!; } /** - * Actually acquires the lock and spawns a new PHP instance. + * Spawn a new PHP instance. */ - private async doSpawn( - factoryArgs: PHPFactoryOptions - ): Promise { - 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 { + if (!this.phpFactory) { + throw new Error( + 'phpFactory must be set before spawning instances.' + ); } + const php = await this.phpFactory({ isPrimary }); + this.instances.push(php); + this.idleInstances.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/php-wasm/util/src/lib/semaphore.spec.ts b/packages/php-wasm/util/src/lib/semaphore.spec.ts index 23ea734829..d5858d05d1 100644 --- a/packages/php-wasm/util/src/lib/semaphore.spec.ts +++ b/packages/php-wasm/util/src/lib/semaphore.spec.ts @@ -49,4 +49,27 @@ describe('RequestsPerIntervaledSemaphore', () => { await semaphore.acquire(); expect(() => semaphore.acquire()).rejects.toThrow(AcquireTimeoutError); }); + + it('should not leave stale resolvers in queue after timeout', async () => { + const semaphore = new Semaphore({ + concurrency: 1, + timeout: 5, + }); + + // Acquire the only slot + const release = await semaphore.acquire(); + + // This waiter will timeout (stale resolver bug would leave it in queue) + await expect(semaphore.acquire()).rejects.toThrow(AcquireTimeoutError); + + // Start a new waiter - should get the slot when released + const waiter = semaphore.acquire(); + + // Release the slot + release(); + + // The new waiter should succeed (not timeout waiting behind stale resolver) + const releaseWaiter = await waiter; + releaseWaiter(); + }); }); diff --git a/packages/php-wasm/util/src/lib/semaphore.ts b/packages/php-wasm/util/src/lib/semaphore.ts index 128053b314..1e3872d66c 100644 --- a/packages/php-wasm/util/src/lib/semaphore.ts +++ b/packages/php-wasm/util/src/lib/semaphore.ts @@ -38,40 +38,48 @@ export default class Semaphore { } async acquire(): Promise<() => void> { - while (true) { - if (this._running >= this.concurrency) { - // Concurrency exhausted – wait until a lock is released: - const acquired = new Promise((resolve) => { - this.queue.push(resolve); - }); - if (this.timeout !== undefined) { - await Promise.race([acquired, sleep(this.timeout)]).then( - (value) => { - if (value === SleepFinished) { - throw new AcquireTimeoutError(); - } - } - ); - } else { - await acquired; + // Concurrency exhausted - wait in queue for other workers to finish: + if (this._running >= this.concurrency) { + // Create a promise and store its resolver in the queue. + const acquired = new Promise((resolve) => { + this.queue.push(resolve); + }); + + // Wait until it is resolved by another worker or a timeout occurs. + if (this.timeout !== undefined) { + // Store the resolver for cleanup in case of timeout. + const resolve = this.queue.at(-1)!; + const result = await Promise.race([ + acquired, + sleep(this.timeout), + ]); + if (result === SleepFinished) { + // Remove the resolver for the timed out worker from the queue. + this.queue.splice(this.queue.indexOf(resolve), 1); + throw new AcquireTimeoutError(); } } else { - // Acquire the lock: - this._running++; - let released = false; - return () => { - if (released) { - return; - } - released = true; - this._running--; - // Release the lock: - if (this.queue.length > 0) { - this.queue.shift()!(); - } - }; + await acquired; } } + + // Acquire the lock: + this._running++; + let released = false; + + // Return a release function: + return () => { + if (released) { + return; + } + released = true; + this._running--; + + // Release the first item in the queue (call its resolver): + if (this.queue.length > 0) { + this.queue.shift()!(); + } + }; } async run(fn: () => T | Promise): Promise { diff --git a/packages/playground/wordpress/src/boot.ts b/packages/playground/wordpress/src/boot.ts index da1d0ade40..85f7c67b6c 100644 --- 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