From cfed1a97d79b0926ffcc60f9f73eb95632111d9b Mon Sep 17 00:00:00 2001 From: Daniil Gentili Date: Sat, 18 Nov 2023 16:59:07 +0100 Subject: [PATCH] Ignore spurious resumes on {main} suspension (#88) --- composer.json | 2 +- psalm-baseline.xml | 43 +++++++++++++++++++++ psalm.xml | 1 + src/EventLoop.php | 4 +- src/EventLoop/Driver.php | 4 +- src/EventLoop/Internal/DriverSuspension.php | 33 ++++++++++------ test/EventLoopTest.php | 36 ++++++++++++++++- 7 files changed, 105 insertions(+), 18 deletions(-) create mode 100644 psalm-baseline.xml diff --git a/composer.json b/composer.json index 18e66e7..c69c780 100644 --- 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 new file mode 100644 index 0000000..9aed453 --- /dev/null +++ b/psalm-baseline.xml @@ -0,0 +1,43 @@ + + + + + signals]]> + signals]]> + + + + + signals]]> + signals]]> + + + + + (int) ($timeout * 1_000_000) + + + + + \debug_backtrace(\DEBUG_BACKTRACE_IGNORE_ARGS) + \debug_backtrace(\DEBUG_BACKTRACE_IGNORE_ARGS) + \debug_backtrace(\DEBUG_BACKTRACE_IGNORE_ARGS) + \debug_backtrace(\DEBUG_BACKTRACE_IGNORE_ARGS) + \debug_backtrace(\DEBUG_BACKTRACE_IGNORE_ARGS) + \debug_backtrace(\DEBUG_BACKTRACE_IGNORE_ARGS) + \debug_backtrace(\DEBUG_BACKTRACE_IGNORE_ARGS) + + + + + handle, $callback->stream)]]> + handle)]]> + signalCallback, $callback->signal)]]> + + + + + stopped]]> + + + diff --git a/psalm.xml b/psalm.xml index 7c63f8b..42fe89f 100644 --- 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" > diff --git a/src/EventLoop.php b/src/EventLoop.php index 347f588..1b81136 100644 --- 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 index 84e5e8a..80464aa 100644 --- 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 index e11a21f..590421f 100644 --- 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,7 +135,7 @@ 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(); @@ -137,6 +143,11 @@ public function suspend(): mixed 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 index 4f01d13..8f334f2 100644 --- 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) {