Re: [pbs-devel] [PATCH v2 proxmox-backup 3/3] client: reader: signal server before client disconnect

[Christian Ebner <c.ebner@proxmox.com>]

On 12/4/24 14:49, Fabian Grünbichler wrote:
> On December 4, 2024 9:31 am, Christian Ebner wrote:
>> Signal the server that the client has successfully finished its
>> operation and is about to close the connection. This allows the server
>> side to react and clean up the connection, without returning and
>> logging an error state, as that can cause confusion [0], as this is
>> not an error but normal behaviour.
>>
>> Report in the community forum:
>> [0] https://forum.proxmox.com/threads/158306/
>>
>> Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
>> ---
>> changes since version 1:
>> - no changes
>>
>> Note:
>> I am not sure this is the best approach, as this might block the
>> thread until the server responds or it runs into a time out.
>>
>> The alternative would require completely reworking all backup reader
>> related call sides. Or maybe there is another alternative?
>>
>>   pbs-client/src/backup_reader.rs | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/pbs-client/src/backup_reader.rs b/pbs-client/src/backup_reader.rs
>> index 88cba599b..63106c999 100644
>> --- a/pbs-client/src/backup_reader.rs
>> +++ b/pbs-client/src/backup_reader.rs
>> @@ -27,6 +27,8 @@ pub struct BackupReader {
>>   
>>   impl Drop for BackupReader {
>>       fn drop(&mut self) {
>> +        // Ignore errors
>> +        let _ = proxmox_async::runtime::block_on(self.post("finish", None));
> 
> should we maybe make this explicit, like we do in the BackupWriter? I
> know that it's a bit less "obvious" here compared to writer sessions
> what constitutes success/being finished ;)
> 
> means a bit more churn now to adapt the users of BackupReader, but would
> make it possible to differentiate server side whether a reader session
> was exited normally or via an error?

Yes, that is basically what I meant in the note:
The current approach is more of a catch all. Explicitly calling the 
finish for each reader instance might be prove difficult because of all 
the Arc instances being passed around and/or the reader instantiation 
happening in a helper which then only returns the wrapped consumer of 
the reader instance, e.g. a `RemoteChunkReader` or `pxar::Accessor`.

Will have a look on how to handle this, but it will require some bigger 
refactoring.

> we could even provide some sort of message via the finish API call that
> the server could log if desired, differentiating between:
> 
> - regular finish (no error/warning)
> - finish called with a warning message (warning)
> - finish not called, reader went away (error)
> 
> ?

Not sure about this: What warnings do we even want to tell the server 
about? I think the reader instance will either work without issues or 
fail with a hard error? Or do you have some specific use-case in mind here?

> 
>>           self.abort.abort();
>>       }
>>   }
>> -- 
>> 2.39.5
>>
>>
>>
>> _______________________________________________
>> pbs-devel mailing list
>> pbs-devel@lists.proxmox.com
>> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
>>
>>
>>
> 
> 
> _______________________________________________
> pbs-devel mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
> 
> 



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