Skip to content

Commit a31422f

Browse files
adamzieladamziel
committed
Address feedback
1 parent 5523d16 commit a31422f

File tree

11 files changed

+115
-26
lines changed

11 files changed

+115
-26
lines changed

‎packages/php-wasm/node/project.json‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -238,7 +238,7 @@
238238
"php-networking.spec.ts",
239239
"php-dynamic-loading.spec.ts",
240240
"php-mysqli.spec.ts",
241-
"php-process-manager.spec.ts"
241+
"php-instance-manager.spec.ts"
242242
]
243243
}
244244
},
@@ -254,7 +254,7 @@
254254
"php-networking.spec.ts",
255255
"php-dynamic-loading.spec.ts",
256256
"php-mysqli.spec.ts",
257-
"php-process-manager.spec.ts"
257+
"php-instance-manager.spec.ts"
258258
]
259259
}
260260
},

packages/php-wasm/node/src/test/php-process-manager.spec.ts renamed to packages/php-wasm/node/src/test/php-instance-manager.spec.ts

Lines changed: 83 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,88 @@
11
import { RecommendedPHPVersion } from '@wp-playground/common';
22
import { loadNodeRuntime } from '..';
3-
import { PHP, PHPProcessManager } from '@php-wasm/universal';
3+
import { PHP, PHPProcessManager, SinglePHPInstanceManager } from '@php-wasm/universal';
4+
5+
describe('SinglePHPInstanceManager', () => {
6+
it('should return the PHP instance passed in the constructor', async () => {
7+
const php = new PHP(await loadNodeRuntime(RecommendedPHPVersion));
8+
const mgr = new SinglePHPInstanceManager({ php });
9+
10+
const primaryPhp = await mgr.getPrimaryPhp();
11+
expect(primaryPhp).toBe(php);
12+
});
13+
14+
it('should create a PHP instance using the factory when no instance is provided', async () => {
15+
const phpFactory = vitest.fn(
16+
async () => new PHP(await loadNodeRuntime(RecommendedPHPVersion))
17+
);
18+
const mgr = new SinglePHPInstanceManager({ phpFactory });
19+
20+
expect(phpFactory).not.toHaveBeenCalled();
21+
const php = await mgr.getPrimaryPhp();
22+
expect(phpFactory).toHaveBeenCalledTimes(1);
23+
expect(php).toBeInstanceOf(PHP);
24+
});
25+
26+
it('should return the same PHP instance on subsequent getPrimaryPhp calls', async () => {
27+
const phpFactory = vitest.fn(
28+
async () => new PHP(await loadNodeRuntime(RecommendedPHPVersion))
29+
);
30+
const mgr = new SinglePHPInstanceManager({ phpFactory });
31+
32+
const php1 = await mgr.getPrimaryPhp();
33+
const php2 = await mgr.getPrimaryPhp();
34+
expect(php1).toBe(php2);
35+
expect(phpFactory).toHaveBeenCalledTimes(1);
36+
});
37+
38+
it('should acquire and release the PHP instance', async () => {
39+
const php = new PHP(await loadNodeRuntime(RecommendedPHPVersion));
40+
const mgr = new SinglePHPInstanceManager({ php });
41+
42+
const acquired = await mgr.acquirePHPInstance();
43+
expect(acquired.php).toBe(php);
44+
45+
acquired.reap();
46+
47+
// Should be able to acquire again after reaping
48+
const acquired2 = await mgr.acquirePHPInstance();
49+
expect(acquired2.php).toBe(php);
50+
});
51+
52+
it('should throw an error when trying to acquire twice without reaping', async () => {
53+
const php = new PHP(await loadNodeRuntime(RecommendedPHPVersion));
54+
const mgr = new SinglePHPInstanceManager({ php });
55+
56+
await mgr.acquirePHPInstance();
57+
await expect(() => mgr.acquirePHPInstance()).rejects.toThrowError(
58+
/already acquired/
59+
);
60+
});
61+
62+
it('should throw an error when neither php nor phpFactory is provided', () => {
63+
expect(
64+
() => new SinglePHPInstanceManager({})
65+
).toThrowError(/requires either php or phpFactory/);
66+
});
67+
68+
it('should only call the factory once even with concurrent getPrimaryPhp calls', async () => {
69+
const phpFactory = vitest.fn(
70+
async () => new PHP(await loadNodeRuntime(RecommendedPHPVersion))
71+
);
72+
const mgr = new SinglePHPInstanceManager({ phpFactory });
73+
74+
// Make concurrent calls
75+
const [php1, php2, php3] = await Promise.all([
76+
mgr.getPrimaryPhp(),
77+
mgr.getPrimaryPhp(),
78+
mgr.getPrimaryPhp(),
79+
]);
80+
81+
expect(phpFactory).toHaveBeenCalledTimes(1);
82+
expect(php1).toBe(php2);
83+
expect(php2).toBe(php3);
84+
});
85+
});
486

587
describe('PHPProcessManager', () => {
688
it('should return the primary PHP instance', async () => {

‎packages/php-wasm/node/src/test/php-request-handler.spec.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -277,7 +277,7 @@ describe.each(configsForRequestTests)(
277277
: joinPaths(
278278
new URL(absoluteUrl as string).pathname,
279279
nonExistentFileUri
280-
);
280+
);
281281
expect(response).toEqual({
282282
httpStatusCode: 200,
283283
headers: expect.any(Object),
@@ -989,7 +989,7 @@ describe('PHPRequestHandler – Loopback call', () => {
989989
);
990990
}
991991
const { php, reap } =
992-
await handler.processManager.acquirePHPInstance();
992+
await handler.instanceManager.acquirePHPInstance();
993993
const result = await php.run({
994994
scriptPath: args[1],
995995
env: options.env,

‎packages/php-wasm/node/src/test/php-worker.spec.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,7 @@ describe('PHP Worker', () => {
3737
* Block the primary PHP instance to ensure run()
3838
* creates a fresh PHP instance.
3939
*/
40-
const { reap } = await handler.processManager.acquirePHPInstance({
40+
const { reap } = await handler.instanceManager.acquirePHPInstance({
4141
considerPrimary: instanceType === 'primary',
4242
});
4343
try {

‎packages/php-wasm/universal/src/lib/php-process-manager.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -174,7 +174,7 @@ export class PHPProcessManager implements PHPInstanceManager {
174174
* budget left to optimistically start spawning the next
175175
* instance.
176176
*/
177-
const AcquiredPHP =
177+
const acquiredPHP =
178178
this.nextInstance || this.spawn({ isPrimary: false });
179179

180180
/**
@@ -187,7 +187,7 @@ export class PHPProcessManager implements PHPInstanceManager {
187187
} else {
188188
this.nextInstance = null;
189189
}
190-
return await AcquiredPHP;
190+
return await acquiredPHP;
191191
}
192192

193193
/**

‎packages/php-wasm/universal/src/lib/php-request-handler.ts‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -182,7 +182,7 @@ export class PHPRequestHandler implements AsyncDisposable {
182182
* This is either a provided instanceManager or a PHPProcessManager
183183
* created from the phpFactory.
184184
*/
185-
processManager: PHPInstanceManager;
185+
instanceManager: PHPInstanceManager;
186186
getFileNotFoundAction: FileNotFoundGetActionCallback;
187187

188188
/**
@@ -219,11 +219,11 @@ export class PHPRequestHandler implements AsyncDisposable {
219219

220220
if ('php' in config) {
221221
setChroot(config.php);
222-
this.processManager = new SinglePHPInstanceManager({
222+
this.instanceManager = new SinglePHPInstanceManager({
223223
php: config.php,
224224
});
225225
} else {
226-
this.processManager = new PHPProcessManager({
226+
this.instanceManager = new PHPProcessManager({
227227
phpFactory: async (info) => {
228228
const php = await config.phpFactory({
229229
...info,
@@ -274,7 +274,7 @@ export class PHPRequestHandler implements AsyncDisposable {
274274
}
275275

276276
async getPrimaryPhp() {
277-
return await this.processManager.getPrimaryPhp();
277+
return await this.instanceManager.getPrimaryPhp();
278278
}
279279

280280
/**
@@ -588,7 +588,7 @@ export class PHPRequestHandler implements AsyncDisposable {
588588
): Promise<PHPResponse> {
589589
let spawnedPHP: AcquiredPHP | undefined = undefined;
590590
try {
591-
spawnedPHP = await this.processManager!.acquirePHPInstance({
591+
spawnedPHP = await this.instanceManager!.acquirePHPInstance({
592592
considerPrimary: true,
593593
});
594594
} catch (e) {
@@ -903,7 +903,7 @@ export class PHPRequestHandler implements AsyncDisposable {
903903
}
904904

905905
async [Symbol.asyncDispose]() {
906-
await this.processManager[Symbol.asyncDispose]();
906+
await this.instanceManager[Symbol.asyncDispose]();
907907
}
908908
}
909909

‎packages/php-wasm/universal/src/lib/php-worker.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -231,7 +231,7 @@ export class PHPWorker implements LimitedPHPApi, AsyncDisposable {
231231
private async acquirePHPInstance() {
232232
const { php, reap } = await _private
233233
.get(this)!
234-
.requestHandler!.processManager.acquirePHPInstance();
234+
.requestHandler!.instanceManager.acquirePHPInstance();
235235
if (this.chroot !== null) {
236236
php.chdir(this.chroot);
237237
}

‎packages/php-wasm/universal/src/lib/single-php-instance-manager.ts‎

Lines changed: 9 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@ export class SinglePHPInstanceManager implements PHPInstanceManager {
2828
private php: PHP | undefined;
2929
private phpPromise: Promise<PHP> | undefined;
3030
private phpFactory?: () => Promise<PHP>;
31+
private isAcquired = false;
3132

3233
constructor(options: SinglePHPInstanceManagerOptions) {
3334
if (!options.php && !options.phpFactory) {
@@ -53,17 +54,20 @@ export class SinglePHPInstanceManager implements PHPInstanceManager {
5354
return this.php;
5455
}
5556

56-
async acquirePHPInstance(
57-
// eslint-disable-next-line @typescript-eslint/no-unused-vars
58-
_options: { considerPrimary?: boolean } = {}
59-
): Promise<AcquiredPHP> {
57+
async acquirePHPInstance(): Promise<AcquiredPHP> {
58+
if (this.isAcquired) {
59+
throw new Error(
60+
'The PHP instance already acquired. SinglePHPInstanceManager cannot spawn another PHP instance since, by definition, it only manages a single PHP instance.'
61+
);
62+
}
6063
const php = await this.getPrimaryPhp();
61-
64+
this.isAcquired = true;
6265
return {
6366
php,
6467
reap: () => {
6568
// For single-instance manager, reap is a no-op.
6669
// The instance is reused for all requests.
70+
this.isAcquired = false;
6771
},
6872
};
6973
}

‎packages/playground/cli/src/blueprints-v2/worker-thread-v2.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -270,7 +270,7 @@ export class PlaygroundCliBlueprintV2Worker extends PHPWorker {
270270
async runBlueprintV2(args: WorkerRunBlueprintArgs) {
271271
const requestHandler = this.__internal_getRequestHandler()!;
272272
const { php, reap } =
273-
await requestHandler.processManager.acquirePHPInstance({
273+
await requestHandler.instanceManager.acquirePHPInstance({
274274
considerPrimary: false,
275275
});
276276

‎packages/playground/wordpress/src/boot.ts‎

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -416,7 +416,7 @@ export async function bootRequestHandler(options: BootRequestHandlerOptions) {
416416
// `popen()`, `proc_open()` etc. calls.
417417
if (spawnHandler) {
418418
await php.setSpawnHandler(
419-
spawnHandler(requestHandler?.processManager)
419+
spawnHandler(requestHandler?.instanceManager)
420420
);
421421
}
422422

@@ -443,16 +443,19 @@ export async function bootRequestHandler(options: BootRequestHandlerOptions) {
443443
cookieStore: options.cookieStore,
444444

445445
/**
446-
* If maxPhpInstances is 1, we use a single PHP instance for all requests.
446+
* If maxPhpInstances is 1, the PHPRequestHandler constructor needs
447+
* a PHP instance. Internally, it creates a SinglePHPInstanceManager
448+
* and uses the same PHP instance to handle all requests.
447449
*/
448450
php:
449451
options.maxPhpInstances === 1
450452
? await createPhp(undefined, true)
451453
: undefined,
452454

453455
/**
454-
* If maxPhpInstances is not 1, we use a PHPProcessManager that dynamically
455-
* spawns and reaps PHP instances as needed.
456+
* If maxPhpInstances is not 1, the PHPRequestHandler constructor needs
457+
* a PHP factory function. Internally, it creates a PHPProcessManager that
458+
* dynamically starts new PHP instances and reaps them after they're used.
456459
*/
457460
phpFactory:
458461
options.maxPhpInstances !== 1

0 commit comments

Comments
 (0)