[Storage] Add Expect: 100-continue support - #50094
Open
gunjansingh-msft wants to merge 5 commits into
Open
Conversation
Applies the HTTP header Expect: 100-continue to requests that carry a body, so that a body is not uploaded just to be rejected while the service is under load. The service can reject the request before a single byte of body is sent, which cuts the bandwidth wasted retrying large uploads against a throttling service. - Adds Request100ContinueOptions and Request100ContinueMode (AUTO, ALWAYS, NEVER) to azure-storage-common. AUTO, the default, applies the header only for a window after the service responds 429, 500, or 503, so the extra round trip is only paid when it earns its keep. - The policies are added below the retry policy so a throttling response takes effect on the very next attempt rather than only on a later call, and above the credential policies since headers may affect the string to sign. - Wired into the blob, data lake, file share, and queue pipelines, and exposed on the blob client builders via request100ContinueOptions. - Can be turned off without a code change by setting the system property or environment variable AZURE_STORAGE_DISABLE_EXPECT_CONTINUE_HEADER. Ported from azure-sdk-for-net #40611 and #52438.
gunjansingh-msft
requested review from
a team,
Alan Zimmer (alzimmermsft),
browndav-msft,
Isabelle (ibrandes),
Kyle Knapp (kyleknap) and
Sean McCullough (seanmcc-msft)
as code owners
August 11, 2026 10:12
|
Azure Pipelines: Successfully started running 1 pipeline(s). 35 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
Isabelle (ibrandes)
left a comment
Isabelle (ibrandes)
left a comment
Member
There was a problem hiding this comment.
first pass :) let me know if you have any questions - thanks for picking this feature up!
| /** | ||
| * Tests for {@link ExpectContinuePolicy} and {@link ExpectContinueOnThrottlePolicy}. | ||
| */ | ||
| public class ExpectContinueTests { |
Member
There was a problem hiding this comment.
every test here builds the pipeline as new HttpPipelineBuilder().policies(...).httpClient(client) where client is RecordingHttpClient, which is a stub that fabricates a response and never opens a socket. So - we're only proving that the policy is in the pipeline, in the right position, and sets the headers under the right conditions (which is good!). however, these tests cant prove that any real HTTP client defers the body until 100 Continue arrives. we should add a per-transport test matrix (netty / okhttp / jdk / vertx) with wire level evidence that show the client withheld the body until 100 continue (and not just that the header was set).
|
|
||
| // Must be after the retry policy so it is evaluated on every attempt, and before the credential policies as | ||
| // headers may affect the string to sign of the request. | ||
| BuilderUtils.addExpectContinuePolicy(policies, null); |
Member
There was a problem hiding this comment.
do we have to add this to datalake / fileshare / queues? i'm reluctant to add it to these packages as on by default with no way to configure it.. in blob, we at least provide the customer with options to turn it off - but in these packages, they have no choice :P lets ask someone from .NET for more info. if we do decide to keep these, we should add coverage for them.
Member
Author
There was a problem hiding this comment.
I asked Jocelyn about this, and it seems like they have implemented this feature only for blobs. shall we do the same?
| * {@code Expect: 100-continue} is applied to requests. | ||
| * @return A new {@link HttpPipeline} from the passed values. | ||
| */ | ||
| public static HttpPipeline buildPipeline(StorageSharedKeyCredential storageSharedKeyCredential, |
Member
There was a problem hiding this comment.
generic comment - we should add HttpHeaderName.EXPECT to BuilderHelper.getDefaultHttpLogOptions() allowed headers to support easier debugging :)
Member
Author
There was a problem hiding this comment.
Added to blob, queue, file share and data lake default log options, with a test.
- Rename Request100ContinueOptions/Request100ContinueMode to ExpectContinueOptions/ExpectContinueMode, the mode values to APPLY_ON_THROTTLE/ON/OFF, autoInterval to throttleInterval, and the builder method to expectContinueBehavior, matching the .NET names. - Make ExpectContinueOnThrottlePolicy extend HttpPipelineSyncPolicy rather than ExpectContinuePolicy. Both policies are now final and share their logic through a package private helper, so neither advertises an extension point that callers outside the package cannot actually use. - Allow the Expect header in the blob default log options, so it is visible when debugging whether a request negotiated the handshake. - Replace the unrelated timeouts link in the options javadoc with RFC 9110. - Add ExpectContinueTransportTests, which drives each HTTP client against a real socket and asserts on what reached the wire and when. The transport tests show that setting the header does not by itself make a client wait. Only okhttp performs the handshake. netty and vertx send the header and stream the body immediately, and jdk-httpclient drops the header entirely since Expect is restricted by java.net.http.HttpClient. That is recorded in the test matrix and documented on ExpectContinueOptions and in the changelog rather than left implicit.
Cover the default log options change with an assertion, and keep the changelog to the feature itself. The transport caveat stays on ExpectContinueOptions, where the behavior it describes lives.
…everywhere Use the .NET casing for the mode values, ApplyOnThrottle / On / Off, rather than Java's upper snake case. Checkstyle accepts it. Allow the Expect header in the default log options of the queue, file share and data lake builders too, not only blob, as the review comment was a general one.
Restructure ExpectContinueTests so the first section mirrors ExpectContinueTests.cs test for test, with the same names and the same case values, making the two suites directly comparable. Java specific coverage, including the per retry behaviour that .NET has no test for, moves to a clearly separated second section. ThrottlePolicyRevertsAfterBackoff is disabled in .NET (Azure/azure-sdk-for-net#41368); the Java mirror is enabled, since the window is a monotonic nanoTime deadline and oversleeping it can only close the window. Extend the transport test to transport x sync/async, as those are separate code paths in every client. All four transports behave the same on both paths, so the earlier finding is unchanged.
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
2 participants
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.
Applies the HTTP header Expect: 100-continue to requests that carry a body, so that a body is not uploaded just to be rejected while the service is under load. The service can reject the request before a single byte of body is sent, which cuts the bandwidth wasted retrying large uploads against a throttling service.
Ported from azure-sdk-for-net #40611 and #52438.
Description
Please add an informative description that covers that changes made by the pull request and link all relevant issues.
If an SDK is being regenerated based on a new swagger spec, a link to the pull request containing these swagger spec changes has been included above.
All SDK Contribution checklist:
General Guidelines and Best Practices
Testing Guidelines