StreamHandle: fix subtle bug when seeking more than 1 MB backward #22
Conversation
|
@melissalinkert with the new decision to combine the Harmony reader and the Operetta one, are there some sample datasets this PR should be tested with (i.e. using internal URL rather than filepaths) or is most of the code review at the level of this repository with the additional unit tests? |
|
From the Harmony/Operetta side, this bug was only exposed when using |
| @@ -66,6 +69,10 @@ public void setup() throws IOException { | |||
| tmpFile.deleteOnExit(); | |||
| FileOutputStream out = new FileOutputStream(tmpFile); | |||
| out.write("hello, world!\n".getBytes(Constants.ENCODING)); | |||
| byte[] emptyBuffer = new byte[EMPTY_BUFFER_SIZE]; | |||
| Arrays.fill(emptyBuffer, (byte) 0); | |||
There was a problem hiding this comment.
This is a no-op: new byte arrays are already zeroed.
|
Looking at the code and fp is being reset which means https://github.com/ome/ome-common-java/pull/22/files#diff-372db24dc54d341492aa9963f8aa7f33L160 was not behaving as expected. Setting both to pos after the reset is accomplishing the intended result. Running the new unit test without the code changes results in a failure: With the code changes the test runs and completes without error. PR looks good from my perspective. |
Originally reported as part of Harmony format support, see glencoesoftware/bioformats#4
f5e4cac updates
URLHandleTestto create a failing test exhibiting the problem. 518f31b is a port of glencoesoftware/bioformats@84fa9b6, which should result in the test passing.To test, check that building and running unit tests with f5e4cac alone results in a single test failure. Building and running unit tests with both commits together should result in all tests passing.