-
Notifications
You must be signed in to change notification settings - Fork 369
[PHP] Inline rotatePHPRuntime() into the PHP class #2559
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…ult" This reverts commit 2745d51.
This reverts commit b2552a9.
- Fixed conflict in symlink test - preserved both HEAD and trunk functionality - Fixed conflict in auto-mount tests - merged both versions properly - Fixed TypeScript errors by changing autoMount from boolean to string - All tests now work with both Blueprint v1 and v2 versions
…ding the WordPress server
| delete php[__private__dont__use].journal; | ||
| } | ||
| php.addEventListener('runtime.beforedestroy', unbindFromOldRuntime); | ||
| php.addEventListener('runtime.beforeExit', unbindFromOldRuntime); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically the PHP instance can still be reinitialized with another PHP runtime so it's not destroyed
| if (!this.#webSapiInitialized) { | ||
| await this.#initWebRuntime(); | ||
| this.#webSapiInitialized = true; | ||
| if (!this.#phpWasmInitCalled) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I inlined this call to reduce the mental load – there were too many names for me to follow
Add error handling to ensure original error is rethrown.
|
@brandonpayton can you see any problems with this implementation? |
|
This is blocking other work so I'll go ahead and merge. Feel free to still leave feedback or even rollback if this ended up having any unintended consequences. |
This PR ensures setting the spawn handler once is effective for the entire lifetime of a given PHP instance. Before this PR, the spawn handler would not be re-bound to the new PHP runtime in the hotSwapPHPRuntime() call. Follows up on #2559. The spawn handler was not preserved even before more prominent. ## Testing instructions CI – there's a new test to ensure the spawn handler is preserved after the runtime rotation / swapping
This PR ensures setting the spawn handler once is effective for the entire lifetime of a given PHP instance. Before this PR, the spawn handler would not be re-bound to the new PHP runtime in the hotSwapPHPRuntime() call. Follows up on #2559. The spawn handler was not preserved even before more prominent. ## Testing instructions CI – there's a new test to ensure the spawn handler is preserved after the runtime rotation / swapping
This PR ships the web wasm binaries with the `run_cli` C function that enables calling php.cli(). Intertwining the CLI SAPI usage with the web SAPI usage was impractical before #2559, but now we can easily call `await php.cli(); await php.run()` on the same PHP instance. Note this PR does not expose the `.cli()` method to the public yet. It only ships the updated WASM binaries to unlock that. ## Testing instructions CI
This PR ships the web wasm binaries with the `run_cli` C function that enables calling php.cli(). Intertwining the CLI SAPI usage with the web SAPI usage was impractical before #2559, but now we can easily call `await php.cli(); await php.run()` on the same PHP instance. Note this PR does not expose the `.cli()` method to the public yet. It only ships the updated WASM binaries to unlock that. ## Testing instructions CI
## Motivation for the change, related issues This PR: 1. Exposes the `php.cli()` method for in-browser PHP.wasm consumers 2. Starts using it in the Playground website by swapping the custom spawn handler for the universal `sandboxedSpawnHandlerFactory()` that calls `php.cli()` and is consistent across all Playground runtimes. For context on what makes this possible, see #2589 and #2559. ## Testing Instructions (or ideally a Blueprint) CI
Motivation for the change, related issues
Moves the
rotatePHPRuntime()logic inside the PHP class and properly await them to prevent async rotation failures: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 everyphp.run(),php.runStream(), andphp.cli()call. This approach also has other benefits:php.cli()andphp.run()multiple times without trashing the instance – the rotation is handled internally.primaryandsecondaryPHP instance concept in the PHPProcessManager class. We no longer need to worry about accidentally trashing the primary instance by calling.cli()on it.Potential follow-up work
PHP.create( runtimeFactory )method to simplify the instance constructions. Right now you need to doconst php = new PHP( runtimeId ); PHP.enableRuntimeRotation( runtimeFactory );.Testing Instructions (or ideally a Blueprint)
CI