Fix bug in BlockCompressedInputStream.checkTermination() #1310
Conversation
* Fixing a bug in BlockCompressedInputStream.checkTermination() The internal method readFully would throw in cases where less bytes were read than were available in the stream. We haven't observed this in practice but it's a valid behavior of SeekableByteChannel.read() so we should honor it. * Fixes #1251
Codecov Report
@@ Coverage Diff @@
## master #1310 +/- ##
===============================================
+ Coverage 67.728% 68.644% +0.917%
- Complexity 8225 8666 +441
===============================================
Files 560 562 +2
Lines 33493 35311 +1818
Branches 5637 6156 +519
===============================================
+ Hits 22684 24239 +1555
- Misses 8632 8828 +196
- Partials 2177 2244 +67
|
| final int bytesRead = channel.read(dst); | ||
| if (bytesRead < dst.capacity()){ | ||
| throw new EOFException(); | ||
| int totalBytesRead = 0; |
There was a problem hiding this comment.
Please reformat using IDE to fix whitespace, in all three files.
| try(final SeekableByteChannel channel = new OneByteAtATimeChannel(buffer)){ | ||
| final int readBufferSize = 10; | ||
| final ByteBuffer readBuffer = ByteBuffer.allocate(readBufferSize); | ||
| // Assert.assertTrue(channel.size() >= readBuffer.capacity()); |
There was a problem hiding this comment.
Delete or uncomment commented code
| @@ -77,6 +77,10 @@ public void close() throws IOException { | |||
| open = false; | |||
| } | |||
|
|
|||
| protected ByteBuffer getBuffer(){ | |||
There was a problem hiding this comment.
Seems a shame to expose this for testing purposes only. If the subclass type was defined in the same package, you could use package (default) scope instead, which is more restrictive than protected, and doesn't lead to API compatibility requirements later on.
So you could use default scope here if you defined OneByteAtATimeChannel in the same package as SeekableByteChannelFromBuffer. You could put the source in the test directory so it wouldn't be part of the public API.
There was a problem hiding this comment.
This class is already in the test directories so it isn't part of the public API. I don't mind moving them both to a subpackage though and restricting it to default scope.
There was a problem hiding this comment.
moved both to htsjdk.testutil.streams
The internal method readFully would throw in cases where less bytes were read than were available in the stream.
We haven't observed this in practice but it's a valid behavior of SeekableByteChannel.read() so we should honor it.
Description
Please explain the changes you made here.
Explain the motivation for making this change. What existing problem does the pull request solve?
Checklist