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

Prevent dtor of generator in suspended fiber #10462

Merged
merged 3 commits into from Jan 27, 2023
Merged

Conversation

arnaud-lb
Copy link
Member

@arnaud-lb arnaud-lb commented Jan 27, 2023

Fixes #9916

The shutdown sequence calls the destructor of any live generator or fiber (or any object).

When a fiber is suspended in a generator, the fiber destructor will resume the execution the generator, which may be already be destroyed. This leads to the issues described in #9916.

Because of this, we must not attempt to destroy a generator that was executing in a suspended fiber.

The fiber destructor conveniently throws a zend_ce_graceful_exit in the fiber context, which will also properly terminate the generator (and execute its finally blocks). I believe that this has the same semantics as the generator destructor.

Generators that suspended a fiber should not be dtor because they will be
executed during the fiber dtor.

Fiber dtor throws an exception in the fiber's context in order to unwind and
execute finally blocks, which will also properly dtor the generator.
bwoebi
bwoebi previously approved these changes Jan 27, 2023
Copy link
Member

@bwoebi bwoebi left a comment

Choose a reason for hiding this comment

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

This looks great to me.

This turned out to be much simpler than expected, but as you say, we can just conveniently rely on the uncatchable exception thrown by the fiber :-)

The bugfix should target PHP 8.1 though.

@arnaud-lb arnaud-lb changed the base branch from master to PHP-8.1 January 27, 2023 16:20
@arnaud-lb arnaud-lb dismissed bwoebi’s stale review January 27, 2023 16:20

The base branch was changed.

Copy link
Member

@trowski trowski left a comment

Choose a reason for hiding this comment

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

I too am pleasantly surprised at the simplicity of this patch. I'll be very happy to get this in as I've been getting reports of CI failures using AMPHP v3 due to this issue.

@arnaud-lb arnaud-lb marked this pull request as ready for review January 27, 2023 18:31
@arnaud-lb arnaud-lb merged commit 1173c2e into php:PHP-8.1 Jan 27, 2023
arnaud-lb added a commit that referenced this pull request Jan 27, 2023
* PHP-8.1:
  [ci skip] NEWS
  Fix overflow check in OnUpdateMemoryConsumption (#10456)
  Prevent dtor of generator in suspended fiber (#10462)
arnaud-lb added a commit that referenced this pull request Jan 27, 2023
* PHP-8.2:
  [ci skip] NEWS
  [ci skip] NEWS
  Fix overflow check in OnUpdateMemoryConsumption (#10456)
  Prevent dtor of generator in suspended fiber (#10462)
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.

Entering shutdown sequence with a fiber suspended in a Generator emits an unavoidable fatal error or crashes
3 participants