-
Notifications
You must be signed in to change notification settings - Fork 39
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
Add panic propagation #37
Conversation
Hmm... It is a bit annoying that it works differently with and without the feature flags. I think it would be possible to use the builder to allow users to explicitly opt-in to this behavior, but given that the current behavior is somewhat odd, I'm not clear that that is a good idea. |
Good idea, just to make sure no one is surprised by the new behavior. I've rebased on |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
After smol-rs/async-task#37 I meant to add this to the executor. This commit makes it so all panics are surfaced in the tasks that the user calls. Hopefully this improves ergonomics. Signed-off-by: John Nunley <dev@notgull.net>
After smol-rs/async-task#37 I meant to add this to the executor. This commit makes it so all panics are surfaced in the tasks that the user calls. Hopefully this improves ergonomics. Signed-off-by: John Nunley <dev@notgull.net> Signed-off-by: Alain Zscheile <fogti+devel@ytrizja.de>
Resolves #36 by making it so, when the
std
feature is enabled, panics are caught and then propagated when theTask
is polled. This generally makes more sense than whatever thread polling theRunnable
panicking.I had to adjust some tests which seemed to assume that the
Runnable
panics rather than theTask
, I'm not sure whether this is an intended feature I'm overwriting or not.