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

Register shutdown function to clear event loop #101

Closed
wants to merge 2 commits into from

Conversation

trowski
Copy link
Member

@trowski trowski commented Dec 7, 2024

This repeatedly registers shutdown functions which run the event loop until a function does not discover any additional enabled and referenced callbacks in the event loop. The first invocation of the shutdown function always registers another invocation so that any shutdown functions registered during script execution are executed before the second invocation.

There are edge cases where a shutdown function registering another shutdown function which then adds callbacks to the event loop may not be executed, but I consider those cases highly unlikely. I think this will cover every practical use case to avoid unexecuted events on script termination.

Testing shutdown functions in unit tests is difficult, so I added an example which demonstrates the new shutdown behavior.

$driver = self::getDriver();

$pending = false;
foreach ($driver->getIdentifiers() as $identifier) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this also check for pending microtasks/queued callbacks?

Copy link
Member Author

@trowski trowski Dec 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, you're right. I guess AbstractDriver::isEmpty() also isn't doing that and maybe should (though that function is private and due to how the queue is invoked, I'm not sure there ever can be queued microtasks when isEmpty() is called).

Unfortunately there is nothing on Driver which will let me query if there are queued microtasks, so that would need to be added.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my code, I just unconditionally do

                if (!EventLoop::getDriver()->isRunning()) {
                        $delay = PHP_SAPI === 'cli' ? 5 : 1;
                        $id2 = null;
                        EventLoop::unreference($id1 = EventLoop::delay((float)$delay, static function () use ($delay, &$id2): void {
                                $t = time() - $delay;
                                EventLoop::unreference($id2 = EventLoop::repeat(1.0, static function () use ($t): void {
                                        $t = time() - $t;
                                        Logger::get()->error("Still waiting for shutdown of the event loop for {$t} seconds...");
                                }));
                        }));
                        EventLoop::run();
                        EventLoop::cancel($id1);
                        if ($id2 !== null) {
                                EventLoop::cancel($id2);
                        }
                }

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i.e. an unconditional run()

@danog
Copy link
Contributor

danog commented Dec 7, 2024

Something like

EventLoop::unreference(EventLoop::onSignal(SIGINT, function () { throw new SignalException(); }));
getStdin()->read();

will cause a hang here, unless we getStdin()->unreference() in onShutdown (this is how I do it in MadelineProto).

@danog
Copy link
Contributor

danog commented Dec 7, 2024

While the issue of unreference watchers is generalized, a CTRL-C while reading from STDIN is an especially nasty case for CLI applications, and an extremely common one too.

@bwoebi
Copy link
Contributor

bwoebi commented Dec 7, 2024

I'm not sure whether we want to happen this unconditionally. I mean, the application may be in an unexpected state upon shutdown being called, especially when the cause is a fatal error or uncaught exception.

I feel like this should be configurable.

Also, how is a server application going to even actually abort on error? (at least unless the shutdown handler then throws or fatals again)

@bwoebi
Copy link
Contributor

bwoebi commented Dec 7, 2024

... Is it possible to just check whether the cause of shutdown is an error? And if yes, don't run the event loop automatically. Then we also don't need configuration.

(I see, @danog had basically the same concern.)

@trowski
Copy link
Member Author

trowski commented Dec 7, 2024

Well that pretty much spoils this idea. I forgot thrown exceptions would still trigger a shutdown function. Back to the drawing board I suppose. I'm not sure there's a good way to solve this problem outside of recommending applications use EventLoop::run() instead of relying on suspending {main}.

@trowski trowski closed this Dec 7, 2024
@bwoebi
Copy link
Contributor

bwoebi commented Dec 7, 2024

@trowski Shouldn't just a simple check for error_get_last()["type"] == E_ERROR (and the other few fatal type errors like E_CORE_ERROR) and such be enough?

@danog
Copy link
Contributor

danog commented Dec 7, 2024

+1 @bwoebi Yep I share the concerns.

There are two main usecases for this:

  1. Partially synchronous (fpm) applications using amphp libraries: since running the event loop will mainly just finish flusing data to sockets, thus the behavior in this PR is very much desired.
    I am using using precisely the behavior of this PR @ work (type 1 partially synchronous application).

  2. Fully async applications using EventLoop::run(): running the event loop on shutdown is still needed for the reasons in 1, but it becomes problematic with many independent background fibers each doing async work (and continuing to do so when the event loop is restarted, unless some custom shutdown handler also terminates them)
    MadelineProto is a type 2 application, and re-starts the event loop on shutdown only to do some cleanup, and save state, without actually running EventLoop::run() (because even if my custom shutdown handler terminates MadelineProto-related stuff, there's always the risk of user-specified async logic keeping the event loop alive): https://github.com/danog/MadelineProto/blob/v8/src/Shutdown.php#L59.

In both cases, there is a risk of working with partially corrupted state in case of exceptions, but the benefits of finishing any pending work overweigh the risk (and personally I never encountered issues with this).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants