#800: fix last block of stream being ignored if size < 8k #802
Conversation
| @@ -142,7 +142,7 @@ public int read(byte b[], int off, int len) throws IOException { | |||
| totalRead += remaining; | |||
| off += remaining; | |||
| len -= remaining; | |||
| if (len == 0) { | |||
| if (len >= 0) { | |||
There was a problem hiding this comment.
This condition would effectively exit from the loop at the first iteration, so it will hurt performance.
| // System.out.println(String.format("Read: %4d Start: %3d End: %3d", read, buffer[0], buffer[read - 1])); | ||
| total += read; | ||
| if (read == BLOCK) { | ||
| assertEquals("Byte values are not correct", buffer[0], buffer[read % BLOCK]); |
There was a problem hiding this comment.
Please refrain from asserting multiple assorted things in a single testcase.
It is hard to understand what this test actually examines
Codecov Report
@@ Coverage Diff @@
## master #802 +/- ##
===========================================
+ Coverage 65.26% 65.3% +0.04%
- Complexity 3517 3525 +8
===========================================
Files 165 166 +1
Lines 15217 15230 +13
Branches 2465 2468 +3
===========================================
+ Hits 9931 9946 +15
+ Misses 4099 4097 -2
Partials 1187 1187 |
|
|
||
| // initialize with values (0, 1, ... 127, 0, 1, ...) | ||
| for (int i = 0; i < data.length; i++) { | ||
| data[i] = (byte) (i & 0x7F); |
There was a problem hiding this comment.
What is the purpose of this initialization?
There was a problem hiding this comment.
It was used to check if the read blocks were contiguous (ie last block ended with value 33, next one should start with 34), but is not needed anymore (it only makes sense with custom charBufferSize in ReaderInputStream). Will remove it.
| int read; | ||
| int total = 0; | ||
|
|
||
| while ((read = r.read(buffer, 0, BLOCK)) > -1) { |
There was a problem hiding this comment.
Can you write the same test without a loop?
There was a problem hiding this comment.
I can replace it with N calls to read (in this case 6). I don't think that using a single (bigger) read would fail the test when the problem occurs.
There was a problem hiding this comment.
Can you make a test that uses just two read calls?
| final byte[] buffer = new byte[BLOCK]; | ||
|
|
||
| InputStreamReader isr = new InputStreamReader(new ByteArrayInputStream(data)); | ||
| ReaderInputStream r = new ReaderInputStream(isr); |
There was a problem hiding this comment.
Will the test still be valid if ReaderInputStream.DEFAULT_CHAR_BUFFER_SIZE is changed for some reason?
There was a problem hiding this comment.
Yes. Although ideally the value would be read from ReaderInputStream instead of being hardcoded there.
There was a problem hiding this comment.
Do you think it makes sense to make ReaderInputStream.DEFAULT_CHAR_BUFFER_SIZE public (or having a getter) and make the test using that?
There was a problem hiding this comment.
DEFAULT_CHAR_BUFFER_SIZE should be private, and it should not be accessed/visible from external point of view.
Fixes #800 (data truncated at the end of the stream, while using
setCharacterStream)