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

Discard {main} suspension on uncaught exception from loop #71

Merged
merged 12 commits into from
Nov 19, 2023
2 changes: 1 addition & 1 deletion composer.json
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@
"ext-json": "*",
"phpunit/phpunit": "^9",
"jetbrains/phpstorm-stubs": "^2019.3",
"psalm/phar": "^4.7"
"psalm/phar": "^5"
},
"autoload": {
"psr-4": {
Expand Down
43 changes: 43 additions & 0 deletions psalm-baseline.xml
Original file line number Diff line number Diff line change
@@ -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>
1 change: 1 addition & 0 deletions psalm.xml
Original file line number Diff line number Diff line change
Expand Up @@ -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"/>
Expand Down
4 changes: 2 additions & 2 deletions src/EventLoop.php
Original file line number Diff line number Diff line change
Expand Up @@ -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
{
Expand All @@ -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
{
Expand Down
4 changes: 2 additions & 2 deletions src/EventLoop/Driver.php
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand All @@ -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;

Expand Down
10 changes: 5 additions & 5 deletions src/EventLoop/Internal/AbstractDriver.php
Original file line number Diff line number Diff line change
Expand Up @@ -395,6 +395,7 @@ final protected function error(\Closure $closure, \Throwable $exception): void
$this->interrupt = static fn () => $exception instanceof UncaughtThrowable
? throw $exception
: throw UncaughtThrowable::throwingCallback($closure, $exception);
unset($this->suspensions[$this]); // Remove suspension for {main}
return;
}

Expand Down Expand Up @@ -624,11 +625,10 @@ private function createErrorCallback(): void
try {
$errorHandler($exception);
} catch (\Throwable $exception) {
$this->setInterrupt(
static fn () => $exception instanceof UncaughtThrowable
? throw $exception
: throw UncaughtThrowable::throwingErrorHandler($errorHandler, $exception)
);
$this->interrupt = static fn () => $exception instanceof UncaughtThrowable
? throw $exception
: throw UncaughtThrowable::throwingErrorHandler($errorHandler, $exception);
unset($this->suspensions[$this]); // Remove suspension for {main}
}
};
}
Expand Down
35 changes: 28 additions & 7 deletions src/EventLoop/Internal/DriverSuspension.php
Original file line number Diff line number Diff line change
Expand Up @@ -21,10 +21,12 @@ final class DriverSuspension implements Suspension
/** @var \WeakReference<\Fiber>|null */
private readonly ?\WeakReference $fiberRef;

private ?\FiberError $fiberError = null;
private ?\Error $error = null;

private bool $pending = false;

private bool $deadMain = false;

public function __construct(
private readonly \Closure $run,
private readonly \Closure $queue,
Expand All @@ -38,8 +40,13 @@ 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->fiberError ?? new \Error('Must call suspend() before calling resume()');
throw $this->error ?? new \Error('Must call suspend() before calling resume()');
}

$this->pending = false;
Expand All @@ -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');
}
Expand All @@ -73,6 +86,7 @@ public function suspend(): mixed
}

$this->pending = true;
$this->error = null;

// Awaiting from within a fiber.
if ($fiber) {
Expand All @@ -81,12 +95,12 @@ public function suspend(): mixed
try {
$value = \Fiber::suspend();
$this->suspendedFiber = null;
} catch (\FiberError $exception) {
} catch (\FiberError $error) {
$this->pending = false;
$this->suspendedFiber = null;
$this->fiberError = $exception;
$this->error = $error;

throw $exception;
throw $error;
}

// Setting $this->suspendedFiber = null in finally will set the fiber to null if a fiber is destroyed
Expand All @@ -100,7 +114,9 @@ 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;

$result && $result(); // Unwrap any uncaught exceptions from the event loop

\gc_collect_cycles(); // Collect any circular references before dumping pending suspensions.
Expand All @@ -127,8 +143,13 @@ 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->fiberError ?? new \Error('Must call suspend() before calling throw()');
throw $this->error ?? new \Error('Must call suspend() before calling throw()');
}

$this->pending = false;
Expand Down
44 changes: 44 additions & 0 deletions test/EventLoopTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -298,6 +298,50 @@ public function testSuspensionThrowingErrorViaInterrupt(): void
} 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(...));
$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.
kelunik marked this conversation as resolved.
Show resolved Hide resolved
$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
Expand Down
Loading