public inbox for pbs-devel@lists.proxmox.com
 help / color / mirror / Atom feed
From: "Fabian Grünbichler" <f.gruenbichler@proxmox.com>
To: Proxmox Backup Server development discussion
	<pbs-devel@lists.proxmox.com>,
	Thomas Lamprecht <t.lamprecht@proxmox.com>
Subject: Re: [pbs-devel] [PATCH proxmox 1/2] rest-server: handle failure in worker task setup correctly
Date: Mon, 02 Dec 2024 10:14:45 +0100	[thread overview]
Message-ID: <1733130490.kfm4zjj2st.astroid@yuna.none> (raw)
In-Reply-To: <01720968-bf49-4693-abdc-fd961fdd7aae@proxmox.com>

On November 29, 2024 2:34 pm, Thomas Lamprecht wrote:
> Am 29.11.24 um 14:13 schrieb Fabian Grünbichler:
>> if setting up a new worker fails after it has been inserted into the
>> WORKER_TASK_LIST, we need to clean it up instead of bubbling up the error right
>> away, else we "leak" the worker task and it never finishes..
>> 
>> Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
>> ---
>> we probably want to optimize update_active_workers as well to reduce the lock
>> contention there that triggers this issue in the first place..
>> 
>>  proxmox-rest-server/src/worker_task.rs | 7 ++++++-
>>  1 file changed, 6 insertions(+), 1 deletion(-)
>> 
>> diff --git a/proxmox-rest-server/src/worker_task.rs b/proxmox-rest-server/src/worker_task.rs
>> index 6e76c2ca..3ca93965 100644
>> --- a/proxmox-rest-server/src/worker_task.rs
>> +++ b/proxmox-rest-server/src/worker_task.rs
>> @@ -923,7 +923,12 @@ impl WorkerTask {
>>              set_worker_count(hash.len());
>>          }
>>  
>> -        setup.update_active_workers(Some(&upid))?;
>> +        let res = setup.update_active_workers(Some(&upid));
>> +        if res.is_err() {
>> +            // needed to undo the insertion into WORKER_TASK_LIST above
>> +            worker.log_result(&res);
>> +            res?
>> +        }
> 
> Seems OK from a quick look, need a bit more time for a proper review.
> 
> What the quick look can give though is style nits, i.e. IMO a bit unidiomatic for our
> code.
> 
> Would prefer one of:
> 
> Combined return path through matching
> 
> match setup.update_active_workers(Some(&upid)) {
>    Err(err) => {
>         // needed to undo the insertion into the active WORKER_TASK_LIST above
>         worker.log_result(&res);
>         Err(err)
>    }
>    Ok(_) => Ok((worker, logger))
> }
> 
> or similar than yours but avoid the outer variable:
> 
> if let Err(err) = setup.update_active_workers(Some(&upid)) {
>     // needed to undo the insertion into the active WORKER_TASK_LIST above
>     worker.log_result(&res);
>     return Err(err);
> }
> 
> IMO both fit slightly (!) better for how errors are commonly dealt with in rust and
> are thus a bit easier to understand correctly on reading.

neither of those work though, since both the log_result and the return
value need the Err(err), and err is not Clone.. maybe there is a way to
make it work, I didn't find one quickly last week and want to hand over
something to work with to Dominik ;) maybe I am missing some easy way
out though..

> 
>>  
>>          Ok((worker, logger))
>>      }
> 
> 


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

  reply	other threads:[~2024-12-02  9:15 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-11-29 13:13 [pbs-devel] [RFC proxmox 0/2] worker task setup improvements Fabian Grünbichler
2024-11-29 13:13 ` [pbs-devel] [PATCH proxmox 1/2] rest-server: handle failure in worker task setup correctly Fabian Grünbichler
2024-11-29 13:34   ` Thomas Lamprecht
2024-12-02  9:14     ` Fabian Grünbichler [this message]
2024-11-29 13:13 ` [pbs-devel] [PATCH proxmox 2/2] rest-server: close race window when updating worker task count Fabian Grünbichler
2024-11-29 13:27   ` Thomas Lamprecht
2024-11-29 14:20     ` Dominik Csapak
2024-12-02  9:14       ` Fabian Grünbichler
2024-11-29 14:53 ` [pbs-devel] [RFC proxmox 0/2] worker task setup improvements Dominik Csapak
2024-12-02 13:04 ` [pbs-devel] superseded: " Fabian Grünbichler

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1733130490.kfm4zjj2st.astroid@yuna.none \
    --to=f.gruenbichler@proxmox.com \
    --cc=pbs-devel@lists.proxmox.com \
    --cc=t.lamprecht@proxmox.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal