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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Flexible fiber bailout handling #7163

Merged
merged 1 commit into from Jun 18, 2021
Merged

Flexible fiber bailout handling #7163

merged 1 commit into from Jun 18, 2021

Conversation

kooldev
Copy link
Contributor

@kooldev kooldev commented Jun 17, 2021

This PR moves struct and functions needed to capture / restore Zend VM state into zend_fibers.c to reduce API exposure. It was never possible to gain access to that state outside of zend_fiber_switch_context() (state is stored on the C stack), hence removing this from the header makes sense.

The reason for not calling zend_bailout() immediately in zend_fiber_switch_context() is that fiber implementations may require additional cleanup actions to be performed before bailing out. If we bailout during switch context it requires every call to zend_fiber_switch_context() to be wrapped in zend_try / zend_catch to dealt with that which is not ideal. Plus there is only a single call to that function in the implementation of Fiber that does direct forwarding of bailouts.

@trowski As always: Ping 馃槈

@trowski
Copy link
Member

trowski commented Jun 17, 2021

Should flags continue to be part of zend_fiber_context? With the bailout flag removed from zend_fiber_switch_context(), none of the context functions refer to flags anymore. Should it be moved to zend_fiber instead and let other implementations handle flags however they want/need?

@kooldev
Copy link
Contributor Author

kooldev commented Jun 17, 2021

I moved flags from zend_fiber_context to zend_fiber and introduced a new fiber transfer flag ZEND_FIBER_TRANSFER_FLAG_BAILOUT. This is needed in order to communicate a bailout to other fibers, they will need to do some cleanup and bailout as well. It makes sense to use the transfer struct for that because it can be used across all fiber implementations.

@krakjoe
Copy link
Member

krakjoe commented Jun 18, 2021

Can you put the vm state changes in a separate PR please, they look unrelated.

@kooldev kooldev changed the title Reduce fiber API surface / flexible bailout handling Flexible fiber bailout handling Jun 18, 2021
@trowski trowski merged commit c5f9cde into php:master Jun 18, 2021
@kooldev kooldev deleted the fiber-refactoring branch June 18, 2021 18:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants