fixed reading the data from the stream and fixed the size of chunks#10409
fixed reading the data from the stream and fixed the size of chunks#10409diemol merged 1 commit intoSeleniumHQ:trunkfrom
Conversation
diemol
left a comment
diemol
left a comment
There was a problem hiding this comment.
Thank you for this PR, @joerg1985.
I left a comment because I did not understand one change. The other one seems fine. I will ask someone else for an extra look, and then we can merge this.
| byte[] ary = new byte[CHUNK_SIZE]; | ||
| InputStream is = seResponse.getContent().get(); | ||
| int byteCount = is.read(ary); | ||
| int byteCount = ByteStreams.read(is, ary, 0, ary.length); |
There was a problem hiding this comment.
I do not understand why this is needed. I thought is.read(ary) becomes is.read(ary, 0, ary.length).
From the PR description, I understood that the initial 1MB chunck is read correctly, right?
There was a problem hiding this comment.
This is not related to the chunking, it is more about the correct usage of the InputStream.
The InputStream.read method does not guarantee to fill the complete array, even if there is more data in the stream. The InputStream.read javadoc states:
Reads some number of bytes from the input stream and stores them into the buffer array b. The number of bytes actually read is returned as an integer. This method blocks until input data is available, end of file is detected, or an exception is thrown.
An InputStream could - for whatever reason - only read some bytes from the stream and write it into the array. This would fullfill the guarantees of the javadoc. The implementation here would assume the end of stream is reached and close the stream.
On the other hand the ByteStream.read javadoc:
Reads some bytes from an input stream and stores them into the buffer array b. This method blocks until len bytes of input data have been read into the array, or end of file is detected.
There is a guarantee the array is filled to the end or the end of stream is reached. This allows detect the EOF by checking the number of bytes read, what is done in the current implementation.
Best wishes
Jörg
There was a problem hiding this comment.
PS: is.readNBytes(ary, 0, ary.length) is Java 9 and i think Selenium is at Java 8.
Codecov Report
@@ Coverage Diff @@
## trunk #10409 +/- ##
=======================================
Coverage 45.03% 45.03%
=======================================
Files 85 85
Lines 5522 5522
Branches 269 269
=======================================
Hits 2487 2487
Misses 2766 2766
Partials 269 269 Continue to review full report at Codecov.
|
InputStream.read may read less bytes than actually are in the stream, this might corrupt the forwarded data. Also fixed the size of the chunks send after the initial chunk.
|
Kudos, SonarCloud Quality Gate passed! |
…eleniumHQ#10409) InputStream.read may read less bytes than actually are in the stream, this might corrupt the forwarded data. Also fixed the size of the chunks send after the initial chunk. [skip ci]
3 participants
Description
InputStream.read may read less bytes than actually are in the stream, this might corrupt the forwarded data.
Also fixed the size of the chunks send after the initial chunk.
Motivation and Context
I noticed the http chunks after the initial 1MiB chunk are only 8KiB, e.g. a 3 MiB screenshot was chunked
in one chunk of 1MiB and 256 chunks of 8KiB, i don't think this was the intention when implementing chunks.
While fixing this i noticed the call to
is.read(ary)did expect the function to block until the array is full or the end of steam is reached, likeis.readNBytes(ary, 0, ary.length)would do. Sinceis.readNBytesis Java 9 i had to fallback tocom.google.common.io.ByteStreamsalready used in other areas of the code.Types of changes
Checklist