Re: [pbs-devel] [PATCH v4 proxmox-backup 4/5] client: reader: add finish method to signal client state to server

["Max Carrara" <m.carrara@proxmox.com>]

On Tue Apr 8, 2025 at 2:58 PM CEST, Christian Ebner wrote:
> Signal the server that the client has finished its operation and is
> about to close the connection. This allows the server side to react
> accordingly.
>
> Termination of the reader connection after successuful completion is
> now no longer logged as connection error, which has caused confusion
> [0].
>
> Report in the community forum:
> [0] https://forum.proxmox.com/threads/158306/
>
> Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
> ---
> changes since version 3:
> - no changes
>
>  pbs-client/src/backup_reader.rs | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/pbs-client/src/backup_reader.rs b/pbs-client/src/backup_reader.rs
> index 18442ebca..3474c8ce3 100644
> --- a/pbs-client/src/backup_reader.rs
> +++ b/pbs-client/src/backup_reader.rs
> @@ -77,6 +77,12 @@ impl BackupReader {
>          Ok(BackupReader::new(h2, abort, crypt_config))
>      }
>  
> +    /// Terminate reader session by signaling server via `finish` api call before closing connection
> +    pub async fn finish(self: Arc<Self>) -> Result<(), Error> {
> +        let _value = self.post("finish", None).await?;
> +        Ok(())
> +    }

There are two concerns I have with this approach here:

  1. While I like moving out of `self` here (I actually love it when
     state is represented via the type system) calling `post` here like
     this might cause a race: `self: Arc<Self>` might still be
     referenced somewhere else, as in, there might still be some other
     operations going on.

  2. Calling `finish()` is not enforced. In patch 05 you're calling
     `finish()` in 9 locations in total if I counted correctly, which
     means that there are 9 locations where haphazard changes could
     introduce subtle bugs.

What I'd instead suggest is enforcing the call to happen through the
type system -- here's a *very* rough example:

    with_new_reader(..., |reader: &BackupReader| {
        // Do stuff in here ...
    
        // Return a result upon successful completion, which then signals
        // to with_new_reader() that finish() should be called
        Ok(...)
    })
    
    fn with_new_reader<F>(..., func: F) -> Result<(), Error>
    where
        F: FnOnce(BackupReader) -> Result<(), Error> {
    
        // [...] set up reader, then call func() on it
        let reader = ...
    
        match func(&reader) {
    	Ok(()) => reader.finish().await,
    	Err(...) => ...,
        }
    }

The idea behind this is that the closure enforces the scope in which the
reader is used for operations. Once the closure ends, `finish()` is
called depending on the result the closure returns. Instead of just
returning `()`, you could also add some kind of enum representing the
possible "exiting" values / states of the reader, in case there's more
stuff to handle (now or in the future).

The thing is though... implementing this would require a rather large
set of changes throughout our code, because we currently pass around
`Arc<BackupReader>` quite a lot (*sigh*), which really gets in the way
when one wants to enforce a certain order of operations (i.e. preventing
`finish()` from being called too early).

Since all of the methods of `BackupReader` take `&self` you could check
if you can get away with s/Arc<BackupReader>/&BackupReader/.

Let me know what you think!

> +
>      /// Execute a GET request
>      pub async fn get(&self, path: &str, param: Option<Value>) -> Result<Value, Error> {
>          self.h2.get(path, param).await



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