Stream: project-error-handling

Topic: panic propagation docs update


view this post on Zulip Jane Lusby (Dec 11 2020 at 23:49):

I have a new issue that might be fun for anyone looking for quick PR to rust-lang/rust. https://github.com/rust-lang/rust/issues/79950

view this post on Zulip oliver (Dec 11 2020 at 23:58):

so something like this line added to https://doc.rust-lang.org/std/panic/fn.catch_unwind.html: "This function is designed to be used in conjunction with resume_unwind to, for example, re-propagate a caught panic without use of Result as described below which should not be used for type erased panic payloads."

view this post on Zulip Jane Lusby (Dec 12 2020 at 00:05):

yea

view this post on Zulip Jane Lusby (Dec 12 2020 at 00:05):

I'd probably recommend using something like

view this post on Zulip Jane Lusby (Dec 12 2020 at 00:05):

let inner: () = result.unwrap_or_else(|payload| std::panic::resume_unwind(payload));

view this post on Zulip Jane Lusby (Dec 12 2020 at 00:05):

and put this recommendation inside of both doc pages

view this post on Zulip Jane Lusby (Dec 12 2020 at 00:06):

and maybe add an extra section to the std::thread::Result docs to warn about why unwrap is bad

view this post on Zulip Jane Lusby (Dec 12 2020 at 00:06):

and there we can point out the differences between unwrap and resume_unwind

view this post on Zulip Jane Lusby (Dec 12 2020 at 00:06):

and how one produces a new panic that has to double downcast

view this post on Zulip Jane Lusby (Dec 12 2020 at 00:06):

alternatively

view this post on Zulip Jane Lusby (Dec 12 2020 at 00:07):

or maybe additionally

view this post on Zulip Jane Lusby (Dec 12 2020 at 00:07):

we might be able to update the default panic hook in std to handle the double downcast case

view this post on Zulip Jane Lusby (Dec 12 2020 at 00:07):

but I think we will need @Mara 's panic plan changes to make that work

view this post on Zulip Jane Lusby (Dec 12 2020 at 00:08):

because rn I think unwrap() on a Result<T, Box<dyn Any>> will discard the box and convert it to a string when panicking, rather than reboxing it

view this post on Zulip Jane Lusby (Dec 12 2020 at 00:08):

so we'd need to update unwrap to somehow try to preserve the payload

view this post on Zulip Jane Lusby (Dec 12 2020 at 00:09):

the main advantage of unwrap is that it actually invokes the panic hook to re-report the panic

view this post on Zulip Jane Lusby (Dec 12 2020 at 00:09):

which will give you an error message at the end of your binary's output

view this post on Zulip Jane Lusby (Dec 12 2020 at 00:09):

rather than way back wherever it was originally caught

view this post on Zulip Jane Lusby (Dec 12 2020 at 00:09):

but the disadvantage of this is that any backtraces in that second panic would be disconnected from the original panic

view this post on Zulip Jane Lusby (Dec 12 2020 at 00:10):

so my preference would be to not recommend that approach, and to just tell ppl to scroll back to wherever the panic happened.

view this post on Zulip Jane Lusby (Dec 12 2020 at 00:10):

However I'm quite worried about someone getting in a situation where they're catching many panics and its hard to correlate which panic actually brought down the system

view this post on Zulip oliver (Dec 12 2020 at 00:12):

I think that's just an inherent problem with wrappers for things which are
normally meant to work like e-brakes

view this post on Zulip Jane Lusby (Dec 12 2020 at 00:12):

yea

view this post on Zulip Jane Lusby (Dec 12 2020 at 00:13):

but still, we're a systems programming language and people depend on it not really being an emergency break

view this post on Zulip Jane Lusby (Dec 12 2020 at 00:13):

_frustration_

view this post on Zulip Jane Lusby (Dec 12 2020 at 00:14):

part of me feels like theres a ton of issues with the design of panics in rust but I have no idea how I'd redo it in a way that satisfies the existing requirements

view this post on Zulip Jane Lusby (Dec 12 2020 at 00:14):

and even if we could redo it we cant because back compat

view this post on Zulip Jane Lusby (Dec 12 2020 at 00:14):

:/

view this post on Zulip Jane Lusby (Dec 12 2020 at 00:14):

or at least we're heavily restricted

view this post on Zulip Jane Lusby (Dec 12 2020 at 00:14):

sigh

view this post on Zulip oliver (Dec 12 2020 at 00:15):

unless break compat as a edition release, unless some magic
metadata capture comes along from another part of the lang

view this post on Zulip oliver (Dec 12 2020 at 00:19):

it would be interesting to spec a redesigned panic system
tho that's probably a lot of work and I don't know if there
is room given the existing timelines for the next edition

view this post on Zulip Jane Lusby (Dec 12 2020 at 00:19):

yea i dont think that should be a high priority

view this post on Zulip Jane Lusby (Dec 12 2020 at 00:19):

feel free to spitball ideas tho

view this post on Zulip oliver (Dec 12 2020 at 00:20):

I imagine something with better metadata capture during panic generation
but nothing technical to point to where to start

view this post on Zulip Jane Lusby (Dec 12 2020 at 00:43):

there's already some work here being done but I don't think it applies at this level

view this post on Zulip oliver (Dec 12 2020 at 00:44):

yeah it's very interesting

view this post on Zulip Jane Lusby (Dec 12 2020 at 00:44):

@Mara has been working on improving PanicInfo but that is what contains the payload

view this post on Zulip Jane Lusby (Dec 12 2020 at 00:44):

rather than the payload itself

view this post on Zulip Jane Lusby (Dec 12 2020 at 00:45):

and I imagine that if we add alternate versions of payloads we will still have to re-erase them when we pass them to catch_unwind

view this post on Zulip Jane Lusby (Dec 12 2020 at 00:45):

there might be some path to deprecation of that method where we can do a new catch_unwind that works on more rich type information rather than only being a Box<Any>

view this post on Zulip Jane Lusby (Dec 12 2020 at 00:46):

like, introduce a std::panic::Payload

view this post on Zulip oliver (Dec 12 2020 at 00:50):

draft for first doc update: https://github.com/rust-lang/rust/compare/master...o752d:master

view this post on Zulip oliver (Dec 12 2020 at 00:51):

I think the recommendation is already in the doc page of resume_unwind

view this post on Zulip oliver (Dec 12 2020 at 00:55):

I'm just not sure about adding new Payloads or improving what's
already being generated, like there might be a lot of interactions
involved in layering information like that, not certain just speculating

view this post on Zulip oliver (Dec 12 2020 at 01:00):

things like this just remind me conceptually of how degrees of freedom aggregate:
image.png


Last updated: Jan 26 2022 at 13:21 UTC