Memory leak: add rotatedPHP to kill and recreate PHP instances after a certain number of requests

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

@@ -116,61 +116,77 @@ export function journalFSEvents(
 	fsRoot: string,
 	onEntry: (entry: FilesystemOperation) => void = () => {}
 ) {
-	fsRoot = normalizePath(fsRoot);
-	const FS = php[__private__dont__use].FS;
-	const FSHooks = createFSHooks(FS, (entry: FilesystemOperation) => {
-		// Only journal entries inside the specified root directory.
-		if (entry.path.startsWith(fsRoot)) {
-			onEntry(entry);
-		} else if (
-			entry.operation === 'RENAME' &&
-			entry.toPath.startsWith(fsRoot)
-		) {
-			for (const op of recordExistingPath(
-				php,
-				entry.path,
-				entry.toPath
-			)) {
-				onEntry(op);
+	function bindToCurrentRuntime() {
+		fsRoot = normalizePath(fsRoot);
+		const FS = php[__private__dont__use].FS;
+		const FSHooks = createFSHooks(FS, (entry: FilesystemOperation) => {
+			// Only journal entries inside the specified root directory.
+			if (entry.path.startsWith(fsRoot)) {
+				onEntry(entry);
+			} else if (
+				entry.operation === 'RENAME' &&
+				entry.toPath.startsWith(fsRoot)
+			) {
+				for (const op of recordExistingPath(
+					php,
+					entry.path,
+					entry.toPath
+				)) {
+					onEntry(op);
+				}
 			}
-		}
-	});
+		});
 
-	/**
-	 * Override the original FS functions with ones running the hooks.
-	 * We could use a Proxy object here if the Emscripten JavaScript module
-	 * did not use hard-coded references to the FS object.
-	 */
-	const originalFunctions: Record<string, Function> = {};
-	for (const [name] of Object.entries(FSHooks)) {
-		originalFunctions[name] = FS[name];
-	}
+		/**
+		 * Override the original FS functions with ones running the hooks.
+		 * We could use a Proxy object here if the Emscripten JavaScript module
+		 * did not use hard-coded references to the FS object.
+		 */
+		const originalFunctions: Record<string, Function> = {};
+		for (const [name] of Object.entries(FSHooks)) {
+			originalFunctions[name] = FS[name];
+		}
 
-	// eslint-disable-next-line no-inner-declarations
-	function bind() {
-		for (const [name, hook] of Object.entries(FSHooks)) {
-			FS[name] = function (...args: any[]) {
-				// @ts-ignore
-				hook(...args);
-				return originalFunctions[name].apply(this, args);
-			};
+		// eslint-disable-next-line no-inner-declarations
+		function bind() {
+			for (const [name, hook] of Object.entries(FSHooks)) {
+				FS[name] = function (...args: any[]) {
+					// @ts-ignore
+					hook(...args);
+					return originalFunctions[name].apply(this, args);
+				};
+			}
 		}
-	}
-	// eslint-disable-next-line no-inner-declarations
-	function unbind() {
-		// Restore the original FS functions.
-		for (const [name, fn] of Object.entries(originalFunctions)) {
-			php[__private__dont__use].FS[name] = fn;
+		// eslint-disable-next-line no-inner-declarations
+		function unbind() {
+			// Restore the original FS functions.
+			for (const [name, fn] of Object.entries(originalFunctions)) {
+				php[__private__dont__use].FS[name] = fn;
+			}
 		}
+
+		php[__private__dont__use].journal = {
+			bind,
+			unbind,
+		};
+		bind();
+	}
+	php.addEventListener('runtime.initialized', bindToCurrentRuntime);
+	if (php[__private__dont__use]) {
+		bindToCurrentRuntime();
 	}
 
-	php[__private__dont__use].journal = {
-		bind,
-		unbind,
-	};
+	function unbindFromOldRuntime() {
+		php[__private__dont__use].journal.unbind();
+		delete php[__private__dont__use].journal;
+	}
+	php.addEventListener('runtime.beforedestroy', unbindFromOldRuntime);
 
-	bind();
-	return unbind;
+	return function unbind() {
+		php.removeEventListener('runtime.initialized', bindToCurrentRuntime);
+		php.removeEventListener('runtime.beforedestroy', unbindFromOldRuntime);
+		return php[__private__dont__use].journal.unbind();
+	};
 }
 
 const createFSHooks = (
@@ -261,7 +277,7 @@ const createFSHooks = (
  */
 export function replayFSJournal(php: BasePHP, entries: FilesystemOperation[]) {
 	// We need to restore the original functions to the FS object
-	// before proceeding, or each replayer FS operation will be journaled.
+	// before proceeding, or each replayed FS operation will be journaled.
 	//
 	// Unfortunately we can't just call the non-journaling versions directly,
 	// because they call other low-level FS functions like `FS.mkdir()`

packages/php-wasm/node/src/lib/node-php.ts

@@ -7,7 +7,6 @@ import {
 	rethrowFileSystemError,
 	__private__dont__use,
 	isExitCodeZero,
-	DataModule,
 } from '@php-wasm/universal';
 
 import { lstatSync, readdirSync } from 'node:fs';
@@ -17,7 +16,6 @@ import { withNetworking } from './networking/with-networking.js';
 export interface PHPLoaderOptions {
 	emscriptenOptions?: EmscriptenOptions;
 	requestHandler?: PHPRequestHandlerConfiguration;
-	dataModules?: Array<DataModule | Promise<DataModule>>;
 }
 
 export type MountSettings = {
@@ -44,20 +42,10 @@ export class NodePHP extends BasePHP {
 		phpVersion: SupportedPHPVersion,
 		options: PHPLoaderOptions = {}
 	) {
-		return await NodePHP.loadSync(phpVersion, {
-			...options,
-			emscriptenOptions: {
-				/**
-				 * Emscripten default behavior is to kill the process when
-				 * the WASM program calls `exit()`. We want to throw an
-				 * exception instead.
-				 */
-				quit: function (code, error) {
-					throw error;
-				},
-				...(options.emscriptenOptions || {}),
-			},
-		}).phpReady;
+		return new NodePHP(
+			await NodePHP.loadRuntime(phpVersion, options),
+			options.requestHandler
+		);
 	}
 
 	/**
@@ -67,29 +55,25 @@ export class NodePHP extends BasePHP {
 	 *
 	 * @see load
 	 */
-	static loadSync(
+	static async loadRuntime(
 		phpVersion: SupportedPHPVersion,
 		options: PHPLoaderOptions = {}
 	) {
-		/**
-		 * Keep any changes to the signature of this method in sync with the
-		 * `PHP.load` method in the @php-wasm/node package.
-		 */
-		const php = new NodePHP(undefined, options.requestHandler);
-
-		const doLoad = async () => {
-			const runtimeId = await loadPHPRuntime(
-				await getPHPLoaderModule(phpVersion),
-				await withNetworking(options.emscriptenOptions || {})
-			);
-			php.initializeRuntime(runtimeId);
-		};
-		const asyncData = doLoad();
-
-		return {
-			php,
-			phpReady: asyncData.then(() => php),
+		const emscriptenOptions: EmscriptenOptions = {
+			/**
+			 * Emscripten default behavior is to kill the process when
+			 * the WASM program calls `exit()`. We want to throw an
+			 * exception instead.
+			 */
+			quit: function (code, error) {
+				throw error;
+			},
+			...(options.emscriptenOptions || {}),
 		};
+		return await loadPHPRuntime(
+			await getPHPLoaderModule(phpVersion),
+			await withNetworking(emscriptenOptions)
+		);
 	}
 
 	/**

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

@@ -0,0 +1,128 @@
+import fs from 'fs';
+import os from 'os';
+import path from 'path';
+import { NodePHP } from '..';
+import { LatestSupportedPHPVersion, rotatedPHP } from '@php-wasm/universal';
+
+const recreateRuntime = async (version: any = LatestSupportedPHPVersion) =>
+	await NodePHP.loadRuntime(version);
+
+async function rotate(php: any) {
+	await php.run({
+		code: `<?php echo 'hi';`,
+	});
+	await php.run({
+		code: `<?php echo 'hi';`,
+	});
+}
+
+describe('rotatedPHP()', () => {
+	it('Should recreate the PHP instance after maxRequests', async () => {
+		const recreateRuntimeSpy = vitest.fn(recreateRuntime);
+		const php = await rotatedPHP({
+			php: new NodePHP(await recreateRuntimeSpy(), {
+				documentRoot: '/test-root',
+			}),
+			recreateRuntime: recreateRuntimeSpy,
+			maxRequests: 1,
+		});
+		await rotate(php);
+		expect(recreateRuntimeSpy).toHaveBeenCalledTimes(2);
+	});
+
+	it('Should hotswap the PHP instance from 8.2 to 8.3', async () => {
+		let nbCalls = 0;
+		const recreateRuntimeSpy = vitest.fn(() =>
+			++nbCalls === 1 ? recreateRuntime('8.2') : recreateRuntime('8.3')
+		);
+		const php = await rotatedPHP({
+			php: new NodePHP(await recreateRuntimeSpy(), {
+				documentRoot: '/test-root',
+			}),
+			recreateRuntime: recreateRuntimeSpy,
+			maxRequests: 1,
+		});
+		const version1 = (
+			await php.run({
+				code: `<?php echo PHP_VERSION;`,
+			})
+		).text;
+		const version2 = (
+			await php.run({
+				code: `<?php echo PHP_VERSION;`,
+			})
+		).text;
+		expect(version1).toMatch(/^8\.2/);
+		expect(version2).toMatch(/^8\.3/);
+	});
+
+	it('Should preserve the custom SAPI name', async () => {
+		const php = await rotatedPHP({
+			php: new NodePHP(await recreateRuntime(), {
+				documentRoot: '/test-root',
+			}),
+			recreateRuntime,
+			maxRequests: 1,
+		});
+		php.setSapiName('custom SAPI');
+		await rotate(php);
+		const result = await php.run({
+			code: `<?php echo php_sapi_name();`,
+		});
+		expect(result.text).toBe('custom SAPI');
+	});
+
+	it('Should preserve the MEMFS files', async () => {
+		const php = await rotatedPHP({
+			php: new NodePHP(await recreateRuntime(), {
+				documentRoot: '/test-root',
+			}),
+			recreateRuntime,
+			maxRequests: 1,
+		});
+		await rotate(php);
+		php.mkdir('/test-root');
+		php.writeFile('/test-root/index.php', '<?php echo "hi";');
+		await rotate(php);
+		expect(php.fileExists('/test-root/index.php')).toBe(true);
+		expect(php.readFileAsText('/test-root/index.php')).toBe(
+			'<?php echo "hi";'
+		);
+	});
+
+	it('Should not overwrite the NODEFS files', async () => {
+		const php = await rotatedPHP({
+			php: new NodePHP(await recreateRuntime(), {
+				documentRoot: '/test-root',
+			}),
+			recreateRuntime,
+			maxRequests: 1,
+		});
+		await rotate(php);
+		php.mkdir('/test-root');
+		php.writeFile('/test-root/index.php', 'test');
+		php.mkdir('/test-root/nodefs');
+
+		const tempDir = fs.mkdtempSync(path.join(os.tmpdir(), 'temp-'));
+		const tempFile = path.join(tempDir, 'file');
+		fs.writeFileSync(tempFile, 'playground');
+		const date = new Date();
+		date.setFullYear(date.getFullYear() - 1);
+		fs.utimesSync(tempFile, date, date);
+		try {
+			php.mount(tempDir, '/test-root/nodefs');
+
+			await rotate(php);
+
+			// Expect the file to still have the same utime
+			const stats = fs.statSync(tempFile);
+			expect(Math.round(stats.atimeMs)).toBe(Math.round(date.getTime()));
+
+			// The MEMFS file should still be there
+			expect(php.fileExists('/test-root/index.php')).toBe(true);
+		} finally {
+			fs.rmSync(tempFile);
+			fs.rmdirSync(tempDir);
+		}
+	});
+});