8000 addSignal with 100% cpu · Issue #175 · reactphp/event-loop · GitHub
[go: up one dir, main page]
Skip to content

addSignal with 100% cpu #175

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.

Sign up for GitHub

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

Jump to bottom
Closed
yizhihouzi opened this issue Nov 6, 2018 · 6 comments
Closed

addSignal with 100% cpu #175

yizhihouzi opened this issue Nov 6, 2018 · 6 comments
Labels
bug

Comments

@yizhihouzi
Copy link
yizhihouzi commented Nov 6, 2018 •
edited
Loading

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
Copy link
Member

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 clue added the bug label Nov 12, 2018
@clue
Copy link
Member
clue commented Nov 12, 2018

@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
Copy link
Author

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

@WyriHaximus
Copy link
Member

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.

@ghost
Copy link
ghost commented Jan 20, 2019 •
edited by ghost
Loading

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.

@ghost ghost mentioned this issue Jan 20, 2019
Closed
@clue clue mentioned this issue Feb 6, 2019
Merged
@clue
Copy link
Member
clue commented Feb 6, 2019

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).

@jsor jsor closed this as completed in #183 Feb 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@WyriHaximus @clue @yizhihouzi
0