Add finisher drop implementation to BzEncoder #121
Conversation
|
This looks roughly correct, but could you add some test cases that are fixed by this change, so we don't regress in the future? |
| impl<W: Write> Drop for BzEncoder<W> { | ||
| fn drop(&mut self) { | ||
| if self.obj.is_some() { | ||
| let _ = self.try_finish(); |
There was a problem hiding this comment.
std::io::BufWriter skips the flush on Drop if a panic happened while writing to the inner writer. It does so by having a panicked field which is set to true before writing to the inner writer and set to false immediately after writing. This avoids a double panic if the inner writer would panic on the second write too.
There was a problem hiding this comment.
Are you implying that we should be doing something similar here?
There was a problem hiding this comment.
It would be nice to do so yeah.
There was a problem hiding this comment.
I do not understand what you want me to tell here or how to apply the panicked field in this implementation.
|
I can add a test case to show why the |
|
That would be great, we'll take it from there then. |
This ensures that finish will always be attempted to be called, for example when operating as a `Box<dyn Writer>`.
…regress
Without the `impl<W: Write> Drop for BzEncoder<W>` the test fails with
the error message:
called `Result::unwrap()` on an `Err` value: Custom { kind: UnexpectedEof, error: "Input EOF reached before logical stream end" }
jonasbb
force-pushed
the
add-drop-bzencoder
branch
from
8cb2358 to
1539a77
Compare
| let uncompressed = { | ||
| let mut d = BzDecoder::new(Vec::new()); | ||
| d.write_all(&compressed).unwrap(); | ||
| d.finish().unwrap() |
There was a problem hiding this comment.
Without the Drop implementation, this line fails with:
called `Result::unwrap()` on an `Err` value: Custom { kind: UnexpectedEof, error: "Input EOF reached before logical stream end" }
|
I added a test for the behavior. It does fail as expected, when I remove the |
als use a bit less memory; it's not really needed to prove the point
This ensures that finish will always be attempted to be called, for
example when operating as a
Box<dyn Writer>. This existed in the 0.4 version of the crate and adding back the implementation fixes some test failures I observed after upgrading.Without the
Dropimplementation the generated byte sequence ends up too short and cannot be decompressed.