Avoid infinite loop writing and empty buffer (in write::BzDecoder) #97
Conversation
In `write::BzDecoder::try_finish` do not loop if nothing was written Closes trifectatechfoundation#96
bjorn3
force-pushed
the
96-infinite-loop-writing-empty-buffer
branch
from
1db557d to
cccce2f
Compare
|
Rebased to fix a merge conflict and added a test. |
| @@ -203,7 +203,8 @@ impl<W: Write> BzDecoder<W> { | |||
| /// | |||
| /// [`write`]: Self::write | |||
| pub fn try_finish(&mut self) -> io::Result<()> { | |||
| while !self.done { | |||
| // If nothing was written, there is no need to loop | |||
| while !self.done && self.total_in() > 0 { | |||
There was a problem hiding this comment.
Maybe this should actually return an error when self.total_in() == 0? An empty file is not a valid bzip2 file.
There was a problem hiding this comment.
I don't know, should we return error if we try_finish a writer without ever writing anything? I think maybe yes, we should at least pass in the empty bzip2 payload. If we didn't, we have a logic error somewhere on the caller side.
Considering that the code path was "protected" by an infinite loop, returning an error is not a breaking change 😄
That said, I completely forgot the context in which I made the PR 😞
There was a problem hiding this comment.
returning an error on an empty input sounds like the best approach to me. The doc comment should reflect that change
There was a problem hiding this comment.
hmm although doesn't that break the write_empty test below? also what happens when you write just part of a bzip file (e.g. the first 2 bytes of the header) and then flush? That behavior should be consistent.
In
write::BzDecoder::try_finishdo not loop if nothing was writtenCloses #96