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

Fix assertion failure in generator dtor #16025

Merged
merged 1 commit into from
Oct 2, 2024
Merged

Conversation

arnaud-lb
Copy link
Member

Fixes GH-15866. Related: GH-15158, GH-10462.

I don't remember why I assumed that a generator with the ZEND_GENERATOR_IN_FIBER flag (implying the generator is running) could only be dtor during shutdown, but this assumption was wrong. It can happen during GC, of course.

Unfortunately it means that we can not cache the result of check_node_running_in_fiber() anymore, so we may traverse the tree more than once. However we only ever call this function when collecting a Fiber, which I expect to be rare.

This is already the 3rd follow up of GH-10462, so I considered this idea again, but since we can start fibers in destructors now, this is not trivial.

@arnaud-lb
Copy link
Member Author

I assume the ABI break is because of the removal of ZEND_GENERATOR_DTOR_VISITED, but this symbol is not in any release yet.

@cmb69
Copy link
Member

cmb69 commented Sep 24, 2024

I assume the ABI break is because of the removal of ZEND_GENERATOR_DTOR_VISITED, but this symbol is not in any release yet.

The labeler is dumb. If there is no ABI break, just remove the label (if ZEND_GENERATOR_DTOR_VISITED was already in PHP-8.3, the label would be a nice reminder to document the removal in UPGRADING.INTERNALS).

@cmb69 cmb69 removed the ABI break label Sep 24, 2024
@arnaud-lb arnaud-lb marked this pull request as ready for review September 24, 2024 17:56
@arnaud-lb arnaud-lb requested a review from bwoebi September 24, 2024 17:56
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.

I'm approving it because the changes are definitely right, just without caching.
Caching would be nice, but I suppose we can defer that until it actually becomes a problem?! (As far as I'm aware it hasn't been one.)

@arnaud-lb arnaud-lb merged commit 6e55f4d into php:PHP-8.2 Oct 2, 2024
8 checks passed
arnaud-lb added a commit that referenced this pull request Oct 2, 2024
arnaud-lb added a commit that referenced this pull request Oct 2, 2024
* PHP-8.2:
  [ci skip] NEWS for GH-16025
  Fix assertion failure in generator dtor (#16025)
arnaud-lb added a commit that referenced this pull request Oct 2, 2024
arnaud-lb added a commit that referenced this pull request Oct 2, 2024
* PHP-8.3:
  [ci skip] NEWS for GH-16025
  [ci skip] NEWS for GH-16025
  Fix assertion failure in generator dtor (#16025)
arnaud-lb added a commit that referenced this pull request Oct 2, 2024
arnaud-lb added a commit that referenced this pull request Oct 2, 2024
* PHP-8.4:
  [ci skip] NEWS for GH-16025
  [ci skip] NEWS for GH-16025
  [ci skip] NEWS for GH-16025
  Fix assertion failure in generator dtor (#16025)
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.

3 participants