Patch the premature "request in progress" semaphore release (#2455)

Guards against releasing the "request in progress" semaphore too early
in the php.runStream() call.

A single PHP runtime can only handle one request at a time. The PHP
class calls a `wasm_sapi_handle_request` C function that initializes the
PHP runtime and starts the request. That function is asynchronous and
may yield back to the event loop before the request is fully handled,
the exit code known, and the runtime is cleaned up and prepared for
another request.

The PHP class uses an async semaphore to protect against calling
`wasm_sapi_handle_request` again while a previous call is still running.

However, PR 2266 [1] introduced a regression where the semaphore was
released too early. As a result, it opened the runtime to a race
condition where a subsequent runStream() call tried to run PHP code on a
runtime that was in a middle of handling a request.

This test ensures that two runStream() calls can be made without
crashing the runtime.

[1] #2266

Author: adamziel

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

@@ -178,7 +178,18 @@ phpLoaderOptions.forEach((options) => {
 				const exitCode1Http200 = await php.runStream({
 					code: '<?php trigger_error("Fatal error", E_USER_ERROR);',
 				});
-				expect(await exitCode1Http200.ok()).toBe(false);
+
+				/**
+				 * trigger_error does not affect the HTTP status code set by PHP.
+				 *
+				 * Some dev servers that buffer the response will notice the
+				 * HTTP status code is 200, the exit code is 1, and will replace
+				 * the HTTP status code with 500.
+				 *
+				 * However, from the PHP process perspective, the status code is
+				 * still 200.
+				 */
+				expect(await exitCode1Http200.ok()).toBe(true);
 
 				const http500 = await php.runStream({
 					code: '<?php http_response_code(500); ',
@@ -349,6 +360,86 @@ phpLoaderOptions.forEach((options) => {
 					'stderr second'
 				);
 			});
+
+			/**
+			 * Guards against releasing the "request in progress" semaphore
+			 * too early in the php.runStream() call.
+			 *
+			 * ## Context
+			 *
+			 * A single PHP runtime can only handle one request at a time.
+			 * The PHP class calls a `wasm_sapi_handle_request` C function that
+			 * initializes the PHP runtime and starts the request. That function is
+			 * asynchronous and may yield back to the event loop before the request
+			 * is fully handled, the exit code known, and the runtime is cleaned up
+			 * and prepared for another request.
+			 *
+			 * The PHP class uses an async semaphore to protect against calling
+			 * `wasm_sapi_handle_request` again while a previous call is still
+			 * running.
+			 *
+			 * However, PR 2266 [1] introduced a regression where the semaphore
+			 * was released too early. As a result, it opened the runtime to a
+			 * race condition where a subsequent runStream() call tried to run
+			 * PHP code on a runtime that was in a middle of handling a request.
+			 *
+			 * This test ensures that two runStream() calls can be made without
+			 * crashing the runtime.
+			 *
+			 * [1] https://github.com/WordPress/wordpress-playground/pull/2266
+			 */
+			it('should stagger concurrent runStream() calls', async () => {
+				/**
+				 * Call runStream() twice without waiting for the first one to finish.
+				 */
+				const response1Promise = php.runStream({
+					code: `<?php
+					// Declare a new function to ensure a name collision if
+					// the second runStream() call actually runs before this
+					// one concludes.
+					function unique_fn_name(){};
+
+					// Yield back to the event loop.
+					sleep(3);
+
+					// Output some stdout information just to be extra sure the
+					// execution actually got here.
+					echo "response 1";`,
+				});
+				const response2Promise = php.runStream({
+					code: `<?php
+					// Ensure a name collision if the first PHP request haven't
+					// concluded yet and the symbol is already registered in the
+					// runtime.
+					function unique_fn_name(){};
+
+					// Yield back to the event loop. Technically we don't need to
+					// do that, but switching stacks catalyzes the crash.
+					sleep(1);
+
+					// Output some stdout information just to be extra sure the
+					// execution actually got here.
+					echo "response 2";`,
+				});
+
+				// Only now start awaiting things.
+				// First, the streaming response objects
+				const response1 = await response1Promise;
+				const response2 = await response2Promise;
+
+				// Then, wait for the requests to conclude.
+				await response1.finished;
+				await response2.finished;
+
+				console.log(await response1.stderrText);
+				// Ensure both requests succeeded.
+				expect(await response1.ok()).toBe(true);
+				expect(await response2.ok()).toBe(true);
+
+				// Confirm the STDOUT output.
+				expect(await response1.stdoutText).toBe('response 1');
+				expect(await response2.stdoutText).toBe('response 2');
+			});
 		});
 
 		describe('ENV variables', { skip: options.withXdebug }, () => {

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

@@ -449,10 +449,26 @@ export class PHPRequestHandler implements AsyncDisposable {
 					// Pass along URL with the #fragment filtered out
 					url: requestedUrl.toString(),
 				};
-				return this.#spawnPHPAndDispatchRequest(
+				const response = await this.#spawnPHPAndDispatchRequest(
 					effectiveRequest,
 					fsPath
 				);
+
+				/**
+				 * If the response is but the exit code is non-zero, let's rewrite the
+				 * HTTP status code as 500. We're acting as a HTTP server here and
+				 * this behavior is in line with what Nginx and Apache do.
+				 */
+				if (response.ok() && response.exitCode !== 0) {
+					return new PHPResponse(
+						500,
+						response.headers,
+						response.bytes,
+						response.errors,
+						response.exitCode
+					);
+				}
+				return response;
 			} else {
 				return this.#serveStaticFile(primaryPhp, fsPath);
 			}
@@ -568,6 +584,7 @@ export class PHPRequestHandler implements AsyncDisposable {
 					response.headers
 				);
 			}
+
 			return response;
 		} catch (error) {
 			const executionError = error as PHPExecutionFailureError;

packages/php-wasm/universal/src/lib/php-response.ts

@@ -123,12 +123,8 @@ export class StreamedPHPResponse {
 	 * Resolves once HTTP status code is available.
 	 */
 	get httpStatusCode(): Promise<number> {
-		return Promise.race([
-			this.getParsedHeaders().then((headers) => headers.httpStatusCode),
-			this.exitCode.then((exitCode) =>
-				exitCode !== 0 ? 500 : undefined
-			),
-		])
+		return this.getParsedHeaders()
+			.then((headers) => headers.httpStatusCode)
 			.then((result) => {
 				if (result !== undefined) {
 					return result;
@@ -293,6 +289,14 @@ export class PHPResponse implements PHPResponseData {
 		);
 	}
 
+	/**
+	 * True if the response is successful (HTTP status code 200-399),
+	 * false otherwise.
+	 */
+	ok(): boolean {
+		return this.httpStatusCode >= 200 && this.httpStatusCode < 400;
+	}
+
 	toRawData(): PHPResponseData {
 		return {
 			headers: this.headers,

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

@@ -654,24 +654,39 @@ export class PHP implements Disposable {
 			}
 		);
 
-		// Free up resources when the response is done
-		await streamedResponsePromise
-			.finally(() => {
-				if (heapBodyPointer) {
+		const cleanup = () => {
+			if (heapBodyPointer) {
+				try {
 					this[__private__dont__use].free(heapBodyPointer);
+				} catch (e) {
+					logger.error(e);
 				}
-			})
-			.finally(() => {
-				release();
-				/**
-				 * Notify the filesystem journal and rotatePHPRuntime() that we've handled
-				 * another request.
-				 */
-				this.dispatchEvent({
-					type: 'request.end',
-				});
+			}
+
+			// Release the "request in progress" semaphore.
+			release();
+
+			/**
+			 * Notify the filesystem journal and rotatePHPRuntime() that we've handled
+			 * another request.
+			 */
+			this.dispatchEvent({
+				type: 'request.end',
 			});
-		return streamedResponsePromise;
+		};
+
+		// Free up resources once the request is fully handled.
+		return streamedResponsePromise.then(
+			(streamedResponse) => {
+				streamedResponse.finished.finally(cleanup);
+				return streamedResponse;
+			},
+			(error) => {
+				// If we couldn't even get the response,
+				cleanup();
+				throw error;
+			}
+		);
 	}
 
 	/**