HDDS-15794. StreamBlockInputStream.onNext masks the real failure and can throw IndexOutOfBoundsException on short payloads#10698
Draft
smengcl wants to merge 3 commits into
Draft
HDDS-15794. StreamBlockInputStream.onNext masks the real failure and can throw IndexOutOfBoundsException on short payloads#10698smengcl wants to merge 3 commits into
smengcl wants to merge 3 commits into
Conversation
…tion and masked failure in onNext error handling
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes edge cases in the HDDS client streaming read path where premature gRPC stream completion and failures inside onNext() could previously surface as NullPointerException / IndexOutOfBoundsException, masking the real failure and bypassing intended error propagation.
Changes:
- Treat “stream completed + response queue drained” as a clean “no more data” signal in the streaming reader to avoid NPEs during reads.
- Harden
onNext()failure handling to (a) record the real failure first, (b) safely build a short payload preview, and (c) avoid dereferencing a possibly-nullStreamingReadResponse. - Add unit tests covering premature completion mid-read and checksum-failure paths (including short payloads and failure-before-response-registration timing).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| hadoop-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/storage/StreamBlockInputStream.java | Prevents NPE on drained queue after stream completion and makes onNext() error handling robust (no masking exceptions, null-safe observer signaling). |
| hadoop-hdds/client/src/test/java/org/apache/hadoop/hdds/scm/storage/TestStreamBlockInputStream.java | Adds targeted unit tests to reproduce and validate the fixed premature-completion and checksum-failure scenarios. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
|
I think this partly duplicates #10431 (HDDS-15480). |
Contributor
Author
|
Thanks @adoroszlai . I will update this once #10431 is merged. |
Conflicts: hadoop-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/storage/StreamBlockInputStream.java
This file contains hidden or 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
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.
Generated-by: Claude Code (Fable 5)
What changes were proposed in this pull request?
After HDDS-15480 (#10431) landed, the premature-stream-completion / null-poll() NPE is fixed. One independent bug remains in
StreamBlockInputStream.StreamingReader.onNext(), in the catch block that handles a failure while processing a response (for example a checksum mismatch):data.substring(0, 10), which throwsIndexOutOfBoundsExceptionwhen the payload is shorter than 10 bytes. That secondary exception replaces the real failure.getResponse()(r.getRequestObserver().onError(e)) before the real failure is recorded, andgetResponse()can be null if the failure happens before the streaming read response is registered, producing an NPE that again masks the original cause.setFailed(e)is called after the logging/observer work, so any exception thrown above prevents the real error from ever being recorded.Changes
setFailed(e)first so the real failure is always recorded before any log or observer call can throw.StringUtils.bytes2Hex(ByteBuffer, int)overload, which clamps to 10 bytes and appends...on overflow, instead of the unguardeddata.substring(0, 10).getResponse()before invokingonError.This PR originally also fixed the premature-completion / null-poll() NPE, which overlapped with #10431. That overlap is now merged on master via HDDS-15480, so this PR is rebased down to just the
onNext()error-handling fix.What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-15794
How was this patch tested?
TestStreamBlockInputStreamtestChecksumFailureOnShortPayloadSurfacesRealError: a checksum failure on a sub-10-byte payload surfaces the realOzoneChecksumExceptionrather thanIndexOutOfBoundsException.testChecksumFailureBeforeResponseRegistered: a failure before the response is registered no longer NPEs on a null response.testStreamCompletedMidReadWithEmptyQueueSurfacesEof: read surfaces EOF (not NPE) when the stream completes mid-read with a drained queue.