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

Error: event_base_loop reentrant invocation #24

Closed
Provoker opened this issue Jan 19, 2022 · 9 comments · Fixed by #26
Closed

Error: event_base_loop reentrant invocation #24

Provoker opened this issue Jan 19, 2022 · 9 comments · Fixed by #26
Labels
question Further information is requested
Milestone

Comments

@Provoker
Copy link

Hello.
The following code is causing an error:
EventBase::loop(): event_base_loop: reentrant invocation. Only one event_base_loop can run on each event_base at once.

<?php

use React\EventLoop\Loop;
use React\Promise\Deferred;
use function React\Async\await;

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

Loop::addTimer(0, function () {
	$deferred = new Deferred();
	Loop::addTimer(0, static fn() => $deferred->resolve());
	await($deferred->promise());
});

Tested on ExtEventLoop.

@WyriHaximus
Copy link
Member

Which version of this package are you using?

@Provoker
Copy link
Author

@WyriHaximus,
react/async: dev-main (ff11a7a)
react/event-loop: v1.2.0

Tested on PHP 8.1.0 and PHP 8.1.1

@WyriHaximus
Copy link
Member

Here you tried:

<?php

use React\EventLoop\Loop;
use React\Promise\Deferred;
use function React\Async\async;
use function React\Async\await;

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

Loop::addTimer(0, async(function () {
	$deferred = new Deferred();
	Loop::addTimer(0, static fn() => $deferred->resolve());
	await($deferred->promise());
}));

Or using https://github.com/reactphp/promise-timer#sleep

<?php

use React\EventLoop\Loop;
use React\Promise\Deferred;
use function React\Async\async;
use function React\Async\await;
use React\Promise\Timer\sleep;

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

Loop::addTimer(0, async(function () {
	await(sleep(0));
}));

@Provoker
Copy link
Author

Thanks, there is no problem with async().
Does this mean that any function that contains await() should be wrapped with async() ?

@Provoker
Copy link
Author

Forgotten async() call forces SimpleFiber::suspend() to call Loop::run()

self::$scheduler = new \Fiber(static fn() => Loop::run());

But if Loop::run() is already called before await() is called, then this causes various problems.

Another example

<?php

use React\EventLoop\Loop;
use React\Promise\Deferred;

use function React\Async\await;

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

Loop::futureTick(/* missed await */ function () {
	$deferred = new Deferred();
	Loop::addTimer(0, static fn() => $deferred->resolve());
	await($deferred->promise());
});

Loop::futureTick((static fn() => 0));
PHP Fatal error:  Uncaught RuntimeException: Can't shift from an empty datastructure in /vendor/react/event-loop/src/Tick/FutureTickQueue.php:46
Stack trace:
#0 /vendor/react/event-loop/src/Tick/FutureTickQueue.php(46): SplQueue->dequeue()
#1 /vendor/react/event-loop/src/ExtEventLoop.php(203): React\EventLoop\Tick\FutureTickQueue->tick()
#2 /vendor/react/event-loop/src/Loop.php(55): React\EventLoop\ExtEventLoop->run()
#3 [internal function]: React\EventLoop\Loop::React\EventLoop\{closure}()
#4 {main}
  thrown in /vendor/react/event-loop/src/Tick/FutureTickQueue.php on line 46

As a result, we do not know the problem file and line. It's very easy to miss async() in future code.

Maybe just throw an exception instead of calling Loop::run() ?

@WyriHaximus
Copy link
Member

Forgotten async() call forces SimpleFiber::suspend() to call Loop::run()

self::$scheduler = new \Fiber(static fn() => Loop::run());

But if Loop::run() is already called before await() is called, then this causes various problems.
Another example

<?php

use React\EventLoop\Loop;
use React\Promise\Deferred;

use function React\Async\await;

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

Loop::futureTick(/* missed await */ function () {
	$deferred = new Deferred();
	Loop::addTimer(0, static fn() => $deferred->resolve());
	await($deferred->promise());
});

Loop::futureTick((static fn() => 0));
PHP Fatal error:  Uncaught RuntimeException: Can't shift from an empty datastructure in /vendor/react/event-loop/src/Tick/FutureTickQueue.php:46
Stack trace:
#0 /vendor/react/event-loop/src/Tick/FutureTickQueue.php(46): SplQueue->dequeue()
#1 /vendor/react/event-loop/src/ExtEventLoop.php(203): React\EventLoop\Tick\FutureTickQueue->tick()
#2 /vendor/react/event-loop/src/Loop.php(55): React\EventLoop\ExtEventLoop->run()
#3 [internal function]: React\EventLoop\Loop::React\EventLoop\{closure}()
#4 {main}
  thrown in /vendor/react/event-loop/src/Tick/FutureTickQueue.php on line 46

As a result, we do not know the problem file and line. It's very easy to miss async() in future code.

We MIGHT always wrap loop handers in an async() call in the future. For now, you have to do this yourself, and yes that means you might miss one.

But I'll also open a PR to test this package against all event loops we support because honestly I only develop and tested it against ext-uv. Also @clue has looked into if we still perse need the future ticks and we might drop them from the implementation. I'll also PR your example as a test, if it's not already in there in a maybe slightly different way.

Maybe just throw an exception instead of calling Loop::run() ?

Because that Fiber constructor argument triggers when the main Fiber starts.

@clue clue added the question Further information is requested label Jan 25, 2022
@clue
Copy link
Member

clue commented Jan 25, 2022

Thanks, there is no problem with async(). Does this mean that any function that contains await() should be wrapped with async() ?

Excellent input! The await() function can indeed be considered "blocking", so if you're using it from an event loop timer, event listener, or promise callback, you should always wrap it in an async() function. I've just filed #26 to add documentation for this behavior.

Technically, we could also wrap this automatically for each callback, but this incurs a significant overhead. This is especially true as we would have to do this for each callback even if it does not use await() internally, because there's no way to tell from the outside.

At the moment, explicitly using async() in these cases instead seems like a reasonable compromise between ease of use and performance. This is still an early development version and I'm curious to see how the ecosystem and performance considerations evolve over time! 👍

@WyriHaximus
Copy link
Member

@Provoker Which version of ext-event are you running?

@Provoker
Copy link
Author

@WyriHaximus, Latest stable from pecl

Sockets support => enabled
Debug support => disabled
Extra functionality support including HTTP, DNS, and RPC => enabled
OpenSSL support => enabled
Thread safety support => disabled
Extension version => 3.0.6
libevent2 headers version => 2.0.21-stable

@clue clue added this to the v4.0.0 milestone Feb 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants