Re: [pbs-devel] [PATCH proxmox-backup v2 1/1] proxy/parallel_handler: Improved panic errors with formatted strings

[Thomas Lamprecht <t.lamprecht@proxmox.com>]

Thanks, looks OK from functionallity POV, some–mostly style–comments inline.

Am 21.01.25 um 14:11 schrieb Laurențiu Leahu-Vlăducu:
> Improved errors when panics occur and the panic message is a
> formatted (not static) string. This worked already for &str literals,
> but not for Strings.
> 
> Downcasting to both &str and String is also done by the Rust Standard
> Library in the default panic handler. See:
> https://github.com/rust-lang/rust/blob/b605c65b6eb5fa71783f8e26df69975f9f1680ee/library/std/src/panicking.rs#L777

A hint that you switch from eprintln! to tracing::error would be good to
have in the commit message, ideally with the semantic changes, or the lack
thereof, recorded here too.

> 
> Signed-off-by: Laurențiu Leahu-Vlăducu <l.leahu-vladucu@proxmox.com>
> ---
> 
> Changes since v1:
> * use tracing::error instead of eprintln
> * inline some variables when formatting
> 
>  src/bin/proxmox-backup-proxy.rs |  9 ++++++---
>  src/tools/parallel_handler.rs   | 10 +++++-----
>  2 files changed, 11 insertions(+), 8 deletions(-)
> 
> diff --git a/src/bin/proxmox-backup-proxy.rs b/src/bin/proxmox-backup-proxy.rs
> index ce1be1c0..21602fdc 100644
> --- a/src/bin/proxmox-backup-proxy.rs
> +++ b/src/bin/proxmox-backup-proxy.rs
> @@ -423,10 +423,13 @@ async fn run_task_scheduler() {
>  
>          match schedule_tasks().catch_unwind().await {
>              Err(panic) => match panic.downcast::<&str>() {
> -                Ok(msg) => eprintln!("task scheduler panic: {msg}"),
> -                Err(_) => eprintln!("task scheduler panic - unknown type"),
> +                Ok(msg) => tracing::error!("task scheduler panic: {msg}"),
> +                Err(panic) => match panic.downcast::<String>() {
> +                    Ok(msg) => tracing::error!("task scheduler panic: {msg}"),
> +                    Err(_) => tracing::error!("task scheduler panic - cannot show error message due to unknown error type")


Can be ok that way, but maybe using an if/else chain like the linked example
makes this slightly easier to follow when reading? Nested matches that are
basically an else branch look IMO often more complex than what they are,
certainly somewhat subjective though.

Err(panic) => {
    if let Some(msg) = panic.downcast_ref::<&str>() {
        tracing::error!("task scheduler panic: {msg}");
    } else if let Some(s) = panic.downcast_ref::<String>() {
        tracing::error!("task scheduler panic: {msg}");
    } else {
        tracing::error!("task scheduler panic - cannot show error message due to unknown error type")
    }
}

If we have even more occurrences than the three you fix in this series, it might
be worth adding a helper fn for this.

> +                }
>              },
> -            Ok(Err(err)) => eprintln!("task scheduler failed - {err:?}"),
> +            Ok(Err(err)) => tracing::error!("task scheduler failed - {err:?}"),
>              Ok(Ok(_)) => {}
>          }
>      }
> diff --git a/src/tools/parallel_handler.rs b/src/tools/parallel_handler.rs
> index 17f70179..afa0bc97 100644
> --- a/src/tools/parallel_handler.rs
> +++ b/src/tools/parallel_handler.rs
> @@ -136,11 +136,11 @@ impl<I: Send + 'static> ParallelHandler<I> {
>          while let Some(handle) = self.handles.pop() {
>              if let Err(panic) = handle.join() {
>                  match panic.downcast::<&str>() {
> -                    Ok(panic_msg) => msg_list.push(format!(
> -                        "thread {} ({}) panicked: {}",
> -                        self.name, i, panic_msg
> -                    )),
> -                    Err(_) => msg_list.push(format!("thread {} ({}) panicked", self.name, i)),
> +                    Ok(panic_msg) => msg_list.push(format!("thread {} ({i}) panicked: {panic_msg}", self.name)),
> +                    Err(panic) => match panic.downcast::<String>() {
> +                        Ok(panic_msg) => msg_list.push(format!("thread {} ({i}) panicked: {panic_msg}", self.name)),
> +                        Err(_) => msg_list.push(format!("thread {} ({i}) panicked", self.name))
> +                    },
>                  }
>              }
>              i += 1;



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel