Make stream seek error message informative #927

Merged
merged 1 commit into from Jul 5, 2017
Jump to file or symbol
Failed to load files and symbols.
+30 −7
Split
@@ -57,10 +57,12 @@
public final static String INCORRECT_HEADER_SIZE_MSG = "Incorrect header size for file: ";
public final static String UNEXPECTED_BLOCK_LENGTH_MSG = "Unexpected compressed block length: ";
public final static String PREMATURE_END_MSG = "Premature end of file: ";
- public final static String CANNOT_SEEK_STREAM_MSG = "Cannot seek on stream based file ";
@magicDGS

magicDGS Jul 4, 2017

Contributor

This is a non-backwards compatible change... is it really needed?

@ronlevine

ronlevine Jul 4, 2017

Contributor

It's a clearer name but you're right, it's breaks backward compatibility and is not really need. I will revert the name.

@yfarjoun

yfarjoun Jul 5, 2017

Contributor

you didn't revert the name change.

@ronlevine

ronlevine Jul 5, 2017

Contributor

I did, it's on the line below. This is the new one. I can reverse the order to make the diff easier to view.

+ public final static String CANNOT_SEEK_STREAM_MSG = "Cannot seek a position for a non-file stream";
+ public final static String CANNOT_SEEK_CLOSED_STREAM_MSG = "Cannot seek a position for a closed stream";
public final static String INVALID_FILE_PTR_MSG = "Invalid file pointer: ";
private InputStream mStream = null;
+ private boolean mIsClosed = false;
private SeekableStream mFile = null;
private byte[] mFileBuffer = null;
private DecompressedBlock mCurrentBlock = null;
@@ -222,6 +224,9 @@ public void close() throws IOException {
// Encourage garbage collection
mFileBuffer = null;
mCurrentBlock = null;
+
+ // Mark as closed
+ mIsClosed = true;
}
/**
@@ -344,12 +349,20 @@ public int read(final byte[] buffer, int offset, int length) throws IOException
* Seek to the given position in the file. Note that pos is a special virtual file pointer,
* not an actual byte offset.
*
- * @param pos virtual file pointer
+ * @param pos virtual file pointer position
+ * @throws IOException if stream is closed or not a file based stream
*/
public void seek(final long pos) throws IOException {
+ // Must be before the mFile == null check because mFile == null for closed files and streams
+ if (mIsClosed) {
+ throw new IOException(CANNOT_SEEK_CLOSED_STREAM_MSG);
@magicDGS

magicDGS Jul 4, 2017

Contributor

Maybe a simple test could be added for this codepath...

@ronlevine

ronlevine Jul 4, 2017

Contributor

Done.

+ }
+
+ // Cannot seek on streams that are not file based
if (mFile == null) {
throw new IOException(CANNOT_SEEK_STREAM_MSG);
}
+
// Decode virtual file pointer
// Upper 48 bits is the byte offset into the compressed stream of a
// block.
@@ -81,6 +81,7 @@ public void testBasic() throws Exception {
Assert.assertEquals(bcis2.read(buffer), available, "Should read to end of block");
Assert.assertTrue(bcis2.endOfBlock(), "Should be at end of block");
bcis2.close();
+ Assert.assertEquals(bcis2.read(buffer), -1, "Should be end of file");
}
@DataProvider(name = "seekReadExceptionsData")
@@ -89,24 +90,32 @@ public void testBasic() throws Exception {
return new Object[][]{
{HTSJDK_TRIBBLE_RESOURCES + "vcfexample.vcf.truncated.gz", FileTruncatedException.class,
BlockCompressedInputStream.PREMATURE_END_MSG + System.getProperty("user.dir") + "/" +
- HTSJDK_TRIBBLE_RESOURCES + "vcfexample.vcf.truncated.gz", true, false, 0},
+ HTSJDK_TRIBBLE_RESOURCES + "vcfexample.vcf.truncated.gz", true, false, false, 0},
{HTSJDK_TRIBBLE_RESOURCES + "vcfexample.vcf.truncated.hdr.gz", IOException.class,
BlockCompressedInputStream.INCORRECT_HEADER_SIZE_MSG + System.getProperty("user.dir") + "/" +
- HTSJDK_TRIBBLE_RESOURCES + "vcfexample.vcf.truncated.hdr.gz", true, false, 0},
+ HTSJDK_TRIBBLE_RESOURCES + "vcfexample.vcf.truncated.hdr.gz", true, false, false, 0},
{HTSJDK_TRIBBLE_RESOURCES + "vcfexample.vcf.gz", IOException.class,
- BlockCompressedInputStream.CANNOT_SEEK_STREAM_MSG, false, true, 0},
+ BlockCompressedInputStream.CANNOT_SEEK_STREAM_MSG, false, true, false, 0},
+ {HTSJDK_TRIBBLE_RESOURCES + "vcfexample.vcf.gz", IOException.class,
+ BlockCompressedInputStream.CANNOT_SEEK_CLOSED_STREAM_MSG, false, true, true, 0},
{HTSJDK_TRIBBLE_RESOURCES + "vcfexample.vcf.gz", IOException.class,
BlockCompressedInputStream.INVALID_FILE_PTR_MSG + 1000 + " for " + System.getProperty("user.dir") + "/" +
- HTSJDK_TRIBBLE_RESOURCES + "vcfexample.vcf.gz", true, true, 1000 }
+ HTSJDK_TRIBBLE_RESOURCES + "vcfexample.vcf.gz", true, true, false, 1000 }
};
}
@Test(dataProvider = "seekReadExceptionsData")
- public void testSeekReadExceptions(final String filePath, final Class c, final String msg, final boolean isFile, final boolean isSeek, final int pos) throws Exception {
+ public void testSeekReadExceptions(final String filePath, final Class c, final String msg, final boolean isFile, final boolean isSeek, final boolean isClosed,
+ final int pos) throws Exception {
final BlockCompressedInputStream bcis = isFile ?
new BlockCompressedInputStream(new File(filePath)) :
new BlockCompressedInputStream(new FileInputStream(filePath));
+
+ if ( isClosed ) {
+ bcis.close();
+ }
+
boolean haveException = false;
try {
if ( isSeek ) {
@@ -212,5 +221,6 @@ public Deflater makeDeflater(final int compressionLevel, final boolean gzipCompa
}
bcis.close();
Assert.assertEquals(deflateCalls[0], 3, "deflate calls");
+ Assert.assertEquals(reader.readLine(), null);
}
}