[pbs-devel] [PATCH pxar] encoder: flush after writing last entry

[Wolfgang Bumiller]

> On 03/29/2021 6:25 PM Dietmar Maurer <dietmar at proxmox.com> wrote:
> 
>  
> > > +        flush(self.output.as_mut()).await?;
> > 
> > According to the patch comment this hasn't broken anywhere at the time,
> > but was there any test-code that did need this?
> > 
> > I'd like to make this at least conditional on the writer being
> > `EncoderOutput::Owned` to not cause additional flushes for every single
> > level of directory nesting.
> 
> Oh, I was not aware that this calls flush for every directory. I guess
> nobody really wants that.
> 
> > That said, I'm not even convinced an `Owned` writer would really need
> > this? You don't need to explicitly call `flush()` on a `std::fs::File`
> > or even a `std::io::BufWriter` explicitly (`BufWriter` explicitly
> > flushes in its `Drop` handler), unless you *explicitly* want to handle
> > its error, but then you should keep ownership of the writer you pass to
> > the encoder anyway and flush manually, not leave that up to the pxar
> > code.
> 
> I am ok with reverting this patch.

After an off-list talk with Dominik we concluded that keeping it for `Owned`
writers is the safer approach for the simple reason that in *async* code (eg.
tokio's async BufWriter equivalent) you *do* need to flush buffered writers.

This is probably because there's no `AsyncDrop` and there's no guarantee that
the future's `Drop` handler is called in a place where it is safe to call
tokio's `block_in_place` (after all it panics when it is outside a tokio RT
thread, *including* being inside a `runtime.block_on()` called from outside
a tokio RT thread).

So yeah, let's not revert this, but limit it to `EncoderOutput::Owned`.