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

[Christian Ebner]

On 4/9/25 15:53, Max Carrara wrote:
> 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 at 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!

Thanks for your suggestions. Given that this will however require more 
in-depth changes and has a larger regression potential this will be 
postponed to after the next point release (as discussed of list with 
Thomas).