Fix bug in BlockCompressedInputStream.checkTermination() (#1310)

* Fix bug in BlockCompressedInputStream.checkTermination()

* 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

src/main/java/htsjdk/samtools/util/BlockCompressedInputStream.java

@@ -689,12 +689,18 @@ public static FileTermination checkTermination(SeekableByteChannel channel) thro
 
     /**
      * read as many bytes as dst's capacity into dst or throw if that's not possible
+     *
      * @throws EOFException if channel has fewer bytes available than dst's capacity
      */
     static void readFully(SeekableByteChannel channel, ByteBuffer dst) throws IOException {
-        final int bytesRead = channel.read(dst);
-        if (bytesRead < dst.capacity()){
-            throw new EOFException();
+        int totalBytesRead = 0;
+        final int capacity = dst.capacity();
+        while (totalBytesRead < capacity) {
+            final int bytesRead = channel.read(dst);
+            if (bytesRead == -1) {
+                throw new EOFException();
+            }
+            totalBytesRead += bytesRead;
         }
     }
 

src/test/java/htsjdk/samtools/SamReaderFactoryTest.java

@@ -10,6 +10,7 @@
 import htsjdk.samtools.seekablestream.SeekableFileStream;
 import htsjdk.samtools.seekablestream.SeekableHTTPStream;
 import htsjdk.samtools.seekablestream.SeekableStreamFactory;
+import htsjdk.testutil.streams.SeekableByteChannelFromBuffer;
 import htsjdk.samtools.util.*;
 import htsjdk.samtools.util.zip.InflaterFactory;
 import org.testng.Assert;

src/test/java/htsjdk/samtools/util/BlockCompressedTerminatorTest.java

@@ -26,7 +26,7 @@
 import com.google.common.jimfs.Configuration;
 import com.google.common.jimfs.Jimfs;
 import htsjdk.HtsjdkTest;
-import htsjdk.samtools.SeekableByteChannelFromBuffer;
+import htsjdk.testutil.streams.OneByteAtATimeChannel;
 import org.testng.Assert;
 import org.testng.annotations.DataProvider;
 import org.testng.annotations.Test;
@@ -120,6 +120,16 @@ public void testReadFullyThrowWhenItCantReadEnough() throws IOException {
         }
     }
 
-
-
-}
+    @Test
+    public void testReadFullyReadsBytesCorrectlyWhenPartialReadOccurs() throws IOException {
+        final byte[] expected = "something to test reading from".getBytes();
+        final ByteBuffer buffer = ByteBuffer.wrap(expected);
+        try (final SeekableByteChannel channel = new OneByteAtATimeChannel(buffer)) {
+            final int readBufferSize = 10;
+            final ByteBuffer readBuffer = ByteBuffer.allocate(readBufferSize);
+            Assert.assertTrue(channel.size() >= readBuffer.capacity());
+            BlockCompressedInputStream.readFully(channel, readBuffer);
+            Assert.assertEquals(readBuffer.array(), Arrays.copyOfRange(expected, 0, readBufferSize));
+        }
+    }
+}

src/test/java/htsjdk/testutil/streams/OneByteAtATimeChannel.java

@@ -0,0 +1,27 @@
+package htsjdk.testutil.streams;
+
+import java.io.IOException;
+import java.nio.ByteBuffer;
+
+/**
+ * A buffer backed channel that only reads 1 byte at a time on each read instance.  Used for testing that read operations
+ * work correctly when read returns less than the desired number of bytes and it isn't because of reaching EOF.
+ */
+public class OneByteAtATimeChannel extends SeekableByteChannelFromBuffer {
+    public OneByteAtATimeChannel(ByteBuffer buf) {
+        super(buf);
+    }
+
+    @Override
+    public int read(ByteBuffer dst) throws IOException {
+        final ByteBuffer buf = getBuffer();
+        if (buf.position() == buf.limit()) {
+            // signal EOF
+            return -1;
+        }
+        int before = dst.position();
+        final byte oneByte = buf.get();
+        dst.put(oneByte);
+        return dst.position() - before;
+    }
+}

src/test/java/htsjdk/samtools/SeekableByteChannelFromBuffer.java → src/test/java/htsjdk/testutil/streams/SeekableByteChannelFromBuffer.java

@@ -1,18 +1,16 @@
-package htsjdk.samtools;
+package htsjdk.testutil.streams;
 
 import java.io.IOException;
-import java.nio.Buffer;
 import java.nio.ByteBuffer;
 import java.nio.channels.ClosedChannelException;
 import java.nio.channels.SeekableByteChannel;
-import java.nio.file.StandardOpenOption;
 
 /**
  * A buffer-backed SeekableByteChannel, for testing.
  */
 public class SeekableByteChannelFromBuffer implements SeekableByteChannel {
 
-  private ByteBuffer buf;
+  private final ByteBuffer buf;
   private boolean open = true;
 
   public SeekableByteChannelFromBuffer(ByteBuffer buf) {
@@ -77,6 +75,10 @@ public void close() throws IOException {
     open = false;
   }
 
+  ByteBuffer getBuffer() {
+    return buf;
+  }
+
   private void checkOpen() throws IOException {
     if (!open) {
       throw new ClosedChannelException();