[pbs-devel] [PATCH proxmox/proxmox-backup v3 0/1] Improved panic errors with formatted strings

[pbs-devel] [PATCH proxmox/proxmox-backup v3 0/1] Improved panic errors with formatted strings

[Laurențiu Leahu-Vlăducu]

While I was working on some patches, I noticed that error messages
were not properly logged in some situations where we were formatting
errors (thus not logging errors defined at compile-time).

This patch series fixes the issues I had by patching the proxmox-rest-server,
but also fixes similar issues in PBS itself from happening in the future.

Changes since v2:
* refactored nested matches to if let
* improved commit message


proxmox-backup:

Laurențiu Leahu-Vlăducu (1):
  proxy/parallel_handler: Improved panic errors with formatted strings

 src/bin/proxmox-backup-proxy.rs | 13 +++++++++----
 src/tools/parallel_handler.rs   | 12 ++++++------
 2 files changed, 15 insertions(+), 10 deletions(-)


proxmox:

Laurențiu Leahu-Vlăducu (1):
  rest-server: Improved panic errors with formatted strings

 proxmox-rest-server/src/worker_task.rs | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

-- 
2.39.5



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

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

[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

* Switched from eprintln! to tracing::error when logging panics in the
task scheduler.

Signed-off-by: Laurențiu Leahu-Vlăducu <l.leahu-vladucu@proxmox.com>
---
 src/bin/proxmox-backup-proxy.rs | 13 +++++++++----
 src/tools/parallel_handler.rs   | 12 ++++++------
 2 files changed, 15 insertions(+), 10 deletions(-)

diff --git a/src/bin/proxmox-backup-proxy.rs b/src/bin/proxmox-backup-proxy.rs
index a6d0a332..e9870532 100644
--- a/src/bin/proxmox-backup-proxy.rs
+++ b/src/bin/proxmox-backup-proxy.rs
@@ -422,11 +422,16 @@ async fn run_task_scheduler() {
         tokio::time::sleep_until(tokio::time::Instant::from_std(delay_target)).await;
 
         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"),
+            Err(panic) => {
+                if let Some(msg) = panic.downcast_ref::<&str>() {
+                    tracing::error!("task scheduler panic: {msg}");
+                } else if let Some(msg) = 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")
+                }
             },
-            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..d8ee3c72 100644
--- a/src/tools/parallel_handler.rs
+++ b/src/tools/parallel_handler.rs
@@ -135,12 +135,12 @@ impl<I: Send + 'static> ParallelHandler<I> {
         let mut i = 0;
         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)),
+                if let Some(panic_msg) = panic.downcast_ref::<&str>() {
+                    msg_list.push(format!("thread {} ({i}) panicked: {panic_msg}", self.name));
+                } else if let Some(panic_msg) = panic.downcast_ref::<String>() {
+                    msg_list.push(format!("thread {} ({i}) panicked: {panic_msg}", self.name));
+                } else {
+                    msg_list.push(format!("thread {} ({i}) panicked", self.name));
                 }
             }
             i += 1;
-- 
2.39.5



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

[pbs-devel] [PATCH proxmox v3 1/1] rest-server: Improved panic errors with formatted strings

[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

Signed-off-by: Laurențiu Leahu-Vlăducu <l.leahu-vladucu@proxmox.com>
---
 proxmox-rest-server/src/worker_task.rs | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/proxmox-rest-server/src/worker_task.rs b/proxmox-rest-server/src/worker_task.rs
index beec691e..24e2676e 100644
--- a/proxmox-rest-server/src/worker_task.rs
+++ b/proxmox-rest-server/src/worker_task.rs
@@ -980,9 +980,14 @@ impl WorkerTask {
 
                     let result = match std::panic::catch_unwind(move || f(worker1)) {
                         Ok(r) => r,
-                        Err(panic) => match panic.downcast::<&str>() {
-                            Ok(panic_msg) => Err(format_err!("worker panicked: {}", panic_msg)),
-                            Err(_) => Err(format_err!("worker panicked: unknown type.")),
+                        Err(panic) => {
+                            if let Some(panic_msg) = panic.downcast_ref::<&str>() {
+                                Err(format_err!("worker panicked: {panic_msg}"))
+                            } else if let Some(panic_msg) = panic.downcast_ref::<String>() {
+                                Err(format_err!("worker panicked: {panic_msg}"))
+                            } else {
+                                Err(format_err!("worker panicked: cannot show error message due to unknown error type."))
+                            }
                         },
                     };
 
-- 
2.39.5



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

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

[Thomas Lamprecht]

Am 24.01.25 um 16:29 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
> 
> * Switched from eprintln! to tracing::error when logging panics in the
> task scheduler.

This describes what happens but not why and which visible changes this
has, or not has, like I asked in my review for v2.

Background reasoning is important for commit messages, as while a
summary about "what" changes can be quite nice to have it can almost
always reconstructed relatively easy from checking the diff. The
semantic change that affects users and or devs OTOH is harder to
reconstruct, sometimes even impossible (besides that brains tend to
forget details, staff is not always around to be able to ask them).
And while one won't need that information for all commits, one cannot
really predict for which, so adding a rationale for every non-trivial
commit–with a rather narrow definition of trivial–is the only way to
ensure that this information is there when needed; short of time-travel
;) Note that one does not need a ton of text, one or two sentences can
often be enough, sometimes a few short paragraphs tend to help though;
I'm also fine with always going for relatively long commit messages, a
short summary at the top would be helpful then though.

Oh and FWIW, for this change it would have been fine to switch over to
tracing::error in a separate patch, as it's not directly related to
better relaying errors from panics.

The first part above I mentioned on v2, that's on you so to say, the
latter I did not mention when reviewing on v2, so that's on me, as it's
not a big change and I did not want to delay it further I now applied
the patches as is.

And the reason I went for a relatively big explanation here, which
actually applies to all devs not just you, is mostly because first,
you're relatively new at Proxmox thus a bit stricter review with more
rationale is warranted and second, doing so safes, among others, the
people writing changelogs and tracking down bugs a ton of time, and I
tend to do both quite often and know the pain about having to retrace
all relations and background reasoning from a fresh starts because the
author did not bother with any commit message, and while it was
self-explanatory or obvious back then for them, they "interestingly"
often do not remember when asking about the background in persona; which
_is_ relatable, as said making errors and forgetting things is human,
but as the fix is so easy, i.e. just write an actual commit message with
some actual background, it's nonetheless not a good excuse.  Another
benefit of writing short summarize is that one needs to reflect on the
work on a higher level, just like rubber duck programming that can help
to improve things without another's person's review or sometimes also
surface some flaws.

anyhow: applied, thanks!


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

[pbs-devel] applied: [PATCH proxmox v3 1/1] rest-server: Improved panic errors with formatted strings

[Thomas Lamprecht]

Am 24.01.25 um 16:29 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
> 
> Signed-off-by: Laurențiu Leahu-Vlăducu <l.leahu-vladucu@proxmox.com>
> ---
>  proxmox-rest-server/src/worker_task.rs | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
>

applied, thanks!


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

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

[Laurențiu Leahu-Vlăducu]

I fully agree with you on all points - thanks for the info! :)

On 27.01.25 16:57, Thomas Lamprecht wrote:
> Am 24.01.25 um 16:29 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
>>
>> * Switched from eprintln! to tracing::error when logging panics in the
>> task scheduler.
> 
> This describes what happens but not why and which visible changes this
> has, or not has, like I asked in my review for v2.
> 
> Background reasoning is important for commit messages, as while a
> summary about "what" changes can be quite nice to have it can almost
> always reconstructed relatively easy from checking the diff. The
> semantic change that affects users and or devs OTOH is harder to
> reconstruct, sometimes even impossible (besides that brains tend to
> forget details, staff is not always around to be able to ask them).
> And while one won't need that information for all commits, one cannot
> really predict for which, so adding a rationale for every non-trivial
> commit–with a rather narrow definition of trivial–is the only way to
> ensure that this information is there when needed; short of time-travel
> ;) Note that one does not need a ton of text, one or two sentences can
> often be enough, sometimes a few short paragraphs tend to help though;
> I'm also fine with always going for relatively long commit messages, a
> short summary at the top would be helpful then though.
> 
> Oh and FWIW, for this change it would have been fine to switch over to
> tracing::error in a separate patch, as it's not directly related to
> better relaying errors from panics.
> 
> The first part above I mentioned on v2, that's on you so to say, the
> latter I did not mention when reviewing on v2, so that's on me, as it's
> not a big change and I did not want to delay it further I now applied
> the patches as is.
> 
> And the reason I went for a relatively big explanation here, which
> actually applies to all devs not just you, is mostly because first,
> you're relatively new at Proxmox thus a bit stricter review with more
> rationale is warranted and second, doing so safes, among others, the
> people writing changelogs and tracking down bugs a ton of time, and I
> tend to do both quite often and know the pain about having to retrace
> all relations and background reasoning from a fresh starts because the
> author did not bother with any commit message, and while it was
> self-explanatory or obvious back then for them, they "interestingly"
> often do not remember when asking about the background in persona; which
> _is_ relatable, as said making errors and forgetting things is human,
> but as the fix is so easy, i.e. just write an actual commit message with
> some actual background, it's nonetheless not a good excuse.  Another
> benefit of writing short summarize is that one needs to reflect on the
> work on a higher level, just like rubber duck programming that can help
> to improve things without another's person's review or sometimes also
> surface some flaws.
> 
> anyhow: applied, thanks!



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