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

Add suspension listeners #2

Closed
wants to merge 1 commit into from
Closed

Add suspension listeners #2

wants to merge 1 commit into from

Conversation

trowski
Copy link
Member

@trowski trowski commented Oct 16, 2021

This PR allows user-definable hooks to be invoked when a fiber (or main) is suspended or resumed.

The motivation is to allow libraries/apps to implement behavior such as that requested in amphp/amp#354 if so desired.

This is designed to be lightweight so applications that do not use these hooks do not incur a large performance overhead.

  • Benchmarks to see what effect this has on performance.

@kelunik
Copy link
Member

kelunik commented Oct 16, 2021

This breaks with the additional suspensions we've introduced for event loop callbacks now. I think this should be solved with a core / extension API in PHP itself rather than Revolt.

@trowski
Copy link
Member Author

trowski commented Oct 16, 2021

The event loop callback fiber does not break this mechanism. The fiber switch when invoking an event loop callback is essentially considered part of the event loop fiber from the perspective of this API. Entering the callback is similar to entering a new fiber created by launch(). It is the responsibility of the user to create a new context at that point if they intend to suspend.

However, addressing it from within PHP itself may be a better solution.

As this does not introduce a BC break, we are free to introduce this much later as we explore our options.

@trowski trowski force-pushed the suspension-listener branch from 96cee1b to ffad099 Compare October 16, 2021 15:25
@trowski
Copy link
Member Author

trowski commented Oct 16, 2021

Note that I'm not entirely sure this is necessary myself. I wanted to propose a solution though and see what others thought.

kelunik added a commit that referenced this pull request Nov 22, 2021
This is equivalent to the interface change in reactphp/async#15 to allow different implementations.

This also allows decorating `Suspension` to implement listeners like proposed in #2.
kelunik added a commit that referenced this pull request Nov 22, 2021
This is equivalent to the interface change in reactphp/async#15 to allow different implementations.

This also allows decorating `Suspension` to implement listeners like proposed in #2.
kelunik added a commit that referenced this pull request Nov 22, 2021
This is equivalent to the interface change in reactphp/async#15 to allow different implementations.

This also allows decorating `Suspension` to implement listeners like proposed in #2.
@kelunik
Copy link
Member

kelunik commented Nov 22, 2021

#23 converts Suspension into an interface, so listeners can now be implemented as decorators without shipping built-in support.

@trowski
Copy link
Member Author

trowski commented Nov 23, 2021

Closing this as it can be implemented in a separate library.

@trowski trowski closed this Nov 23, 2021
@trowski trowski deleted the suspension-listener branch November 23, 2021 00:32
digitaltim-de added a commit to digitaltim-de/event-loop that referenced this pull request Jan 26, 2025
Fatal error: Uncaught FiberError: Cannot suspend in a force-closed fiber in /srv/app/vendor/revolt/event-loop/src/EventLoop/Internal/AbstractDriver.php:441 Stack trace: #0 /srv/app/vendor/revolt/event-loop/src/EventLoop/Internal/AbstractDriver.php(441): Fiber::suspend(Object(stdClass)) revoltphp#1 /srv/app/vendor/revolt/event-loop/src/EventLoop/Internal/AbstractDriver.php(567): Revolt\EventLoop\Internal\AbstractDriver->invokeMicrotasks() revoltphp#2 [internal function]: Revolt\EventLoop\Internal\AbstractDriver->Revolt\EventLoop\Internal\{closure}() revoltphp#3 {main} thrown in /srv/app/vendor/revolt/event-loop/src/EventLoop/Internal/AbstractDriver.php on line 441
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.

2 participants