finish can be problematic

[kali]

Hello,

I ran into a problem while using Encoder in a context where it has to be switchable for other Write implémentation : the finish method has to be called to send the last bytes to the inner writer. Other Write implementation for similar purpose (like flate2 or snappy framed) will just flush the last page on drop().

Working around it is tricky: as finish() consumes self, you can not make a trivial wrapper implementing Drop around the Encoder because drop only has a &mut self...

And, you can not just add drop() to the implementation alongside finish(), because finish() returns the inner Write. I just think we can't have both, unless we have two variants of Encoder.

I see the point of being able to get the inner Write back, but I'm wondering if this feature is preferable to compatibility with other encoder (and safety, as calling drop() can not be forgotten).

What do you think ?

[bozaro]

I ran into a problem while using Encoder in a context where it has to be switchable for other Write implémentation : the finish method has to be called to send the last bytes to the inner writer. Other Write implementation for similar purpose (like flate2 or snappy framed) will just flush the last page on drop().

Writer finish() method add end-of-compressed-stream mark and flush last page.
I think make this logic on drop() is not good idea, because in interrupted sender method we can get a correctlly closed stream (because drop() will called anyway). So I need some mark of successfully completed work.
Also drop() method can't return error.

Working around it is tricky: as finish() consumes self, you can not make a trivial wrapper implementing Drop around the Encoder because drop only has a &mut self...

You can workaround this by code like:

struct Wrapper<W: Write> {
    s: Option<Encoder<W>>,
}

impl<W: Write> Write for Wrapper<W> {
    fn write(&mut self, buffer: &[u8]) -> Result<usize> {
        self.s.as_mut().unwrap().write(buffer)
    }

    fn flush(&mut self) -> Result<()> {
        self.s.as_mut().unwrap().flush()
    }
}

impl<W: Write> Drop for Wrapper<W> {
    fn drop(&mut self) {
        match self.s.take() {
            Some(s) => {s.finish();}
            None => {}
        }
    }
}

I see the point of being able to get the inner Write back, but I'm wondering if this feature is preferable to compatibility with other encoder (and safety, as calling drop() can not be forgotten).

I make this feature because I can. I don't know how this problem solved in another encoders.

[kali]

Thanks. This Wrapper would work, indeed. I have had a look at other implementations (std::io::BufWriter, flate2, snappy_framed, which are the ones I need to switch). The four of them are handling thing differently:

• snappy http://docs.randomhacks.net/snappy_framed-rs/snappy_framed/write/struct.SnappyFramedEncoder.html expects the user to call flush(), and do not allow inner writer retrieval
• flate2 offers finish(), but manage the inner ownership internally with an option
• stdlib BufWriter goes even a bit further by handling a possible panic during the finish()

Do we need a WriterWrapper trait in stdlib ? cc @alexcrichton

[bozaro]

I create rust-lang/rust#32625

[bozaro]

I try with RFC: rust-lang/rfcs#1568

[DrRibosome]

Just hit this issue. Problem for me happens when I try to use this with other libraries, like slog, which take input writers as self and simply drop to close.

Seems a bit strange that the finish method is a self, but it only calls write_end which is &mut self. Maybe you could make the write_end public instead?

[TomKellyGenetics]

Running this workaround on Rust version 0.40. throws an error (possibly due to updated error checking).

warning: unused `std::result::Result` in tuple element 1 that must be used
   --> chunk_reads/src/main.rs:255:27
    |
255 |               Some(s) => {s.finish();}
    |                           ^^^^^^^^^^^
    |
    = note: `#[warn(unused_must_use)]` on by default
    = note: this `Result` may be an `Err` variant, which should be handled

Packages calling this will not compile unless this is added above the "struct". As a workaround, this will allow it to compile with a warning, although error handling would be ideal.

#[allow(unused_must_use)]