search

spatie / laravel-horizon-watcher

Automatically restart Horizon when local PHP files change
https://spatie.be/open-source
MIT License
218 stars 15 forks source link

Trap signals rather than killing processes #16

Closed joelvh closed 1 year ago

joelvh commented 1 year ago

We were debugging rogue processes and didn't realize the package had a fix for the issue already. However, we believe the more "native" solution here is to leverage Watch::shouldContinue with Laravel's built-in trap for signal trapping.

This simplifies the code and doesn't run another shell command.

BTW we have been using spatie/laravel-signal-aware-command and found that it did not work as expected with signal trapping, and found the native solution works more reliably, as seen in this solution.

freekmurze commented 1 year ago

Very nice, thank you!

sdapkus commented 1 year ago

For me this doesn't work. After making some changes in a project and killing horizon I get this log:

$ sail artisan horizon:watch

   INFO  Starting Horizon and will restart it when any files change...  

Horizon started successfully.
   INFO  Change detected! Restarting horizon...  

Horizon started successfully.
   INFO  Change detected! Restarting horizon...  

Horizon started successfully.
   INFO  Change detected! Restarting horizon...  

Horizon started successfully.
^CShutting down...
Shutting down...
Shutting down...
Shutting down...

Also in a Horizon dashboard I can see that after each change new worker is started.

image

I have double check 1.0.4 version - all worker processes between reloads seems to be killed normally. My environment:

freekmurze commented 1 year ago

@joelvh could you have a look?

joelvh commented 1 year ago

Hi @sdapkus @freekmurze,

It appears that this is a bug that was already present. This PR addressed signal trapping, but didn't account for the signal forwarding piece that the previous process killing was addressing.

At some point, we generalized this watcher command to watch any process we want, and we fixed it in our version of the command. FWIW, I wasn't aware that it was something that was a pre-existing bug here as well.

The fix in #22 should do the trick. @sdapkus if you could please verify. Thanks!

binaryfire commented 10 months ago

We're also experiencing this when running Horizon inside a container: https://github.com/spatie/laravel-horizon-watcher/issues/26.

Has anyone found a workaround? We've got a remote team and our app has a lot of services, so running Horizon outside of our Docker development environment isn't an option unfortunately.