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

Author: JanJakes

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);
 	});
 });

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);
 	});
 });