Handles the case where the binary cursor's InputStream provides fewer bytes than requested before reaching EOF. #623
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
popematt
approved these changes
Oct 24, 2023
Comment on lines
670
to
673
| if (numberOfBytesFilled < 0) { | ||
| return; | ||
| return numberOfBytesFilled; | ||
| } | ||
| limit += numberOfBytesFilled; | ||
| return numberOfBytesFilled; |
There was a problem hiding this comment.
It took me a minute to understand what this was doing. Can we rearrange this a bit?
if (numberOfBytesFilled > 0) {
limit += numberOfBytesFilled;
}
return numberOfBytesFilled;| int numberOfBytesToReturn; | ||
| if (available > 1) { | ||
| // Return fewer bytes than requested and fewer than are available, avoiding EOF. | ||
| numberOfBytesToReturn = Math.min(available -1, len - 1); |
There was a problem hiding this comment.
Suggested change
| numberOfBytesToReturn = Math.min(available -1, len - 1); | |
| numberOfBytesToReturn = Math.min(available - 1, len - 1); |
tgregg
force-pushed
the
handle-throttled-inputstream
branch
from
7117003 to
cf6ef90
Compare
rmarrowstone
approved these changes
Oct 24, 2023
| @@ -649,8 +655,9 @@ private void shiftIndicesLeft(int shiftAmount) { | |||
| * Fills the buffer with up to the requested number of additional bytes. It is the caller's responsibility to | |||
| */ | ||
| private void refill(long numberOfBytesToFill) { | ||
| private int refill(long numberOfBytesToFill) { |
There was a problem hiding this comment.
I realize this is a private method and only used in the fillAt method, for now. I would put the loop here lest we add some other method that calls this and have the same problem. Or being crystal clear in the javadoc that it is the caller's responsbility to retry, though then we'd have two callers doing looping that could be here (unless I'm missing something).
There was a problem hiding this comment.
Good point, moved the loop into refill. Also added a test to ensure skipping over a value works when the InputStream has this behavior.
tgregg
force-pushed
the
handle-throttled-inputstream
branch
from
cf6ef90 to
72c1962
Compare
… bytes than requested before reaching EOF.
tgregg
force-pushed
the
handle-throttled-inputstream
branch
from
72c1962 to
884ae15
Compare
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.
Description of changes:
InputStream implementations are allowed to return fewer bytes than requested, and doing so does not necessarily indicate that the end of the stream has been reached. GZIPInputStream has this behavior in certain cases.
Before this fix, the binary reader would throw an
IonExceptionfor unexpected EOF if this happened in the middle of a scalar value. After the fix, the reader will continue requesting bytes from the stream until EOF is definitely reached or all requested bytes have been received.The test
shouldNotFailWhenAnInputStreamProvidesFewerBytesThanRequestedWithoutReachingEofis the one that reproduced the problem, and now succeeds. I initially tried reproducing it using GZIPInputStream, starting withshouldNotFailWhenGZIPBoundaryIsEncounteredInStringValue, but gave up when it became clear that the early-return behavior occurred in a native method that I couldn't step-through debug to root cause. I left the test because it's still a good thing to verify.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.