Skip to content

HDDS-15794. StreamBlockInputStream.onNext may throw IndexOutOfBoundsException on short payloads#10698

Open
smengcl wants to merge 3 commits into
apache:masterfrom
smengcl:HDDS-15521-npe
Open

HDDS-15794. StreamBlockInputStream.onNext may throw IndexOutOfBoundsException on short payloads#10698
smengcl wants to merge 3 commits into
apache:masterfrom
smengcl:HDDS-15521-npe

Conversation

@smengcl

@smengcl smengcl commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

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):

  1. It builds a hex preview with data.substring(0, 10), which throws IndexOutOfBoundsException when the payload is shorter than 10 bytes. That secondary exception replaces the real failure.
  2. It dereferences getResponse() (r.getRequestObserver().onError(e)) before the real failure is recorded, and getResponse() can be null if the failure happens before the streaming read response is registered, producing an NPE that again masks the original cause.
  3. setFailed(e) is called after the logging/observer work, so any exception thrown above prevents the real error from ever being recorded.

Changes

  • Call setFailed(e) first so the real failure is always recorded before any log or observer call can throw.
  • Build the log preview with the existing StringUtils.bytes2Hex(ByteBuffer, int) overload, which clamps to 10 bytes and appends ... on overflow, instead of the unguarded data.substring(0, 10).
  • Null-guard getResponse() before invoking onError.

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?

  • Added new unit tests in TestStreamBlockInputStream
    • testChecksumFailureOnShortPayloadSurfacesRealError: a checksum failure on a sub-10-byte payload surfaces the real OzoneChecksumException rather than IndexOutOfBoundsException.
    • 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.

…tion and masked failure in onNext error handling
@smengcl smengcl requested a review from Copilot July 9, 2026 09:51
@smengcl smengcl added the bug Something isn't working label Jul 9, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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-null StreamingReadResponse.
  • 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.

@adoroszlai

Copy link
Copy Markdown
Contributor

I think this partly duplicates #10431 (HDDS-15480).

@smengcl

smengcl commented Jul 9, 2026

Copy link
Copy Markdown
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
@smengcl smengcl changed the title HDDS-15794. StreamBlockInputStream can NPE on premature stream completion and masked failure in onNext error handling HDDS-15794. StreamBlockInputStream.onNext masks the real failure and can throw IndexOutOfBoundsException on short payloads Jul 9, 2026
@smengcl smengcl marked this pull request as ready for review July 10, 2026 01:53
@adoroszlai adoroszlai changed the title HDDS-15794. StreamBlockInputStream.onNext masks the real failure and can throw IndexOutOfBoundsException on short payloads HDDS-15794. StreamBlockInputStream.onNext may throw IndexOutOfBoundsException on short payloads Jul 10, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants