Buffer.[set|write]Bytes doesn't respect length when reading from InputStream
#2389
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…putStream Motivation: Javadoc for `Buffer.writeBytes` and `Buffer.setBytes` says it reads "length bytes have been read, the end of stream is reached, or an exception is thrown". However, its internal implementation in Netty does only a single `InputStream.read(byte[], int, int)` invocation. `InputStream` contract does not guarantees to read the whole requested `length`, is says "reads up to len bytes of data from the input stream". There are implementations that may return less than `len` bytes on a single read attempt. Modifications: - `NettyBuffer` keeps reading from the `InputStream` until length bytes have been read; - Add tests to demonstrate a problem; - Also add tests for `writeBytesUntilEndStream` bcz it's a custom method that does not exist in Netty; Result: `Buffer.writeBytes` and `Buffer.setBytes` behave according to their javadoc when reading from `InputStream`.
idelpivnitskiy
added a commit
to idelpivnitskiy/servicetalk
that referenced
this pull request
Oct 13, 2022
Motivation: apple#2389 introduced a bug for `Buffer.[set|write]Bytes` methods that returns `0` instead of `-1` after EOF. Modifications: - Make sure we preserve `-1` signal if nothing else was read from the `InputStream`; Result: `Buffer.[set|write]Bytes` correctly return `-1` after EOF.
idelpivnitskiy
added a commit
that referenced
this pull request
Oct 13, 2022
Motivation: #2389 introduced a bug for `Buffer.[set|write]Bytes` methods that returns `0` instead of `-1` after EOF. Modifications: - Make sure we preserve `-1` signal if nothing else was read from the `InputStream`; Result: `Buffer.[set|write]Bytes` correctly return `-1` after EOF.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation:
Javadoc for
Buffer.writeBytesandBuffer.setBytessays it reads"length bytes have been read, the end of stream is reached, or an
exception is thrown". However, its internal implementation in Netty
does only a single
InputStream.read(byte[], int, int)invocation.InputStreamcontract does not guarantee to read the whole requestedlength, it says "reads up to len bytes of data from the input stream".There are implementations that may return less than
lenbytes on asingle read attempt.
Modifications:
NettyBufferkeeps reading from theInputStreamuntil length byteshave been read;
writeBytesUntilEndStreambcz it's a custommethod that does not exist in Netty;
Result:
Buffer.writeBytesandBuffer.setBytesbehave according to theirjavadoc when reading from
InputStream.