Repository navigation
GH-55428: [C++][S3] Preserve retryable errors when ranged-read buffers overflow - #55437
Closed
JonSnow1807 wants to merge 1 commit into
Closed
JonSnow1807 wants to merge 1 commit into
JonSnow1807 wants to merge 1 commit into
Conversation
|
|
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.
Closes #55428.
Rationale for this change
A ranged GetObject response currently writes into a buffer sized to the requested read. An S3 error document can be larger than that buffer, even when the requested object is small.
When this happens, the response stream fails before the AWS SDK can classify the error. A SlowDown response becomes INTERNAL_FAILURE with “Failed to flush response stream”, preventing the expected retry behavior.
What changes are included in this PR?
Introduce a GET-specific response stream that writes successful reads directly into the caller’s buffer.
If a response exceeds that buffer, the stream retains a prefix of up to 64 KiB for SDK error parsing and consumes subsequent bytes without growing the retained buffer. This lets the SDK classify and retry throttling responses.
After a successful SDK result, check whether the response overflowed. Oversized successful responses return an IOError instead of being accepted as a valid ranged read.
The upload stream is unchanged.
Are these changes tested?
Six new C++ tests cover normal and short writes, bulk and split overflow, character writes, direct put-area writes, rewinding, spill-cap boundaries, and overflow detection.
Eleven Python integration cases use a local HTTP server to cover:
The two small-range regression cases fail against the original source library, while the fitting control passes. All eleven integration cases pass with the patch.
The complete C++ S3 suite passes with 81 tests passed and two skipped. Validation used Linux, the curl HTTP backend, and AWS SDK C++ 1.11.778. Actual Windows and CRT HTTP backends were not run; direct put-area behavior is covered by the C++ tests.
Are there any user-facing changes?
Small ranged reads can now retain the SDK’s throttling classification and retry behavior when the error body exceeds the requested range.
Oversized successful responses produce an explicit IOError. There are no public API changes.
Error documents larger than 64 KiB may lose XML details and rely on HTTP-status classification. The retained buffer is bounded, but consuming the response still takes network time.
Was AI used for this PR?
AI tools assisted with implementation, regression tests, independent review, and validation. I reviewed the code, tests, and description before submission.
PR code and description written by:
Reviewed before submission by: