Storage - SeekableByteChannel Unnecessary Request Bugfix#49526
Storage - SeekableByteChannel Unnecessary Request Bugfix#49526ibrandes wants to merge 9 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes an inefficiency and related error handling in BlobClientBase.openSeekableByteChannelRead by updating the underlying StorageSeekableByteChannelBlobReadBehavior to avoid an unnecessary trailing HTTP 416 call after the full blob has already been read, and to treat certain transport-level failures at EOF as end-of-file instead of surfacing exceptions to the caller.
Changes:
- Short-circuit reads at/after the known blob length to EOF when the read is protected by an
If-MatchETag (avoids an unnecessary 416 round trip). - Swallow and warn on transport-level
RuntimeExceptionfailures when reading at/after the known EOF, returning-1instead. - Add unit tests for the new behaviors and document the fixes in the Storage Blob changelog.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| sdk/storage/azure-storage-blob/src/main/java/com/azure/storage/blob/specialized/StorageSeekableByteChannelBlobReadBehavior.java | Adds EOF short-circuit for ETag-locked reads and runtime-exception handling when reading at/after known EOF. |
| sdk/storage/azure-storage-blob/src/test/java/com/azure/storage/blob/specialized/StorageSeekableByteChannelBlobReadBehaviorTests.java | Adds coverage validating no-call EOF behavior when ETag-locked, and EOF signaling behavior on transport failures at EOF. |
| sdk/storage/azure-storage-blob/CHANGELOG.md | Documents the unnecessary 416 request fix and the transport-error-at-EOF behavior change. |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
…randes/azure-sdk-for-java into storage/byteChannelReadBugfix
browndav-msft
left a comment
There was a problem hiding this comment.
I think the PR looks good. I just added something things that might help clarity and perhaps adding a test for blobShrink, if that's a thing.
| } | ||
|
|
||
| String ifMatch = requestConditions.getIfMatch(); | ||
| return !CoreUtils.isNullOrEmpty(ifMatch) && !"*".equals(ifMatch.trim()); |
There was a problem hiding this comment.
nit: this is just hard to read because of the multiple negations. Is it possible to simplify this so that it is just easier to understand e.g. isIfMatchPopulated(ifMatch) && isIfMatchNotAWildcard(ifMatch)
idk, it just takes way more time to read it with all of the double negations.
There was a problem hiding this comment.
Or maybe something like this might be easier to understand:
String ifMatch = requestConditions.getIfMatch();
if (CoreUtils.isNullOrEmpty(ifMatch)) {
return false;
}
// A specific ETag locks the blob's content; the wildcard "*" matches any blob and does not.
String trimmedIfMatch = ifMatch.trim();
return !trimmedIfMatch.isEmpty() && !trimmedIfMatch.equals("*");| */ | ||
| @Test | ||
| public void readPastEndShortCircuitsWhenETagLocked() throws IOException { | ||
| BlobClientBase client = Mockito.mock(BlobClientBase.class); |
There was a problem hiding this comment.
nit: I think we could move this to the @BeforeEach setup() and create a mockClient instance variable
| * must continue to surface as exceptions, since some bytes the caller asked for could not be retrieved. | ||
| */ | ||
| @Test | ||
| public void readWithinResourcePropagatesTransportError() { |
There was a problem hiding this comment.
This might be a clearer name: readBeforeEndOfBlobPropagatesTransportError
| } | ||
|
|
||
| @Test | ||
| void readDetectsBlobGrowth() throws IOException { |
There was a problem hiding this comment.
should there be a readDetectsBlobShrink(). I don't even know if this is possible.
Fix unnecessary HTTP 416 round trip in
openSeekableByteChannelReadAddresses #38070
Summary
This PR fixes two related issues in
StorageSeekableByteChannelBlobReadBehavior(used byBlobClientBase.openSeekableByteChannelRead) that caused an unnecessary HTTP request after a blob's entire content had already been delivered to the caller.Unnecessary HTTP 416 round trip: When a seekable byte channel is opened with ETag consistency control (the default), the read behavior previously issued an additional
downloadStreamWithResponsecall even after the full blob had been returned in the initial range download. Because the requested offset was at or past the end of the blob, the service rejected this request with an HTTP 416 (Requested Range Not Satisfiable) response. The read now short-circuits to end-of-file (-1) once the known resource length is reached, avoiding the extra call entirely.Transport error while streaming the trailing 416 response: Even when the extra request was issued, streaming the body of the 416 response could fail with a transport-level error (for example, "Connection reset by peer" wrapped in reactor's
ReactiveException). Since the requested offset was already at or past the known end of the resource, no data could have been returned anyway. The read now logs a warning and signals end-of-file instead of propagating the exception to a caller that has already received all of the blob's content.Reason for the changes
Callers using
openSeekableByteChannelReadwere seeing an unnecessary HTTP 416 response—and in some cases an unexpected exception (e.g., connection reset)—after they had already successfully read the entire blob. Made evident by stress testing, this caused confusing errors and wasted a network round trip on what should be a clean end-of-file signal.Changes
StorageSeekableByteChannelBlobReadBehavior.javasourceOffset >= resourceLengthand the blob content is locked via anIf-MatchETag (the content cannot have grown without invalidating the precondition), via a newisEtagLocked()helper.RuntimeExceptioncatch block that, when the requested offset is at or past the known end of the resource, logs a warning and returns-1instead of propagating the error.CHANGELOG.mdBugs Fixedentries describing the fixes.Behavior preserved