-
Notifications
You must be signed in to change notification settings - Fork 845
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
#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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the purpose of this initialization?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you write the same test without a loop?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you make a test that uses just two read calls?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will the test still be valid if ReaderInputStream.DEFAULT_CHAR_BUFFER_SIZE
is changed for some reason?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. Although ideally the value would be read from ReaderInputStream
instead of being hardcoded there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
)