Ignore spurious resumes on {main} suspension (#88)

revoltphp · Nov 18, 2023 · cfed1a9 · cfed1a9
1 parent ccf9d00
commit cfed1a9
Show file tree

Hide file tree

Showing 7 changed files with 105 additions and 18 deletions.
diff --git a/composer.json b/composer.json
@@ -36,7 +36,7 @@
         "ext-json": "*",
         "phpunit/phpunit": "^9",
         "jetbrains/phpstorm-stubs": "^2019.3",
-        "psalm/phar": "^4.7"
+        "psalm/phar": "^5"
     },
     "autoload": {
         "psr-4": {

diff --git a/psalm-baseline.xml b/psalm-baseline.xml
@@ -0,0 +1,43 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<files psalm-version="5.15.0@5c774aca4746caf3d239d9c8cadb9f882ca29352">
+  <file src="src/EventLoop/Driver/EvDriver.php">
+    <UnsupportedPropertyReferenceUsage>
+      <code><![CDATA[self::$activeSignals = &$this->signals]]></code>
+      <code><![CDATA[self::$activeSignals = &$this->signals]]></code>
+    </UnsupportedPropertyReferenceUsage>
+  </file>
+  <file src="src/EventLoop/Driver/EventDriver.php">
+    <UnsupportedPropertyReferenceUsage>
+      <code><![CDATA[self::$activeSignals = &$this->signals]]></code>
+      <code><![CDATA[self::$activeSignals = &$this->signals]]></code>
+    </UnsupportedPropertyReferenceUsage>
+  </file>
+  <file src="src/EventLoop/Driver/StreamSelectDriver.php">
+    <RedundantCast>
+      <code>(int) ($timeout * 1_000_000)</code>
+    </RedundantCast>
+  </file>
+  <file src="src/EventLoop/Driver/TracingDriver.php">
+    <InvalidArgument>
+      <code>\debug_backtrace(\DEBUG_BACKTRACE_IGNORE_ARGS)</code>
+      <code>\debug_backtrace(\DEBUG_BACKTRACE_IGNORE_ARGS)</code>
+      <code>\debug_backtrace(\DEBUG_BACKTRACE_IGNORE_ARGS)</code>
+      <code>\debug_backtrace(\DEBUG_BACKTRACE_IGNORE_ARGS)</code>
+      <code>\debug_backtrace(\DEBUG_BACKTRACE_IGNORE_ARGS)</code>
+      <code>\debug_backtrace(\DEBUG_BACKTRACE_IGNORE_ARGS)</code>
+      <code>\debug_backtrace(\DEBUG_BACKTRACE_IGNORE_ARGS)</code>
+    </InvalidArgument>
+  </file>
+  <file src="src/EventLoop/Driver/UvDriver.php">
+    <UndefinedFunction>
+      <code><![CDATA[\uv_poll_init_socket($this->handle, $callback->stream)]]></code>
+      <code><![CDATA[\uv_signal_init($this->handle)]]></code>
+      <code><![CDATA[\uv_signal_start($event, $this->signalCallback, $callback->signal)]]></code>
+    </UndefinedFunction>
+  </file>
+  <file src="src/EventLoop/Internal/AbstractDriver.php">
+    <RedundantCondition>
+      <code><![CDATA[!$this->stopped]]></code>
+    </RedundantCondition>
+  </file>
+</files>
diff --git a/psalm.xml b/psalm.xml
@@ -5,6 +5,7 @@
         xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
         xmlns="https://getpsalm.org/schema/config"
         xsi:schemaLocation="https://getpsalm.org/schema/config vendor/vimeo/psalm/config.xsd"
+        errorBaseline="psalm-baseline.xml"
 >
     <projectFiles>
         <directory name="examples"/>

diff --git a/src/EventLoop.php b/src/EventLoop.php
@@ -346,7 +346,7 @@ public static function getType(string $callbackId): CallbackType
      *
      * @param string $callbackId The callback identifier.
      *
-     * @return bool {@code true} if the callback is currently enabled, otherwise {@code false}.
+     * @return bool `true` if the callback is currently enabled, otherwise `false`.
      */
     public static function isEnabled(string $callbackId): bool
     {
@@ -358,7 +358,7 @@ public static function isEnabled(string $callbackId): bool
      *
      * @param string $callbackId The callback identifier.
      *
-     * @return bool {@code true} if the callback is currently referenced, otherwise {@code false}.
+     * @return bool `true` if the callback is currently referenced, otherwise `false`.
      */
     public static function isReferenced(string $callbackId): bool
     {

diff --git a/src/EventLoop/Driver.php b/src/EventLoop/Driver.php
@@ -294,7 +294,7 @@ public function getType(string $callbackId): CallbackType;
      *
      * @param string $callbackId The callback identifier.
      *
-     * @return bool {@code true} if the callback is currently enabled, otherwise {@code false}.
+     * @return bool `true` if the callback is currently enabled, otherwise `false`.
      */
     public function isEnabled(string $callbackId): bool;
 
@@ -303,7 +303,7 @@ public function isEnabled(string $callbackId): bool;
      *
      * @param string $callbackId The callback identifier.
      *
-     * @return bool {@code true} if the callback is currently referenced, otherwise {@code false}.
+     * @return bool `true` if the callback is currently referenced, otherwise `false`.
      */
     public function isReferenced(string $callbackId): bool;
 

diff --git a/src/EventLoop/Internal/DriverSuspension.php b/src/EventLoop/Internal/DriverSuspension.php
@@ -25,6 +25,8 @@ final class DriverSuspension implements Suspension
 
     private bool $pending = false;
 
+    private bool $deadMain = false;
+
     public function __construct(
         private readonly \Closure $run,
         private readonly \Closure $queue,
@@ -38,6 +40,11 @@ public function __construct(
 
     public function resume(mixed $value = null): void
     {
+        // Ignore spurious resumes to old dead {main} suspension
+        if ($this->deadMain) {
+            return;
+        }
+
         if (!$this->pending) {
             throw $this->error ?? new \Error('Must call suspend() before calling resume()');
         }
@@ -62,6 +69,12 @@ public function resume(mixed $value = null): void
 
     public function suspend(): mixed
     {
+        // Throw exception when trying to use old dead {main} suspension
+        if ($this->deadMain) {
+            throw new \Error(
+                'Suspension cannot be suspended after an uncaught exception is thrown from the event loop',
+            );
+        }
         if ($this->pending) {
             throw new \Error('Must call resume() or throw() before calling suspend() again');
         }
@@ -101,17 +114,10 @@ public function suspend(): mixed
 
         /** @psalm-suppress RedundantCondition $this->pending should be changed when resumed. */
         if ($this->pending) {
-            $this->pending = false;
+            // This is now a dead {main} suspension.
+            $this->deadMain = true;
 
-            try {
-                $result && $result(); // Unwrap any uncaught exceptions from the event loop
-            } catch (\Throwable $throwable) {
-                $this->error = new \Error(
-                    'Suspension cannot be resumed after an uncaught exception is thrown from the event loop',
-                );
-
-                throw $throwable;
-            }
+            $result && $result(); // Unwrap any uncaught exceptions from the event loop
 
             \gc_collect_cycles(); // Collect any circular references before dumping pending suspensions.
 
@@ -129,14 +135,19 @@ public function suspend(): mixed
                 }
             }
 
-            throw $this->error = new \Error('Event loop terminated without resuming the current suspension (the cause is either a fiber deadlock, or an incorrectly unreferenced/canceled watcher):' . $info);
+            throw new \Error('Event loop terminated without resuming the current suspension (the cause is either a fiber deadlock, or an incorrectly unreferenced/canceled watcher):' . $info);
         }
 
         return $result();
     }
 
     public function throw(\Throwable $throwable): void
     {
+        // Ignore spurious resumes to old dead {main} suspension
+        if ($this->deadMain) {
+            return;
+        }
+
         if (!$this->pending) {
             throw $this->error ?? new \Error('Must call suspend() before calling throw()');
         }

diff --git a/test/EventLoopTest.php b/test/EventLoopTest.php
@@ -299,11 +299,14 @@ public function testSuspensionThrowingErrorViaInterrupt(): void
             self::assertSame($error, $t->getPrevious());
         }
 
+        $suspension->resume(); // Calling resume on the same suspension should not throw an Error.
+        $suspension->throw(new \RuntimeException()); // Calling throw on the same suspension should not throw an Error.
+
         try {
-            $suspension->resume(); // Calling resume on the same suspension should throw an Error.
+            $suspension->suspend(); // Calling suspend on the same suspension should throw an Error.
             self::fail("Error was not thrown");
         } catch (\Error $e) {
-            self::assertStringContainsString('resumed after an uncaught exception', $e->getMessage());
+            self::assertStringContainsString('suspended after an uncaught exception', $e->getMessage());
         }
 
         // Creating a new Suspension and re-entering the event loop (e.g. in a shutdown function) should work.
@@ -312,6 +315,35 @@ public function testSuspensionThrowingErrorViaInterrupt(): void
         $suspension->suspend();
     }
 
+    public function testSuspensionThrowingErrorViaInterrupt2(): void
+    {
+        $suspension = EventLoop::getSuspension();
+        $error = new \Error("Test error");
+        EventLoop::queue(static fn () => throw $error);
+        EventLoop::queue($suspension->resume(...), 123);
+        try {
+            $suspension->suspend();
+            self::fail("Error was not thrown");
+        } catch (UncaughtThrowable $t) {
+            self::assertSame($error, $t->getPrevious());
+        }
+
+        $suspension->resume(); // Calling resume on the same suspension should not throw an Error.
+        $suspension->throw(new \RuntimeException()); // Calling throw on the same suspension should not throw an Error.
+
+        try {
+            $suspension->suspend(); // Calling suspend on the same suspension should throw an Error.
+            self::fail("Error was not thrown");
+        } catch (\Error $e) {
+            self::assertStringContainsString('suspended after an uncaught exception', $e->getMessage());
+        }
+
+        // Creating a new Suspension and re-entering the event loop (e.g. in a shutdown function) should work.
+        $suspension = EventLoop::getSuspension();
+        EventLoop::queue($suspension->resume(...), 321);
+        $this->assertEquals(321, $suspension->suspend());
+    }
+
     public function testFiberDestroyedWhileSuspended(): void
     {
         $outer = new class (new class ($this) {