task: add panic string to debug/display output of JoinError

[abonander]

Fixes: #6749

cc @Darksonn

Unresolved questions:

• Would it be useful to expose panic_payload_as_str() as an inherent method on JoinError?
• Do we want to support the alternate-formatting flag ({:#}, {:#?}) to hide the panic message?
• If the panic payload is not String or &'static str, should anything else be printed (e.g. "<non-string payload>")?
• Does this behavior need to be documented?
• Does this need a test? The purpose of the existing relevant tests is not clear:
  • rt_panic.rs and task_panic.rs seem primarily concerned with correct reporting of the panic location.
  • task_abort.rs appears to be primarily regression tests.

Motivation

(From #6749)

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)")
}

Solution

(From #6749)

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.

The logic is identical to that in the default panic hook in std: https://github.com/rust-lang/rust/blob/e57f3090aec33cdbf66063c866afaa5e1e78b9bb/library/std/src/panicking.rs#L744-L751

[Darksonn]

Would it be useful to expose panic_payload_as_str() as an inherent method on JoinError?

For now, let's not do that.

Do we want to support the alternate-formatting flag ({:#}, {:#?}) to hide the panic message?

I don't think that's necessary.

If the panic payload is not String or &'static str, should anything else be printed (e.g. "<non-string payload>")?

We can mirror std and print Box<dyn Any>.

Does this behavior need to be documented?

Nah, it's fine as is.

Does this need a test? The purpose of the existing relevant tests is not clear:

I would like to see a single test. I'm curious how the {:?} gets formatted, and I'm wondering if we want to print the &str with Display instead of Debug.

[abonander]

I would like to see a single test.

Can you tell me where it should go?

[Darksonn]

We don't have a perfect file for it, but you can use task_abort.rs.

[abonander]

@Darksonn while I'm at it, what do you think of making the Debug impl use debug_struct() so we can label the fields with useful names?

[abonander]

Added tests: https://github.com/abonander/tokio/blob/join-error-panic-str/tokio/tests/task_abort.rs#L224-L302

[abonander]

As it turns out, we can't just assert the full string output of these impls because the task ID can change...

[abonander]

We can mirror std and print Box<dyn Any>.

That would work fine for the Debug output, but what about Display?

[mox692]

Looks good to me.

[abonander]

I still had a couple questions but if ya'll are happy with it, that works.

[Darksonn]

Yeah, this is fine. Thanks!