Skip to content
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.

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

Implementations of ::run() use signals->isEmpty() which prevents exiting #218

Closed
idimsh opened this issue Dec 26, 2020 · 3 comments
Closed
Labels

Comments

@idimsh
Copy link

idimsh commented Dec 26, 2020

In all implementations of \React\EventLoop\LoopInterface::run

$this->signals->isEmpty()

is part of the conditions to check if there is something else to do.
This is not really correct, the loop after executing all the added Timers is waiting for a the signals registered to response to and make

$nothingLeftToDo

become true.
It is not really required to wait for the signal, what if the signal is not received? The loop run() keep going until it is either stop() or all the registered signals have been triggered.

Registering signals should mean: what to do if this signal received, and not that registered signals have to be received.

sample

<?php
declare(strict_types=1);
require_once '../../vendor/autoload.php';

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

$loop->addTimer(
    0.01,
    function () use ($loop) {
        $loop->addSignal(
            SIGUSR1,
            function (int $signal) {
                fputs(STDOUT, "signal [$signal] received\n");
            }
        );
        fputs(STDOUT, "in first callback\n");
    }
);
$loop->addSignal(
    SIGINT,
    function (int $signal) {
        fputs(STDOUT, "signal [$signal] received\n");
    }
);

fputs(STDOUT, "Now it will keep waiting for SIGUSR1 && for SIGINT, clicking Ctrl-c will not end the process \n");
$loop->run();
@WyriHaximus
Copy link
Member

All event loops will continue running as long as they have something to do. Once you'll remove everything from them they'll automatically stop. So in your example, you have two single handlers registered with the loop that will keep it running even after you've received a signal because you don't remove the handlers from the event loop.

@idimsh
Copy link
Author

idimsh commented Dec 27, 2020

"as long as they have something to do" this is the conflict point, from signal handling point of view they have to do something if the signal comes in, otherwise they have nothing to do, this is my point of view also.
As for removing the handlers, me and the user of the lib can do it if there is an indication that the loop reached the end of firing all the registered Time-ly handlers and only signals handlers are left, again there is no API for such information.

Another problem is all concrete implementations are 'final' and there is no way to extend them. Also adding methods to the interface that gives back information about the status and what is left does not seem right.

May adding some configuration parameters or flags to the constructors be a good solution?

@clue clue added the question label Dec 27, 2020
@clue
Copy link
Member

clue commented Dec 27, 2020

@idimsh Thanks for bringing this up, I understand that this can be confusing and/or annoying!

As @WyriHaximus already pointed out, this behavior is indeed by design. There's some discussion in #107 about being able to add "unreferenced watchers" in the future, but at this point in time any timer/stream/signal will always keep the loop running. There are very large number of programs that rely on this current behavior and we're not aware of any issues, so I still think this makes sense as the default behavior. Please contribute to the referenced ticket if you feel that's something that needs to be worked on nonetheless 👍

For the time being, you can always unregister your signal handlers if you don't want to keep the loop running any longer. The loop will automatically stop once there's nothing left to do. This is something that has worked out in plenty of programs, so I think this should also work for your use case afaict:

$loop->addSignal(
    SIGINT,
    $fn = function (int $signal) use ($loop, &$fn) {
        fputs(STDOUT, "signal [$signal] received\n");
        $loop->removeSignal($signal, $fn);
    }
);

I believe this has been answered, so I'm closing this for now. Please come back with more details if this problem persists and we can always reopen this 👍

@clue clue closed this as completed Dec 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants