laravel
[12.x] Fix serve command ignoring `PHP_CLI_WORKER_SERVERS` env
Changed
- In https://github.com/laravel/framework/pull/54606, the
artisan servecommand was changed to require a--no-reloadflag when using thePHP_CLI_WORKER_SERVERS. Instead of ignoring the config (which is the default in Laravel's scaffold), we can wait for the port to become available (for a few seconds) and throw an exception if it doesn't (clearly something went wrong and we can't recover)
Related PRs:
- https://github.com/laravel/framework/pull/54606
Context:
As I was working on the Hotreload package, I noticed the processes were getting stuck by the SSE endpoint. Turns out there was only 1 process, even though we set the PHP_CLI_WORKER_SERVERS in the default Laravel env.
The current way it's working right now, we're ignoring the desired number of processes configured. With the changes here, we'd respect that but disable the auto-reload behavior. This is important for the Hotreload package, but also for Sail, since the --no-reload flag is not being used in the default command there.
During development, the ability to restart serve command when .env changed should be the priority (unless specifically set with --no-reload). This may improve serve performance but at the same time make development experience worst for many developer as experienced in https://github.com/laravel/framework/issues/54574 (instead of --port getting change, they now will have updated .env ignored).
The only fix to prioritise PHP_CLI_WORKER_SERVERS is to handle process exit signal to all "workers" before restart a new process.
@crynobone I understand your point. Personally, I depend way more on having multiple processes when using artisan serve than on reloading the envs. I'm more used to restarting after config changes, since that's also the behavior on workers.
The only fix to prioritise
PHP_CLI_WORKER_SERVERSis to handle process exit signal to all "workers" before restart a new process.
Yeah, I explored sending a SIGHUP to the underlying process for it to reload configs and handling it inside the bootstrapers... but that felt way more effort than what I have time right now.
What if we displayed a "Automatic Reloading of Environment Variables is disabled." message when that behavior is disabled?
Send a fix to https://github.com/hotwired-laravel/hotreload/pull/2
Testbench handles this slightly differently, and would break with the above changes.
Oh, thanks!
The issue wasn't in the package itself, though. I noticed it in a Laravel app that was using the package, because the package requires more than one process due to the SSE route endpoint, and I found out it wasn't respecting the config for the number of procs. I updated the package docs to say "use the --no-reload flag" too.
But this is also happening on Sail, for instance, since it doesn't default to using the --no-reload, it ignores the config and defaults to a single process, besides Laravel defaulting to shipping with the PHP_CLI_SERVER_WORKERS=4...
But this is also happening on Sail, for instance, since it doesn't default to using the --no-reload, it ignores the config and defaults to a single process, besides Laravel defaulting to shipping with the PHP_CLI_SERVER_WORKERS=4
I still believe environment reload should be prioritized (see comment above) as PHP_CLI_SERVER_WORKERS doesn't work on Windows environment. The best solution for this is to fix the process restart when PHP_CLI_SERVER_WORKERS is more than 1.
@crynobone what if we waited for the port to become available instead? I've updated the behavior to check for it. It should wait a few seconds (should be more than enough) for that to happen. If it doesn't, something is wrong so we throw an exception.
After testing this, it doesn't seem the port will become available even after hours. Then I consider what's exactly running on the background:
From the screenshot, it is clearly shown that PHP_CLI_SERVER_WORKERS creates 5 PIDs to handle the request, and we need to find a way to remove all processes via Symfony Process.