Skip to content

Conversation

@trowski
Copy link
Member

@trowski trowski commented Jul 22, 2023

Since fixing the hard reference retained to suspensions in the event loop, the cycle collector may collect an object holding a DriverSuspension, the DriverSuspension itself, and the suspended Fiber, queuing them for destruction. Once queued for destruction, all destructors are invoked. If the object holding the suspension queues the fiber to be resumed, the fiber destructor is run and the fiber terminated before it can be resumed.

This PR adds a check if the fiber has been destroyed while the fiber was queued to be resumed. I believe the fiber can only be destroyed while being queued for resuming after a call to gc_collect_cycles or if the cycle collector is automatically triggered.

The only alternative seems to be to retain a hard reference to the fiber elsewhere, but this would again cause any non-resumed fiber to leak because the fiber reference would remain in the list of hard references.

@trowski trowski requested a review from kelunik July 22, 2023 15:54
@kelunik
Copy link
Member

kelunik commented Jul 23, 2023

What's code that currently fails with this? Could you add a test exposing the fixed behavior?

@trowski
Copy link
Member Author

trowski commented Jul 23, 2023

I've added a test, which happened to reveal another issue because the reference to the suspended fiber was being set to null in this case because the finally block was executed when the fiber was destroyed, causing the event loop to try to use an interrupt instead of resuming a terminated fiber.

The added test fails with v1.0.2 with Revolt\EventLoop\UncaughtThrowable : Uncaught FiberError thrown in event loop callback Fiber::resume; use Revolt\EventLoop::setErrorHandler() to gracefully handle such exceptions: Cannot resume a fiber that is not suspended

Copy link
Member

@kelunik kelunik left a comment

Choose a reason for hiding this comment

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

As said in chat, neither of use like this, but it seems to be necessary.

@kelunik kelunik merged commit 4a69379 into main Jul 29, 2023
@kelunik kelunik deleted the fiber-destruct branch July 29, 2023 17:02
kafkiansky added a commit to thesis-php/nats that referenced this pull request Nov 9, 2025
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.

3 participants