Add support for signal handling #104

WyriHaximus · 2017-07-25T16:59:22Z

This PR adds OS signal handling. There are a few things of discussion, I'll post comments on the representing code lines.

Signal handlers:

PCNTL (feasible now that check return value of stream_select #20 has been resolved via check return value of stream_select including a test case #44)
Use internal signal handling when loops have support for it (Possible of dealing with SignalEvent #8 has more details)

WyriHaximus · 2017-07-25T17:02:00Z

src/Signal/Pcntl.php

+    public function on($signal, callable $listener)
+    {
+        if (!$this->enabled) {
+            return;


Do we want to fail silently or do we want to throw an exception?

It should throw, otherwise an app cannot depend on signal handling.

Leaning towards the same, most other options add either more methods or silently fail and the consumer doesn't have clue

Throwing an exception also makes sense to me 👍

WyriHaximus · 2017-07-25T17:03:09Z

src/Signal/Pcntl.php

+        }
+
+        if ($this->timer === null) {
+            $this->timer = $this->loop->addPeriodicTimer(0.1, function () {


Is checking for signals 10 times a second enough or should we do it 25 times a second or X per second?

Calling pcntl_signal_dispatch() can simply be rolled into the main event loop after calling stream_select(), since the latter is interrupted by the arrival of signals, regardless of the timeout parameter.

Need to check, but that would only be for the stream_select() loop right? As the other have signaling build in

Right, I made that comment before I realized you were using the object with other loop implementations. Integrating it into StreamSelectLoop should be the eventual goal then.

Agreed, integration into the event loop is the ultimate goal here 👍

In the meantime, 0.1s is either too slow or too fast depending on what you're doing, so we may as well add an argument or constant at least.

WyriHaximus · 2017-07-25T17:09:20Z

tests/AbstractLoopTest.php

+    public function testSignal()
+    {
+        if (!extension_loaded('pcntl')) {
+            $this->markTestSkipped('Signal test skipped because PCNTL extension isn\'t loaded.');


Another way around the support of PCNTL would be an extra method that could be called to check if the loop supports signals. Thoughts?

It could… though if an app depends on signals, likely it will throw anyway, so may as well throw from the onSignal() method. Note that libevent, libev, and libuv support signal handling without the need of the pcntl extension.

Note that libevent, libev, and libuv support signal handling without the need of the pcntl extension.

Yeah it's on the list for this PR 😎

trowski · 2017-07-25T17:47:51Z

src/LibEvLoop.php


    public function __construct()
    {
        $this->loop = new EventLoop();
        $this->futureTickQueue = new FutureTickQueue();
        $this->timerEvents = new SplObjectStorage();
+        $this->signals = new Pcntl($this);


LibEvLoop should use the signal handling provided by the extension, as it is more reliable and integrates with the loop backend being used by the extension. Same goes for LibEventLoop and ExtEventLoop.

It's on the list at the top ^ 😎

Ah, so it is, missed that when focusing on the diff. 👍

No problem, your input is much appreciated 👍

clue

Nice, I really like this feature and the direction this is heading! 👍 I think @trowski made some very good points about integration with other event loop implementations here 👍

clue · 2017-07-25T19:05:47Z

README.md

+be notified about OS signals. This is useful to catch user interrupt signals or 
+shutdown signals from tools like `supervisor` or `systemd`.
+
+The 


clue · 2017-07-25T19:08:18Z

src/LoopInterface.php

+     *
+     * @return void
+     */
+    public function onSignal($signal, callable $listener);


Should probably be named addSignal() for consistency with other methods?

Also, the docs should probably include whether multiple listeners can be attached to the same signal?

clue · 2017-07-25T19:09:15Z

src/LoopInterface.php

+     *
+     * @return void
+     */
+    public function removeSignal($signal, callable $listener);


Can the same listener be added multiple times? If so, which one will be removed here?

Also, the docs should probably include what happens if the given signal or listener is not attached (NO-OP just like other methods?).

clue · 2017-07-25T19:12:50Z

src/Signal/Pcntl.php

+    public function on($signal, callable $listener)
+    {
+        if (!$this->enabled) {
+            return;


Throwing an exception also makes sense to me 👍

clue · 2017-07-25T19:14:16Z

src/Signal/Pcntl.php

+        }
+
+        if ($this->timer === null) {
+            $this->timer = $this->loop->addPeriodicTimer(0.1, function () {


Agreed, integration into the event loop is the ultimate goal here 👍

In the meantime, 0.1s is either too slow or too fast depending on what you're doing, so we may as well add an argument or constant at least.

clue · 2017-07-25T19:15:50Z

tests/AbstractLoopTest.php

+        });
+
+        $this->loop->futureTick(function () {
+            posix_kill(posix_getpid(), SIGUSR1);


Note that these functions may be disabled http://php.net/manual/en/posix.installation.php

Given this is a unit test I'm not that worried. Might put an if and fail in the if the function isn't available

Just skip the test if it isn't available.

WyriHaximus · 2017-07-27T10:29:39Z

@clue @trowski Reworked the signal handling for stream_select to use PCNTL and the other loops to use the signaling provided by the underlying loops.

The question is what to do with trying to add the same listener more then once. We can either throw, ignore it silently, or just allow it. I've written it now in such a way that the latter works (or better should work, added tests for it this afternoon) and also removes all occurrences of a listener when you remove it.

WyriHaximus · 2017-07-28T06:58:52Z

Had to rework the signal handler because not all loops support using the same listener more then once. That has all been resolve now and they all behave the same 😎 . Personally I think this would be the most transparent way of dealing the reused listeners.

clue · 2017-08-14T14:24:23Z

src/ExtEventLoop.php

+            $this->signalEvents[$signal]->add();
+        });
+        $this->signals->on('off', function ($signal) {
+            $this->futureTick(function () use ($signal) {


Does this have to be in a future tick? This looks unexpected from a consumer perspective and would at least need some documentation here? I haven't verified this, but it looks like this could potentially cause a race condition, can you verify this?

It does, the issue is that certain loops throw errors when you try to remove a signal handler while handling the given signal. In order to mitigate the race condition each loop does a check to ensure there are no registered signal handlers for the given signal. Should add documentation for it.

clue · 2017-08-14T14:25:34Z

composer.json

@@ -4,15 +4,17 @@
    "keywords": ["event-loop", "asynchronous"],
    "license": "MIT",
    "require": {
-        "php": ">=5.4.0"
+        "php": ">=5.4.0",
+        "evenement/evenement": "^3.0||^2.0"


I know this dependency is used throughout the ecosystem, but is it really worth it or should we try to keep this component stand-alone?

Well given the signal handler only has an on and an off even called once we don't really need it. I'll refactor and remove it.

clue · 2017-08-14T14:26:24Z

README.md

+
+**Note: A listener can be added more then once to the same signal and it will be called 
+for each time it is added. But on [`removeSignal`](#removeSignal) it is removed for all 
+the times it has been added. 


This behavior looks really odd to me. Does it make sense to either reject adding the same listener multiple times or only remove a single instance so adding/removing behaves consistent?

Indeed, it should either error out or have another mechanism for removing.

Removing a single instance would be easy to do. But it might be more consistent to only allow one occurrence of a instance per signal in the first place.

clue · 2017-08-14T14:27:36Z

composer.json

    },
    "require-dev": {
        "phpunit/phpunit": "~4.8"
    },
    "suggest": {
        "ext-libevent": ">=0.1.0",
        "ext-event": "~1.0",
-        "ext-libev": "*"
+        "ext-libev": "*",
+        "ext-pcntl": "For signals support"


Afaict this is only need for the default loop, right? Documentation should probably be updated to explicitly say when this extension is recommended?

Correct, I assume you mean the documentation here and in README.md. I'll update both

clue · 2017-08-14T14:28:48Z

src/LoopInterface.php

+     *
+     * @throws UnsupportedFeatureException when signals
+     * aren't supported by the loop, e.g. when required
+     * extensions are missing.


Can this method really throw? Looks like adding isn't possible in this case either, so this actually boils down to removing a non-existent listener?

It is more about consistency between two methods than it is likely someone will call this method when addSignal throws.

I still think that this situation shouldn't throw. Removing a non-existent listener does not throw, so why would removing a listener throw when it's impossible to add listeners in the first place?

Makes sense to me, they can catch the exception when trying top add it. I'll remove the exception on removing signal listeners 👍

clue · 2017-08-14T14:30:52Z

src/StreamSelectLoop.php

@@ -196,6 +237,9 @@ private function waitForStreamActivity($timeout)
        $write = $this->writeStreams;

        $available = $this->streamSelect($read, $write, $timeout);
+        if ($this->pcntl) {
+            \pcntl_signal_dispatch();
+        }


Does this need to be checked after every loop iteration and does this cause any performance issues? It's my understanding that a signal would interrupt the select() call checked below, no?

A signal interrupts stream_select(), correct.

Signals aren't picked up when I change if ($this->pcntl) { into if (false === $available && $this->pcntl) {only dispatching when the loop received a signal. Not aware of any performance degradations.

kelunik · 2017-08-14T14:40:42Z

src/SignalsHandler.php

+            if (isset($this->signals[$signal]) && \count($this->signals[$signal]) === 0) {
+                unset($this->signals[$signal]);
+                $this->emit('off', [$signal]);
+            }


That if can be moved below the while loop, no?

Yup, kinda depends on the result of the discussion here: #104 (comment)

kelunik · 2017-08-14T14:43:50Z

src/StreamSelectLoop.php

+        $this->signals->on('on', function ($signal) {
+            \pcntl_signal($signal, function ($signal) {
+                $this->signals->call($signal);
+            });


That traps the signal and installs a process-wide handler. I guess it should at least be cleaned up in something like __destruct(). Behavior with multiple loops needs to be defined somehow.

kelunik · 2017-08-14T21:04:38Z

That traps the signal and installs a process-wide handler. I guess it should at least be cleaned up in something like __destruct(). Behavior with multiple loops needs to be defined somehow.

Repeating that comment so it doesn't get hidden by changes.

WyriHaximus · 2017-08-24T10:14:17Z

@clue @kelunik Updated the PR after your comments with changes taking those in mind

kelunik · 2017-08-24T11:10:42Z

@WyriHaximus why are duplicates silently ignored instead of erroring out?

WyriHaximus · 2017-08-24T12:45:20Z

@kelunik because it is consistent with how stream callables are handled

jsor

Approved from a user presepective and delegating technical approval to @clue.

WyriHaximus · 2017-10-10T08:26:32Z

Ping @clue

clue

I like the direction this is heading! Only added some minor remarks:

clue · 2017-10-11T18:55:38Z

examples/04-signals.php

+
+$loop = React\EventLoop\Factory::create();
+
+$loop->onSignal(SIGINT, $func = function ($signal) use ($loop, &$func) {


clue · 2017-10-11T18:58:12Z

src/ExtEventLoop.php

+                $this->signalEvents[$signal]->add();
+            },
+            function ($signal) {
+                $this->futureTick(function () use ($signal) {


Just curious, why does this use futureTick() and what consequence does this have? (see also below)

In come loops removing the signal handled within the function handling a signal will make it fail hard. Another way would be to use future tick in the SignalHandler when receiving the signal but that means the SignalHandler becomes aware of the event loop.

Not sure I follow, can you provide a gist that shows this problem? Reading the documentation of our event loop implementations I could not find anything that supports this claim.

Hmm odd can't reproduce is niet 🤐 . I'll remove the ticks

And I'll add them again because this is what happens: https://travis-ci.org/reactphp/event-loop/jobs/290513376#L1401

Interesting find! Looks like this was actually bug in PHP (https://bugs.php.net/bug.php?id=62452) that was fixed with PHP 7. Removing this in the next tick has some consequences, so I wonder if this can be worked around by keeping a reference to the closure instead (see also the PHP bug for some workarounds).

@clue yeah also know why I couldn't see it locally, didn't ran the tests on 5.6 locally 🤐 . Just committed a possible fix, since I can't test locally I'm going to run them on Travis and squash the commits once fixed if it needs more then one try.

@clue resolved the issue 🎉

clue · 2017-10-11T19:01:02Z

src/LoopInterface.php

+     *
+     * @throws UnsupportedFeatureException when signals
+     * aren't supported by the loop, e.g. when required
+     * extensions are missing.


I still think that this situation shouldn't throw. Removing a non-existent listener does not throw, so why would removing a listener throw when it's impossible to add listeners in the first place?

clue · 2017-10-11T19:04:38Z

src/UnsupportedFeatureException.php

+    {
+        return new self('Event loop feature "' . $feature . '" isn\'t supported by the "' . $loopName . '" event loop');
+    }
+}


This adds the first custom exception to this package, so I would much rather discuss this in a separate PR and remove here for now? 👍 A BadMethodCallException seems to be a good replacement for now.

Why Bad method call?

@kelunik What was your suggestion again?

Exception thrown if a callback refers to an undefined method or if some arguments are missing.

BadMethodCallException is usually used in combination with __call, not if requirements for a call are missing.

My suggestion is to keep a custom exception that makes things clear.

@clue personally I moving more and more towards custom exceptions for each use case so the error handled in a specific manner for that error. Maybe we should have a discussion about if we want that in ReactPHP before adding a custom exception to a packages. I'll change it unless you and @jsor prefer this idea of a custom exception per error.

I'm not suggesting that "custom exception per error" is not a good idea (see above), merely that I think it makes sense to discuss this in a separate ticket because I would love to get this feature in already :-) Also note that this is the first method can may throw in the first place.

Alright I've swap it out with a core exception and we can have this discussion in another issue so we can get this feature in

clue · 2017-10-11T19:06:20Z

tests/SignalsHandlerTest.php

+
+final class SignalsHandlerTest extends TestCase
+{
+    public function testEmittedEventsAndCallHandling()


Signal constants are only defined when PCNTL is loaded: http://php.net/manual/en/pcntl.constants.php

This test should probably check for this first. Also, maybe add a remark about this to the README?

Defining in tests/bootstrap.php when they don't exist in case PCNTL isn't loaded

kelunik · 2017-10-12T07:29:34Z

tests/bootstrap.php

+
+if (!defined('SIGUSR2')) {
+    define('SIGUSR2', 2);
+}


How about making a package both Amp and ReactPHP can use that polyfills the signal constants?

For two constants not really. Seems way to much overhead tbh.

It's not only those two. It's also for using them with other reactor implementations if pcntl isn't available.

@kelunik I agree with @WyriHaximus, this is probably overkill for this test suite. But I agree that it may make sense for consumers that want to rely on signal constant names, so I'd suggest go for it and create that package

kelunik · 2017-10-12T10:29:08Z

examples/04-signals.php

@@ -10,6 +10,7 @@
    $loop->stop();
 });

+// This timer is here to keep the loop alive, signaling an-sich doesn't do that


an-sich -> by ifself

kelunik · 2017-10-12T10:34:25Z

@WyriHaximus Could you add a test that a signal watcher doesn't keep the loop running if no other watcher exists?

WyriHaximus · 2017-10-12T15:28:40Z

@kelunik just added the test and waiting for the results from travis

kelunik · 2017-10-12T15:31:45Z

src/UnsupportedFeatureException.php

+{
+    public static function create($feature, $loopName)
+    {
+        return new self('Event loop feature "' . $feature . '" isn\'t supported by the "' . $loopName . '" event loop');


I'd go for a regular constructor here. The message should probably cover why the feature isn't supported (missing ext-pcntl). The $loopName should also be the class name instead of "stream_select" IMO, but I'd argue that it's not needed in the message, because the used implementation will show up in the stack trace.

WyriHaximus · 2017-10-26T06:00:41Z

Ping @clue

clue · 2017-10-30T08:49:27Z

README.md

@@ -292,6 +292,37 @@ echo 'a';

 See also [example #3](examples).

+### addSignal()
+
+**Note: Signaling is only available on Unix-like platform, Windows isn't supported.**


I think that it makes sense to not support Windows in the first step. 👍

However, note that others (https://docs.python.org/3/library/signal.html#signal.signal or nodejs/node-v0.x-archive#1553) do provide limited, simulated signal handling supports on Windows. As such, I would suggest moving this note down a bit and make it more clear that this is not an interface limitations.

We should probably document the exception that we throw when signal handling is not supported. This should also cover Windows compatibility.

clue · 2017-10-30T08:51:45Z

README.md

+See also [example #4](examples).
+
+**Note: A listener can only be added once to the same signal, any attempts to add it 
+more then once will be ignored.**


No objection to this limitation, but I would suggest making sure this is closer to the verbiage for our stream API (https://github.com/reactphp/event-loop/pull/110/files#diff-04c6e90faac2675aa89e2176d2eec7d8R312). What do you think about this?

Updated it, it isn't my strongest skill and I rather not let it block this PR thus I suggest if it needs work I do it in a follow up PR.

clue · 2017-10-30T08:54:42Z

tests/AbstractLoopTest.php

+        $this->loop->addTimer(0.1, function () {});
+
+        $this->assertRunFasterThan(0.2);
+    }


clue · 2017-10-30T08:58:24Z

examples/04-signals.php

+    $loop->stop();
+});
+
+// This timer is here to keep the loop alive, signaling by itself doesn't do that


This appears unexpected to me and is apparently undocumented. I'm curious for the reasoning and documentation here, as I would clearly have expected this to behave otherwise. What is the motivation for not making this "keep the loop alive"? (Keeping consistency with other event loop listeners in mind here)

Also note that this is related to #107.

The motivation to not keeping the loop alive with signals was that there where not inputs into the loop kicking off more activity besides the signals. But now that I think about it signals COULD be used to kick off more activity. I'll update this PR shortly using a timer to keep the loop alive as long as signals are registered to it.

WyriHaximus · 2017-10-30T14:25:07Z

@clue reworked the PR so signals now keep the loop running

clue · 2017-11-04T16:38:55Z

examples/04-signals.php

+$loop->addSignal(SIGINT, $func = function ($signal) use ($loop, &$func) {
+    echo 'Signal: ', (string)$signal, PHP_EOL;
+    $loop->removeSignal(SIGINT, $func);
+    $loop->stop();


The $loop->stop() doesn't do any harm, but it's not really needed anymore, right?

Nope no need for it there \o/

clue · 2017-11-04T16:41:08Z

src/SignalsHandler.php

+    {
+        if (count($this->signals) == 0 && $this->timer === null) {
+            $this->timer = $this->loop->addPeriodicTimer(1, function () {});
+        }


I would suggest using a much higher interval do prevent the CPU from waking up every second. Also, maybe add a simple comment what this does?

Don't really see the harm in once a second but increased it to five minutes

@clue

…suggested by @clue at reactphp#104 (comment)

WyriHaximus · 2017-11-04T22:21:00Z

@clue updated as per your comments 👍

WyriHaximus · 2017-11-08T21:27:05Z

@clue solved merge conflict, could you review again?

clue

Awesome job @WyriHaximus! 🎉 This should be squashed to a reasonable number of commits, otherwise LGTM! 🎉

$ git fetch origin
$ git checkout stoplichten
$ git rebase -i origin/master
# squash/fixup as applicable

WyriHaximus added easy pick help wanted new feature labels Jul 25, 2017

WyriHaximus added this to the v0.5.0 milestone Jul 25, 2017

WyriHaximus requested review from jsor and clue July 25, 2017 16:59

WyriHaximus changed the title ~~Stoplichten~~ OS signaling Jul 25, 2017

WyriHaximus commented Jul 25, 2017

View reviewed changes

trowski reviewed Jul 25, 2017

View reviewed changes

clue reviewed Jul 25, 2017

View reviewed changes

clue reviewed Aug 14, 2017

View reviewed changes

kelunik reviewed Aug 14, 2017

View reviewed changes

jsor approved these changes Aug 29, 2017

View reviewed changes

kelunik mentioned this pull request Oct 10, 2017

Support reference / unreference of watchers #107

Open

clue removed the easy pick label Oct 11, 2017

clue reviewed Oct 11, 2017

View reviewed changes

kelunik reviewed Oct 12, 2017

View reviewed changes

kelunik suggested changes Oct 12, 2017

View reviewed changes

WyriHaximus force-pushed the stoplichten branch from db0bda0 to 3f5457f Compare October 20, 2017 18:06

clue reviewed Oct 30, 2017

View reviewed changes

WyriHaximus force-pushed the stoplichten branch from 41606c1 to 65c0a9b Compare October 30, 2017 15:07

clue reviewed Nov 4, 2017

View reviewed changes

WyriHaximus added a commit to WyriHaximus-labs/event-loop that referenced this pull request Nov 4, 2017

Remove $loop->stop(); as caught by @clue at reactphp#104 (comment)

7389831

WyriHaximus added a commit to WyriHaximus-labs/event-loop that referenced this pull request Nov 4, 2017

Increase the keep alive timer to 5 minutes instead of each second as …

4a61948

…suggested by @clue at reactphp#104 (comment)

clue approved these changes Nov 10, 2017

View reviewed changes

clue changed the title ~~OS signaling~~ Add support for signal handling Nov 10, 2017

Support for signal handling

f8ad016

WyriHaximus force-pushed the stoplichten branch from 01da14f to f8ad016 Compare November 10, 2017 16:52

WyriHaximus merged commit 55f2d96 into reactphp:master Nov 10, 2017

clue mentioned this pull request Nov 11, 2017

[WIP] moved signal handling tests to AbstractLoopTest #46

Closed

This was referenced Feb 6, 2018

Properly clean up event watchers for ext-event and ext-libev #149

Merged

Refactor and simplify signal handling logic #150

Merged

cebe mentioned this pull request May 17, 2018

Update composer.json mkraemer/react-pcntl#10

Closed

TBK mentioned this pull request Jun 11, 2018

update reactphp event-loop version php-pm/php-pm#386

Closed

clue mentioned this pull request Feb 6, 2019

addSignal with 100% cpu #175

Closed


		$loop = React\EventLoop\Factory::create();

		$loop->onSignal(SIGINT, $func = function ($signal) use ($loop, &$func) {

Add support for signal handling #104

Add support for signal handling #104

Conversation

WyriHaximus commented Jul 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trowski Jul 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clue left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WyriHaximus commented Jul 27, 2017

WyriHaximus commented Jul 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kelunik Aug 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kelunik commented Aug 14, 2017

WyriHaximus commented Aug 24, 2017

kelunik commented Aug 24, 2017

WyriHaximus commented Aug 24, 2017

jsor left a comment

Choose a reason for hiding this comment

WyriHaximus commented Oct 10, 2017

clue left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WyriHaximus commented Jul 25, 2017 •

edited

Loading

trowski Jul 25, 2017 •

edited

Loading

kelunik Aug 14, 2017 •

edited

Loading