Include panic payload string in JoinError display/debug output

[abonander]

Is your feature request related to a problem? Please describe.

When building Tokio applications, we almost always want to capture a tokio::task::JoinError and turn it into another error, e.g. eyre::Report and bubble it up. We don't generally explicitly cancel tasks, so the main possible source of errors is panics.

Because JoinError doesn't include the panic payload in its Debug or Display output, we end up with the same copy-pasted conversion functions in every single project:

pub fn map_join_error(err: tokio::task::JoinError) -> eyre::Report {
    let Ok(panic) = err.try_into_panic() else {
        return eyre::eyre!("task cancelled");
    };

    let panic_str = panic_payload_to_str(&panic);

    eyre::eyre!("task panicked: {panic_str}")
}

/// Extract a string from a panic payload.
pub fn panic_payload_to_str<'a>(panic: &'a (dyn std::any::Any + 'static)) -> &'a str {
    // Panic payloads are almost always `String` (if made with formatting arguments)
    // or `&'static str` (if given a string literal).
    //
    // Non-string payloads are a legacy feature so we don't need to worry about those much.
    panic
        .downcast_ref::<String>()
        .map(|s| &**s)
        .or_else(|| panic.downcast_ref::<&'static str>().copied())
        .unwrap_or("(non-string payload)")
}

Describe the solution you'd like

Include the logic similar to panic_payload_to_str() shown above to extract the payload string of a panic, if applicable, and include it in the Debug and Display output of JoinError.

I'd be happy to open a PR. I thought about just doing that, but I figured it'd be worth opening an issue to discuss first. I couldn't find an existing one.

Describe alternatives you've considered

• Keep copy-pasting the boilerplate like the above.
  • Could be included as an example?
• Only print panic payloads if the alternate format is requested ({:#}, {:#?})
  • Pro: doesn't print panic payloads by default if that's not desirable
  • Con: would require more than just the ? operator when converting to eyre::Report/anyhow::Error
• Include panic_payload_to_str() as a method on JoinError (open to bikeshedding)
  • Pro: doesn't print panic payloads by default if that's not desirable
  • Pro: orthogonal to the primary proposal and could also be implemented.
  • Con: must be manually called
• Upstream as something like std::panic::payload_str()
  • This logic already exists in the default panic hook, just a question of exposing it: https://github.com/rust-lang/rust/blob/e57f3090aec33cdbf66063c866afaa5e1e78b9bb/library/std/src/panicking.rs#L744-L751
  • Pro: usable for everyone, not just Tokio users
  • Con: would require an RFC and all the headache that entails
  • Con: not as convenient as the primary proposal

Additional context
N/A

[Darksonn]

I think the change is a good idea, but you will most likely need unsafe to look inside the SyncWrapper. I think the best way to go about that would be to add a method to SyncWrapper that allows downcasting the inner box to any Sync type. This is sound as we don't look inside the box if the type is not Sync, and it should be sufficient for our purposes.

[abonander]

I didn't realize that JoinError internally stored the payload as a Box<dyn Any + Send>, but your suggestion makes sense.

Something like this?

impl SyncWrapper<Box<dyn Any + Send>> {
    /// Attempt to downcast using [`Any::downcast_ref()`] to a type that is known to be `Sync`.
    pub fn downcast_ref_sync<T: Any + Sync>(&self) -> Option<&T> {
        // SAFETY: if the downcast fails, the inner value is not touched, 
        // so no thread-safety violation can occur.
        self.0.downcast_ref()
    }
}

[Darksonn]

Yes, that looks good to me.