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

["Laurențiu Leahu-Vlăducu" <l.leahu-vladucu@proxmox.com>]

Thanks for the review! I fully agree with your suggestions and sent a v3 
that contains them.

On 24.01.25 09:42, Thomas Lamprecht wrote:
> 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