Check if fiber was destroyed before resuming (#84)

Do not set $suspendedFiber to null when fiber is destroyed.
revoltphp · Jul 29, 2023 · 4a69379 · 4a69379
1 parent c5a663b
commit 4a69379
Show file tree

Hide file tree

Showing 2 changed files with 63 additions and 5 deletions.
diff --git a/src/EventLoop/Internal/DriverSuspension.php b/src/EventLoop/Internal/DriverSuspension.php
@@ -1,5 +1,7 @@
 <?php
 
+/** @noinspection PhpPropertyOnlyWrittenInspection */
+
 declare(strict_types=1);
 
 namespace Revolt\EventLoop\Internal;
@@ -46,7 +48,12 @@ public function resume(mixed $value = null): void
         $fiber = $this->fiberRef?->get();
 
         if ($fiber) {
-            ($this->queue)($fiber->resume(...), $value);
+            ($this->queue)(static function () use ($fiber, $value): void {
+                // The fiber may be destroyed with suspension as part of the GC cycle collector.
+                if (!$fiber->isTerminated()) {
+                    $fiber->resume($value);
+                }
+            });
         } else {
             // Suspend event loop fiber to {main}.
             ($this->interrupt)(static fn () => $value);
@@ -72,15 +79,20 @@ public function suspend(): mixed
             $this->suspendedFiber = $fiber;
 
             try {
-                return \Fiber::suspend();
+                $value = \Fiber::suspend();
+                $this->suspendedFiber = null;
             } catch (\FiberError $exception) {
                 $this->pending = false;
+                $this->suspendedFiber = null;
                 $this->fiberError = $exception;
 
                 throw $exception;
-            } finally {
-                $this->suspendedFiber = null;
             }
+
+            // Setting $this->suspendedFiber = null in finally will set the fiber to null if a fiber is destroyed
+            // as part of a cycle collection, causing an error if the suspension is subsequently resumed.
+
+            return $value;
         }
 
         // Awaiting from {main}.
@@ -125,7 +137,12 @@ public function throw(\Throwable $throwable): void
         $fiber = $this->fiberRef?->get();
 
         if ($fiber) {
-            ($this->queue)($fiber->throw(...), $throwable);
+            ($this->queue)(static function () use ($fiber, $throwable): void {
+                // The fiber may be destroyed with suspension as part of the GC cycle collector.
+                if (!$fiber->isTerminated()) {
+                    $fiber->throw($throwable);
+                }
+            });
         } else {
             // Suspend event loop fiber to {main}.
             ($this->interrupt)(static fn () => throw $throwable);

diff --git a/test/EventLoopTest.php b/test/EventLoopTest.php
@@ -299,4 +299,45 @@ public function testSuspensionThrowingErrorViaInterrupt(): void
             self::assertSame($error, $t->getPrevious());
         }
     }
+
+    public function testFiberDestroyedWhileSuspended(): void
+    {
+        $outer = new class (new class ($this) {
+            private ?Suspension $suspension = null;
+
+            public function __construct(public object $outer)
+            {
+            }
+
+            public function suspend(): void
+            {
+                $this->suspension = EventLoop::getSuspension();
+                $this->suspension->suspend();
+            }
+
+            public function __destruct()
+            {
+                echo 'object destroyed';
+                $suspension = $this->suspension;
+                $this->suspension = null;
+                EventLoop::defer(static fn () => $suspension?->resume());
+            }
+        }) {
+            public function __construct(public object $inner)
+            {
+            }
+        };
+
+        $inner = $outer->inner;
+        unset($outer);
+
+        EventLoop::queue(static fn () => $inner->suspend());
+        unset($inner);
+
+        EventLoop::queue(\gc_collect_cycles(...));
+
+        $this->expectOutputString('object destroyed');
+
+        EventLoop::run();
+    }
 }