seregazhuk / php-watcher

Monitor for any changes in your php application and automatically restart it (suitable for async apps).
https://sergeyzhuk.me/2019/10/18/php-watcher/
389 stars 31 forks source link

Option to prevent running process being wrapped into sh call #21

Closed gorbunov closed 5 years ago

gorbunov commented 5 years ago

I run into small problem while trying to run watcher on daemonized app, that processes posix signals, including SIGTERM, which is used to restart process. Since process wrapped into sh call, it was unable to correctly shutdown, release tcp port, which lead to defunct sh process on restart (since new process was unable to bind socket) and internally process kept watching that defunct sh process with wrong pid. I was able to track down this behavior into reactphp/childprocess (and it's documented feature), and added new command-line option to avoid this: --unwrap

Not sure if it's a feature you want to add, tho.)

seregazhuk commented 5 years ago

Thank you for the request! That's interesting 🤔 How can I reproduce that? Your test actually doesn't test it. It just checks the output. Can we somehow send a signal to the running script in a child process and check that it was correctly handled?

seregazhuk commented 5 years ago

I have tried something like this:

declare(ticks = 1);

pcntl_signal(SIGTERM, "handler");

while (true) {
    echo 'test' . PHP_EOL;
    sleep(1);
}

function handler() {
    echo 'Handled' . PHP_EOL;
    exit;
}

When the watcher restarts this script, it handles the signal.

gorbunov commented 5 years ago

Sorry, I was planning to make more clean test for that today) I actually was testing all this inside docker container on working reactphp app, so I wasn't able extract simple test case easily.

seregazhuk commented 5 years ago

Maybe it is a docker issue 🤔 Anyway, let me know if there is a way to reproduce it in the test. Because for me it looks like a bug and the process should receive SIGTERM on restart.

seregazhuk commented 5 years ago

ping @gorbunov. You were right about handling signals inside docker container 👍 Even Symfony process component uses exec inside.

I have created a PR to reproduce it. The test failed in Travis without using exec.

gorbunov commented 5 years ago

Wow, nice! I was away for couple of days, glad it helped! But I wonder, if this configuration option should be actually reversed, and preferred as default behavior: maybe --no-exec option ? I may be wrong, but I feel like watching dockerized servers may be case much more often than compiled php with custom signaling option?

seregazhuk commented 5 years ago

I'm not sure either 😄 For me it seems the correct solution when a child process receives signals. And even Symfony Process component uses the same approach. I think I can leave it as it is. If there will be any related issues I'll come back to it 👍