-
Notifications
You must be signed in to change notification settings - Fork 275
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
Breaking: Loopback Request Support #1287
Conversation
…() property (#1286) Removes the public `php.addServerGlobalEntry()` method in favor of a new `$_SERVER` property for `PHPRequest`. The provided `$_SERVER` entries only affected the next `run()` call so making them a part of the `run()` argument is more natural. ## What problem does this solve? This PR reduces the amount of state stored in the `BasePHP` class, simplifying [the PHPProcessManager explorations](#1287). ## Testing instructions Confirm the unit and e2e tests pass.
…teStreaming method once the spy has been called
094e6b8
to
713e030
Compare
…equent loading attempts
…HP instances equal standing (there's a single authoritative MEMFS that's proxied to all other PHPs)
The request handler needs to decide whether to serve a static asset or run the PHP interpreter. For static assets it should just reuse the primary PHP even if there's 50 concurrent requests to serve. However, for dynamic PHP requests, it needs to grab an available interpreter. Therefore, it cannot just accept PHP as an argument as serving requests requires access to ProcessManager.
Adds a PHP Process manager, a class that can spawn maintain a number of PHP instances at the same time. The idea is to have: * A single "primary" PHP instance that's never killed – it contains the reference filesystem used by all other PHP instances. * A pool of disposable PHP instances that are spawned to handle a single request and reaped immediately after. Spawning new PHP instances is reasonably fast and can happen on demand, there's no need to keep a pool of "warm" instances that occupies the memory all the time. Example usage: ```ts const mgr = new PHPProcessManager({ phpFactory: async () => NodePHP.load(RecommendedPHPVersion), maxPhpInstances: 4, }); const instance = await mgr.getInstance(); await instance.php.run({ code }); instance.reap(); ``` Related to #1287 ## Remaining work * Add a "context" parameter to `getInstance()` to enable setting the correct SAPI Name, e.g. `CLI` or `FPM` depending on the purpose the PHP instance is created for. This would solve issues like #1223. Alternatively, the consuming program could call `setSapiName()` on the PHP instance returned by `getInstance()` – but that assumes the factory did not initialize the web runtime. ## Testing instructions Confirm the unit tests pass. This PR only adds new code, it does not plug in the PHPProcessManager class into any request dispatching code yet.
…he remote PHP API client (#1321) `setSapiName`, `setPhpIniEntry`, and `setPhpIniPath` are methods that must either be called synchronously before the PHP internal state is initialized, or with a complex argument that can't be serialized over a remote connection. They can only be used in the same process before PHP is fully initialized and have never worked correctly when connecting a Playground API client to `remote.html`. Removing them makes that apparent, prevents confusing API interactions. In addition, it sets the stage for supporting a [Loopback Request](#1287) as the backend for the API endpoint switches from a single PHP instance to a PHPRequestHandler, and here we're removing an API surface that needs to impact all existing and future PHP instances. ## Testing instructions Confirm the CI checks pass.
Alright, let's ship and observe any incoming error reports! I expect a slight increase in out of memory errors on low end phones, which we can solve with #1233, but it should work otherwise. This is exciting 🤞 |
Thanks for the heads up! |
how to modify this to work???? // Use dynamic import for the CommonJS module // Load PHP with the specified version and request handler options
}); php.writeFile('./index.php', // Or use the familiar HTTP concepts: throw new Error("No request handler available."); Error: No request handler available. |
A list of breaking changes
Warning
This is breaking change! See the list of public API changes below to assess the impact on your app.
BasePHP
,WebPHP
, andNodePHP
no longer accept theserverOptions
argument and will not create a PHPRequestHandler instance. Instead, you have to create anew PHPRequestHandler()
explicitly.PHPRequestHandler
now requires either aprocessManager
option or aphpFactory
option.Description
Adds support for PHP spawning more PHP instances:
Without this PR, we only have a single PHP instance running. If that instance needs to run another PHP script while it's running, it will get error 502 as there's no other PHP instance available to handle that.
Closes #1182
Closes #1177
Technical implementation
This PR draws a clear line between BaesPHP and PHPRequestHandler.
Before this PR, BasePHP could be given a
serverOptions
argument and it would create anew PHPRequestHandler
as a convenience. After this PR, BasePHP longer instantiatesPHPRequestHandler
. Why? Two reasons:PHPRequestHandler
to orchestrate them.PHPRequestHandler
must know how to start new PHP instances, but BasePHP shouldn't be concerned with that at all.separates PHP leverages two major building blocks:
Furthermore,
PHPRequestHandler
now uses PHPProcessManager to spawn new PHP instances on demand. To create a new request handler, you now need to pass aprocessManager
instance or, alternatively, aphpFactory
function.Ideally,
BasePHP
would have no more references toPHPRequestHandler
after this PR, but unfortunately we cannot fully decouple the two just yet.BasePHP
exposes anabsoluteUrl
,documentRoot
, and arequest
method that are used by Blueprints, and Blueprint steps only get aBasePHP
instance without any extra contextual information. This will be easier to resolve once we switch to PHP Blueprints where we'll never callphp.request()
and we'll also be able to access these contextual information via$_SERVER
and such.Follow up work
UniversalPHP
and such are interchangeable with bothBasePHP
andWebPHPEndpoint
. This isn't intuitive, especially now thatWebPHPEndpoint
acts on many PHP instances. Let's makeBasePHP
distinct fromWebPHPEndpoint
. They will share some methods and props, but will also have disjoint ones.Pre-requisite PRs
A large chunk of this mega-PR got split into smaller PRs and merged piece by piece in:
Testing instructions
Confirm the unit tests pass.
If you want to actually play with the feature, create the files described in this comment and:
Confirm PHP can run PHP via HTTP by visiting:
Confirm PHP can run PHP via shell by visiting: