close
Skip to content

Prevent dtor of generator in suspended fiber - #10462

Merged
arnaud-lb merged 3 commits into
php:PHP-8.1from
arnaud-lb:gh9916
Jan 27, 2023
Merged

Prevent dtor of generator in suspended fiber#10462
arnaud-lb merged 3 commits into
php:PHP-8.1from
arnaud-lb:gh9916

Conversation

@arnaud-lb

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

Copy link
Copy Markdown
Member

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

@bwoebi bwoebi left a comment

Copy link
Copy Markdown
Member

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.

@trowski trowski left a comment

Copy link
Copy Markdown
Member

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