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

Conversation

trowski
Copy link
Member

@trowski trowski commented Jan 27, 2023

If an exception is thrown from the event loop, the suspension for {main} will throw "Must call suspend() before calling resume()" because the suspension has been marked as no longer pending before throwing the exception from the loop.

$this->pending = false;
$result && $result(); // Unwrap any uncaught exceptions from the event loop

This PR retains that suspension in the instance of DriverSuspension and rethrows the exception from any call to resume() or throw() in the same way as we were already throwing a FiberError.

This new behavior only affects shutdown after an uncaught exception from the event loop and {main} has not been suspended again, which prior to this change was throwing an Error, but now will throw UncaughtThrowable.

@trowski trowski requested a review from kelunik January 27, 2023 22:24
@Bilge
Copy link

Bilge commented Jan 28, 2023

This new behavior only effects

ahem. Hate to be that guy, but... affects.

Actually, I love to be that guy.

@kelunik
Copy link
Member

kelunik commented Jan 30, 2023

This new behavior only affects shutdown after an uncaught exception from the event loop and {main} has not been suspended again, which prior to this change was throwing an Error, but now will throw UncaughtThrowable.

No, this affects everything throwing exceptions and using suspensions, because the reference is now retained and cannot be garbage collected, which is even more problematic with exceptions keeping a reference to arguments in the stack trace.

@trowski
Copy link
Member Author

trowski commented Jan 30, 2023

Not everything throwing exceptions, only those thrown from the event loop. The change is only on the path that would have thrown an Error anyway. Normal throwing of exceptions is unaffected.

@trowski trowski changed the title Rethrow uncaught loop exception from {main} suspension Discard {main} suspension on uncaught exception from loop Feb 12, 2023
@trowski
Copy link
Member Author

trowski commented Feb 12, 2023

@kelunik Could you have another look at this. Instead of retaining the uncaught exception from the event loop, another Error instances is created and thrown instead. The cached suspension for {main} is discarded when throwing from the event loop, allowing the event loop to be re-entered from a shutdown function, while keeping the prior suspension invalid in case it is resumed.

# Conflicts:
#	src/EventLoop/Internal/DriverSuspension.php
@trowski trowski merged commit ad46c94 into main Nov 19, 2023
4 checks passed
@trowski trowski deleted the rethrow-from-suspension branch November 19, 2023 14:06
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.

4 participants