Skip to content
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

Allow stopping commands properly #10

Merged
merged 1 commit into from
Nov 8, 2024

Conversation

SamuelNitsche
Copy link
Contributor

@SamuelNitsche SamuelNitsche commented Nov 8, 2024

While using solo, I noticed there were some issues when trying to signal commands like php artisan serve, php artisan horizon, etc.
This is immediately visible in the case of serve, since the tasks can't be stopped and started again because of the old process still running and blocking the port.
As you can see in the screenshot below, the Symfony Process itself wraps the actual command defined in the service provider with another process. This is documented here

So when the process was stopped, only the wrapper process itself was stopped but not the actual php artisan serve process.

before

When starting the process using an array command instead of a string command, Symfony will not wrap it in a subprocess. This way we can stop the actual php artisan serve command process instead of its wrapper process.

after

I tested this manually with all the default commands solo ships however I am not entirely sure if and how this way of starting the process has some drawbacks.

A lot of command and process here, I hope this makes sense.

I think this is also the reason why pail didn't stop here

@aarondfrancis
Copy link
Owner

Oh I think this is the ticket. Great catch. I think I have one more thing to add and then I'll merge this. Thank you so much for the investigation

@aarondfrancis
Copy link
Owner

Actually I think it's perfect as is. I was gonna look at at Process::fromShellCommandline but it suffers from the same problem: https://github.com/symfony/symfony/blob/7.1/src/Symfony/Component/Process/Process.php#L175.

Thank you!

@aarondfrancis aarondfrancis merged commit 6d32f59 into aarondfrancis:main Nov 8, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants