addSignal with 100% cpu

[yizhihouzi]

Hello, I have a question.
Why the code blow run with 100% CPU, any question with the code?

require __DIR__ . '/../vendor/autoload.php';

use React\EventLoop\Factory;

$loop = Factory::create();
$loop->addSignal(SIGTERM, function () {
});
$loop->run();

[WyriHaximus]

If all you add is checking for the SIGTERM it will only do that and in rapid succession of each check. But when you add timers or streams if will have more to do and wait for.

[clue]

@yizhihouzi Thanks for reporting, I can reproduce this running one the examples (https://github.com/reactphp/event-loop/blob/0266aff7aa7b0613b1f38a723e14a0ebc55cfca3/examples/04-signals.php) with the default StreamSelectLoop and agree that this should not cause busy waiting.

It's my understanding that this only happens with the default StreamSelectLoop and when you only have signals added to the loop with no other stream listeners or timers. I'll look into filing a PR soon-ish.

In the meantime, this can simply be worked around by adding a dummy timer to the loop as suggested by @WyriHaximus, so this expected to have little impact in real-word applications 👍

[yizhihouzi]

@clue yes,I found this problem inadvertently , and I just want to report this bug , there's no impact in real-world.

[WyriHaximus]

So I've been thinking about this, and we could add some kind of timer to avoid this. But I kinda feel that would be patching a very odd edge case that wouldn't/shouldn't exist in the wild.

Is there any real world use case where signals should keep an event loop going, if there is nothing else of interest going on? I'd actively vote to remove this from all loop implementations. I'm arguing that signals are never of active interest and shouldn't prevent a loop from exiting solely because there are signal watchers.

[clue]

Is there any real world use case where signals should keep an event loop going, if there is nothing else of interest going on? I'd actively vote to remove this from all loop implementations. I'm arguing that signals are never of active interest and shouldn't prevent a loop from exiting solely because there are signal watchers.

I think this is debatable, but arguably out of scope for this ticket. Also, this has been introduced via #104 and changing this behavior would be a BC break.

Either way, I've just filed #183 as a simple fix for this specific problem. I do not expect this to affect many real-world use cases, but it makes it work as expected at least (see example 4).