Skip to content

Commit f0e4956

Browse files
[PHP] Inline rotatePHPRuntime() into the PHP class (#2559)
## Motivation for the change, related issues Moves the `rotatePHPRuntime()` logic inside the PHP class and properly await them to prevent async rotation failures: ``` { type: 'request.error', error: Error: PHP.run() failed with exit code 255. at runExecutionFunction.then.dispatchEvent.type (/Users/cloudnik/www/Automattic/core/plugins/wordpress-playground/packages/php-wasm/universal/src/lib/php.ts:1052:14), source: 'php-wasm' } Trace: hotSwapPHPRuntime called on a disposed PHP instance at PHP.hotSwapPHPRuntime (/Users/cloudnik/www/Automattic/core/plugins/wordpress-playground/packages/php-wasm/universal/src/lib/php.ts:1272:13) at rotateRuntime (/Users/cloudnik/www/Automattic/core/plugins/wordpress-playground/packages/php-wasm/universal/src/lib/rotate-php-runtime.ts:45:14) at async rotateRuntimeForPhpWasmError (/Users/cloudnik/www/Automattic/core/plugins/wordpress-playground/packages/php-wasm/universal/src/lib/rotate-php-runtime.ts:64:4) ``` ## Implementation The `rotatePHPRuntime()` function kills the "worn out" PHP Emscripten runtimes and replaces them with new ones. php-fpm does the same thing. Rotating the runtime happens in response to a few events emitted by the PHP class. However, rotation is an async operation and event listeners are expected to be synchronous. As a result, a PHP instance may get killed and the rotate handler will attempt to rotate it asynchronously, causing errors such as the one cited above. This PR inlines that logic in the PHP class via a new `php.enableRuntimeRotation()` method. The PHP instance now keeps track if the rotation condition internally and optionally rotates the runtime before every `php.run()`, `php.runStream()`, and `php.cli()` call. This approach also has other benefits: * Ability to call `php.cli()` and `php.run()` multiple times without trashing the instance – the rotation is handled internally. * It creates a way to get rid of the `primary` and `secondary` PHP instance concept in the PHPProcessManager class. We no longer need to worry about accidentally trashing the primary instance by calling `.cli()` on it. * The rotation is now lazy, not eager, and only happens on the next PHP code execution. ### Potential follow-up work * Expose a static `PHP.create( runtimeFactory )` method to simplify the instance constructions. Right now you need to do `const php = new PHP( runtimeId ); PHP.enableRuntimeRotation( runtimeFactory );`. ## Testing Instructions (or ideally a Blueprint) CI --------- Co-authored-by: Brandon Payton <[email protected]>
1 parent 3bf8006 commit f0e4956

File tree

7 files changed

+191
-214
lines changed

7 files changed

+191
-214
lines changed

packages/php-wasm/fs-journal/src/lib/fs-journal.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -178,11 +178,11 @@ export function journalFSEvents(
178178
php[__private__dont__use].journal.unbind();
179179
delete php[__private__dont__use].journal;
180180
}
181-
php.addEventListener('runtime.beforedestroy', unbindFromOldRuntime);
181+
php.addEventListener('runtime.beforeExit', unbindFromOldRuntime);
182182

183183
return function unbind() {
184184
php.removeEventListener('runtime.initialized', bindToCurrentRuntime);
185-
php.removeEventListener('runtime.beforedestroy', unbindFromOldRuntime);
185+
php.removeEventListener('runtime.beforeExit', unbindFromOldRuntime);
186186
return php[__private__dont__use].journal.unbind();
187187
};
188188
}

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

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2868,6 +2868,22 @@ phpLoaderOptions.forEach((options) => {
28682868
'/internal/shared/bin/php'
28692869
);
28702870
});
2871+
2872+
it('should support multiple calls to php.cli() and php.runStream() when runtime rotation is enabled', async () => {
2873+
php.enableRuntimeRotation({
2874+
maxRequests: 1,
2875+
recreateRuntime: () =>
2876+
loadNodeRuntime(phpVersion as any, options),
2877+
});
2878+
const response = await php.cli(['php', '-r', 'echo "Hello";']);
2879+
expect(await response.stdoutText).toBe('Hello');
2880+
const response2 = await php.runStream({
2881+
code: `<?php echo "Hello";`,
2882+
});
2883+
expect(await response2.stdoutText).toBe('Hello');
2884+
const response3 = await php.cli(['php', '-r', 'echo "Hello";']);
2885+
expect(await response3.stdoutText).toBe('Hello');
2886+
});
28712887
});
28722888

28732889
describe('Response parsing', { skip: options.withXdebug }, () => {

packages/php-wasm/node/src/test/rotate-php-runtime.spec.ts

Lines changed: 21 additions & 73 deletions
Original file line numberDiff line numberDiff line change
@@ -5,22 +5,20 @@ import {
55
LatestSupportedPHPVersion,
66
PHP,
77
__private__dont__use,
8-
rotatePHPRuntime,
98
} from '@php-wasm/universal';
109
import { loadNodeRuntime } from '../lib';
1110
import { createNodeFsMountHandler } from '../lib/node-fs-mount';
1211

1312
const recreateRuntime = async (version: any = LatestSupportedPHPVersion) =>
1413
await loadNodeRuntime(version);
1514

16-
describe('rotatePHPRuntime()', () => {
15+
describe('php.enableRuntimeRotation()', () => {
1716
it('Preserves the /internal directory through PHP runtime recreation', async () => {
1817
// Rotate the PHP runtime
1918
const recreateRuntimeSpy = vitest.fn(recreateRuntime);
2019

2120
const php = new PHP(await recreateRuntime());
22-
rotatePHPRuntime({
23-
php,
21+
php.enableRuntimeRotation({
2422
cwd: '/test-root',
2523
recreateRuntime: recreateRuntimeSpy,
2624
maxRequests: 10,
@@ -55,8 +53,7 @@ describe('rotatePHPRuntime()', () => {
5553
const recreateRuntimeSpy = vitest.fn(recreateRuntime);
5654

5755
const php = new PHP(await recreateRuntime());
58-
rotatePHPRuntime({
59-
php,
56+
php.enableRuntimeRotation({
6057
cwd: '/test-root',
6158
recreateRuntime: recreateRuntimeSpy,
6259
maxRequests: 10,
@@ -89,8 +86,7 @@ describe('rotatePHPRuntime()', () => {
8986
const recreateRuntimeSpy = vitest.fn(recreateRuntime);
9087

9188
const php = new PHP(await recreateRuntime());
92-
rotatePHPRuntime({
93-
php,
89+
php.enableRuntimeRotation({
9490
cwd: '/wordpress',
9591
recreateRuntime: recreateRuntimeSpy,
9692
maxRequests: 10,
@@ -192,8 +188,7 @@ describe('rotatePHPRuntime()', () => {
192188
const recreateRuntimeSpy = vitest.fn(recreateRuntime);
193189
// Rotate the PHP runtime
194190
const php = new PHP(await recreateRuntime());
195-
rotatePHPRuntime({
196-
php,
191+
php.enableRuntimeRotation({
197192
cwd: '/test-root',
198193
recreateRuntime: recreateRuntimeSpy,
199194
maxRequests: 1000,
@@ -222,62 +217,39 @@ describe('rotatePHPRuntime()', () => {
222217
it('Should recreate the PHP runtime after maxRequests', async () => {
223218
const recreateRuntimeSpy = vitest.fn(recreateRuntime);
224219
const php = new PHP(await recreateRuntimeSpy());
225-
rotatePHPRuntime({
226-
php,
220+
php.enableRuntimeRotation({
227221
cwd: '/test-root',
228222
recreateRuntime: recreateRuntimeSpy,
229223
maxRequests: 1,
230224
});
231225
// Rotate the PHP runtime
232226
await php.run({ code: `` });
233-
expect(recreateRuntimeSpy).toHaveBeenCalledTimes(2);
234-
}, 30_000);
235-
236-
it('Should not rotate after the cleanup handler is called, even if max requests is reached', async () => {
237-
const recreateRuntimeSpy = vitest.fn(recreateRuntime);
238-
const php = new PHP(await recreateRuntimeSpy());
239-
const cleanup = rotatePHPRuntime({
240-
php,
241-
cwd: '/test-root',
242-
recreateRuntime: recreateRuntimeSpy,
243-
maxRequests: 1,
244-
});
245-
// Rotate the PHP runtime
246227
await php.run({ code: `` });
247228
expect(recreateRuntimeSpy).toHaveBeenCalledTimes(2);
248-
249-
cleanup();
250-
251-
// No further rotation should happen
252-
await php.run({ code: `` });
253-
await php.run({ code: `` });
254-
255-
expect(recreateRuntimeSpy).toHaveBeenCalledTimes(2);
256229
}, 30_000);
257230

258231
it('Should recreate the PHP runtime after a PHP runtime crash', async () => {
259232
const recreateRuntimeSpy = vitest.fn(recreateRuntime);
260233
const php = new PHP(await recreateRuntimeSpy());
261-
rotatePHPRuntime({
262-
php,
234+
php.enableRuntimeRotation({
263235
cwd: '/test-root',
264236
recreateRuntime: recreateRuntimeSpy,
265237
maxRequests: 1234,
266238
});
267239
// Cause a PHP runtime rotation due to error
268-
await php.dispatchEvent({
240+
php.dispatchEvent({
269241
type: 'request.error',
270242
error: new Error('mock error'),
271243
source: 'php-wasm',
272244
});
245+
await php.run({ code: `` });
273246
expect(recreateRuntimeSpy).toHaveBeenCalledTimes(2);
274247
}, 30_000);
275248

276249
it('Should not recreate the PHP runtime after a PHP fatal', async () => {
277250
const recreateRuntimeSpy = vitest.fn(recreateRuntime);
278251
const php = new PHP(await recreateRuntimeSpy());
279-
rotatePHPRuntime({
280-
php,
252+
php.enableRuntimeRotation({
281253
cwd: '/test-root',
282254
recreateRuntime: recreateRuntimeSpy,
283255
maxRequests: 1234,
@@ -296,31 +268,6 @@ describe('rotatePHPRuntime()', () => {
296268
expect(recreateRuntimeSpy).toHaveBeenCalledTimes(1);
297269
}, 30_000);
298270

299-
it('Should not rotate after the cleanup handler is called, even if there is a PHP runtime error', async () => {
300-
const recreateRuntimeSpy = vitest.fn(recreateRuntime);
301-
const php = new PHP(await recreateRuntimeSpy());
302-
const cleanup = rotatePHPRuntime({
303-
php,
304-
cwd: '/test-root',
305-
recreateRuntime: recreateRuntimeSpy,
306-
maxRequests: 1,
307-
});
308-
// Rotate the PHP runtime
309-
await php.run({ code: `` });
310-
expect(recreateRuntimeSpy).toHaveBeenCalledTimes(2);
311-
312-
cleanup();
313-
314-
// No further rotation should happen
315-
php.dispatchEvent({
316-
type: 'request.error',
317-
error: new Error('mock error'),
318-
source: 'php-wasm',
319-
});
320-
321-
expect(recreateRuntimeSpy).toHaveBeenCalledTimes(2);
322-
}, 30_000);
323-
324271
it('Should hotswap the PHP runtime from 8.2 to 8.3', async () => {
325272
let nbCalls = 0;
326273
const recreateRuntimeSpy = vitest.fn(() => {
@@ -331,8 +278,7 @@ describe('rotatePHPRuntime()', () => {
331278
return recreateRuntime('8.3');
332279
});
333280
const php = new PHP(await recreateRuntimeSpy());
334-
rotatePHPRuntime({
335-
php,
281+
php.enableRuntimeRotation({
336282
cwd: '/test-root',
337283
recreateRuntime: recreateRuntimeSpy,
338284
maxRequests: 1,
@@ -353,8 +299,7 @@ describe('rotatePHPRuntime()', () => {
353299

354300
it('Should preserve the custom SAPI name', async () => {
355301
const php = new PHP(await recreateRuntime());
356-
rotatePHPRuntime({
357-
php,
302+
php.enableRuntimeRotation({
358303
cwd: '/test-root',
359304
recreateRuntime,
360305
maxRequests: 1,
@@ -371,8 +316,7 @@ describe('rotatePHPRuntime()', () => {
371316

372317
it('Should preserve the MEMFS files', async () => {
373318
const php = new PHP(await recreateRuntime());
374-
rotatePHPRuntime({
375-
php,
319+
php.enableRuntimeRotation({
376320
cwd: '/test-root',
377321
recreateRuntime,
378322
maxRequests: 1,
@@ -395,15 +339,15 @@ describe('rotatePHPRuntime()', () => {
395339

396340
it('Should not overwrite the NODEFS files', async () => {
397341
const php = new PHP(await recreateRuntime());
398-
rotatePHPRuntime({
399-
php,
342+
php.enableRuntimeRotation({
400343
cwd: '/test-root',
401344
recreateRuntime,
402345
maxRequests: 1,
403346
});
404347

405348
// Rotate the PHP runtime
406-
await php.run({ code: `` });
349+
const result = await php.run({ code: `` });
350+
await result.text;
407351

408352
php.mkdir('/test-root');
409353
php.writeFile('/test-root/index.php', 'test');
@@ -415,7 +359,10 @@ describe('rotatePHPRuntime()', () => {
415359
date.setFullYear(date.getFullYear() - 1);
416360
fs.utimesSync(tempFile, date, date);
417361
try {
418-
php.mount('/test-root/nodefs', createNodeFsMountHandler(tempDir));
362+
await php.mount(
363+
'/test-root/nodefs',
364+
createNodeFsMountHandler(tempDir)
365+
);
419366

420367
// Rotate the PHP runtime
421368
await php.run({ code: `` });
@@ -429,6 +376,7 @@ describe('rotatePHPRuntime()', () => {
429376
} finally {
430377
fs.rmSync(tempFile);
431378
fs.rmdirSync(tempDir);
379+
php.exit();
432380
}
433381
}, 30_000);
434382
});

0 commit comments

Comments
 (0)