Skip to content

Conversation

joviegas
Copy link
Contributor

@joviegas joviegas commented Oct 7, 2025

Motivation and Context

Fix for #6459

Per RFC 9110 Section 10.1.1, clients MUST NOT generate a 100-continue expectation in requests that do not include content. Currently, the SDK adds the Expect: 100-continue header to all S3 PutObject and UploadPart requests, including those with zero-length content.

This causes issues with sync HTTP clients (e.g., Apache HttpClient) that reuse connections. When a zero-length request is sent with the Expect header, the server may close the connection after receiving the empty content, but the client attempts to reuse it, leading to connection errors.

RFC 9110 Reference:

A client MUST NOT generate a 100-continue expectation in a request that does not include content.
A server MAY omit sending a 100 (Continue) response if it has already received some or all of the content for the corresponding request, or if the framing indicates that there is no content.

Modifications

Updated StreamingRequestInterceptor to skip adding the Expect: 100-continue header when:

  1. Request is a PutObject or UploadPart operation, AND
  2. Request body content length is explicitly 0

Key Changes:

  • Modified shouldAddExpectContinueHeader() method to check for zero-length content

Behavior:

Scenario Content Length Expect Header Added?
No request body N/A Yes (unknown length)
Body present, length unknown Optional.empty() Yes
Body present, length = 0 0L No (RFC compliance)
Body present, length > 0 > 0L Yes

Testing

Added test coverage in StreamingRequestInterceptorTest:

Screenshots (if appropriate)

N/A - Code-only change

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)

License

  • I confirm that this pull request can be released under the Apache 2 license

Notes

Apache 5.x

https://github.com/arturobernalg/httpcomponents-client/blob/237b5f85f743c94d9be2d26d396130bc2053f2ef/httpclient5/src/main/java/org/apache/hc/client5/http/protocol/RequestExpectContinue.java#L85-L88

Apache 4.5.x

https://github.com/apache/httpcomponents-client/blob/54900db4653d7f207477e6ee40135b88e9bcf832/httpclient/src/main/java/org/apache/http/client/protocol/RequestExpectContinue.java#L73-L75

@joviegas joviegas requested a review from a team as a code owner October 7, 2025 19:11
@joviegas joviegas force-pushed the joviegas/Expect_100_continue branch from bad553d to 32853df Compare October 7, 2025 19:19
Comment on lines 59 to 62
return context.requestBody()
.flatMap(RequestBody::optionalContentLength)
.map(length -> length != 0L)
.orElse(true);
Copy link
Contributor

Choose a reason for hiding this comment

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

What about async request body? The length could also be sourced from the request itself, i.e, putObjectRequest.contentLength(). Should we check header instead? It could be content-length or x-amz-decoded-content-length

Copy link
Contributor

Choose a reason for hiding this comment

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

+1, we should probably give priority to x-amz-decoded-content-length and just fall back to content-length. If the decoded length is present and 0, then it's just the trailer and I don't think we need to use expect: 100-continue for that.

Copy link
Contributor Author

@joviegas joviegas Oct 7, 2025

Choose a reason for hiding this comment

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

Thanks @zoewangg @dagnir Initially I was only planning to support for Sync client since Apache client was erring out while Async client open new connection in these cases
Will update code based on above recommendations.

@joviegas joviegas force-pushed the joviegas/Expect_100_continue branch from 75beb91 to 04e9ec5 Compare October 8, 2025 20:22
Copy link

sonarqubecloud bot commented Oct 8, 2025

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants