Refactor AWS Chunked logic from HTTP clients #3635

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Merged

kai-ion merged 1 commit into main from chunky

Dec 15, 2025

Contributor

kai-ion commented Dec 2, 2025 •

edited

Loading

Issue #, if available:
#3297

Description of changes:
Our goal is specifically to refactor this logic out of http clients and into a interceptor. We will introduce a core ChunkingInterceptor that owns all aws-chunked behavior for streaming requests and remove the chunking logics from the individual HTTP clients.

Check all that applies:

Did a review by yourself.
Added proper tests to cover this PR. (If tests are not applicable, explain.)
Checked if this PR is a breaking (APIs have been changed) change.
Checked if this PR will not introduce cross-platform inconsistent behavior.
Checked if this PR would require a ReadMe/Wiki update.

Check which platforms you have built SDK on to verify the correctness of this PR.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

kai-ion force-pushed the chunky branch 2 times, most recently from 083ea7a to a6b040e Compare

December 2, 2025 19:54

sbiscigl reviewed

View reviewed changes

src/aws-cpp-sdk-core/include/aws/core/http/interceptor/ChunkingInterceptor.h Outdated Show resolved Hide resolved

src/aws-cpp-sdk-core/include/aws/core/http/interceptor/ChunkingInterceptor.h Outdated Show resolved Hide resolved

src/aws-cpp-sdk-core/source/auth/signer/AWSAuthV4Signer.cpp Outdated Show resolved Hide resolved

src/aws-cpp-sdk-core/source/http/curl/CurlHttpClient.cpp Show resolved Hide resolved

src/aws-cpp-sdk-core/source/http/interceptor/ChunkingInterceptor.cpp Outdated Show resolved Hide resolved

src/aws-cpp-sdk-core/source/http/interceptor/ChunkingInterceptor.cpp Outdated Show resolved Hide resolved

src/aws-cpp-sdk-core/CMakeLists.txt Outdated Show resolved Hide resolved

src/aws-cpp-sdk-core/include/aws/core/client/ClientConfiguration.h Outdated Show resolved Hide resolved

src/aws-cpp-sdk-core/source/http/interceptor/ChunkingInterceptor.cpp Outdated Show resolved Hide resolved

kai-ion force-pushed the chunky branch from a6b040e to 324bd29 Compare

December 3, 2025 16:27

kai-ion marked this pull request as draft

December 3, 2025 21:25

kai-ion marked this pull request as ready for review

December 3, 2025 21:32

sbiscigl reviewed

View reviewed changes

src/aws-cpp-sdk-core/include/smithy/client/features/ChunkingInterceptor.h Outdated Show resolved Hide resolved

src/aws-cpp-sdk-core/include/smithy/client/features/ChunkingInterceptor.h Outdated

Contributor

sbiscigl Dec 4, 2025

why is this buffer 8KB? why is this buffer a c array and not a c++ array? why not make this configurable?

AwsChunkedStream has a buffer of 64KB for double encoding, why are creating a second buffer to buffer that buffer into?

If the answer is "because i need to use the AwsChunkedStream logic to do what i need it to do" then we need to move the logic from chunked stream into this class to avoid a extra buffer, or add additionally APIs to the existing class.

Contributor Author

kai-ion Dec 4, 2025

Ah yes you're right, this was originally setup just because we needed a streambuff interface. I think extending AwsChunkedStream API to add a streamBuff interface makes sense.

src/aws-cpp-sdk-core/include/smithy/client/features/ChunkingInterceptor.h Outdated

Contributor

sbiscigl Dec 4, 2025

why do you need a default constructor?

Contributor Author

kai-ion Dec 4, 2025

aws-sdk-cpp/src/aws-cpp-sdk-core/include/smithy/client/features/ChecksumInterceptor.h

Line 48 in 8a4f5e9

ChecksumInterceptor() = default;

The checksum interceptor also have a default constructor. But I think we actually dont need this default constructor here since we have the explicit clientconfig constructor

src/aws-cpp-sdk-core/include/smithy/client/features/ChunkingInterceptor.h Outdated Show resolved Hide resolved

src/aws-cpp-sdk-core/include/smithy/client/AwsSmithyClientBase.h Outdated Show resolved Hide resolved

src/aws-cpp-sdk-core/source/http/curl/CurlHttpClient.cpp Show resolved Hide resolved

src/aws-cpp-sdk-core/include/smithy/client/features/ChunkingInterceptor.h Show resolved Hide resolved

src/aws-cpp-sdk-core/source/client/ClientConfiguration.cpp Outdated Show resolved Hide resolved

kai-ion force-pushed the chunky branch from 4a758e6 to 4dba26f Compare

December 8, 2025 20:14

kai-ion marked this pull request as draft

December 9, 2025 03:57

kai-ion marked this pull request as ready for review

December 9, 2025 15:40

kai-ion marked this pull request as draft

December 9, 2025 18:12

kai-ion force-pushed the chunky branch from 92b9c5b to 52b64a0 Compare

December 9, 2025 18:17

kai-ion marked this pull request as ready for review

December 9, 2025 21:22

kai-ion force-pushed the chunky branch from 4c65270 to 86518de Compare

December 10, 2025 18:50

sbiscigl reviewed

View reviewed changes

src/aws-cpp-sdk-core/include/smithy/client/features/ChunkingInterceptor.h Outdated Show resolved Hide resolved

src/aws-cpp-sdk-core/include/smithy/client/features/ChunkingInterceptor.h Outdated Show resolved Hide resolved

src/aws-cpp-sdk-core/include/smithy/client/AwsSmithyClientBase.h Outdated Show resolved Hide resolved

src/aws-cpp-sdk-core/include/smithy/client/AwsSmithyClientBase.h Outdated Show resolved Hide resolved

src/aws-cpp-sdk-core/source/smithy/client/AwsSmithyClientBase.cpp Outdated Show resolved Hide resolved

kai-ion force-pushed the chunky branch from 59966fb to 3834efd Compare

December 11, 2025 16:07

sbiscigl reviewed

View reviewed changes

src/aws-cpp-sdk-core/include/smithy/client/features/ChunkingInterceptor.h Show resolved Hide resolved

sbaluja reviewed

View reviewed changes

src/aws-cpp-sdk-core/include/smithy/client/AwsSmithyClientBase.h Outdated

    
                            Aws::MakeShared<ChecksumInterceptor>("AwsSmithyClientBase", *m_clientConfig),

                            Aws::MakeShared<features::ChunkingInterceptor>("AwsSmithyClientBase", [this]() {

                                Aws::Client::ClientConfiguration chunkingConfig = *m_clientConfig;

                                chunkingConfig.httpClientChunkedMode = m_httpClient->IsDefaultAwsHttpClient() ? m_clientConfig->httpClientChunkedMode : Aws::Client::HttpClientChunkedMode::CLIENT_IMPLEMENTATION;

Contributor

sbaluja Dec 12, 2025

Is this the behavior we want? If it's our implementation (IsDefaultAwsHttpClient() == true) then don't we want to force Aws::Client::HttpClientChunkedMode::DEFAULT so that the chunking is applied? And if it's not our implementation then we follow whatever the customer has set, ie m_clientConfig->httpClientChunkedMode. Seems to me this condition should be reversed:

chunkingConfig.httpClientChunkedMode = m_httpClient->IsDefaultAwsHttpClient() ?  Aws::Client::HttpClientChunkedMode::DEFAULT : m_clientConfig->httpClientChunkedMode;

I think we should default initialize clientConfig.httpClientChunkedMode = HttpClientChunkedMode::CLIENT_IMPLEMENTATION; so the client can opt-in for their HttpClient implementation if they want to, but for our implementation we should ensure it's going to use chunking

sbaluja reviewed

View reviewed changes

src/aws-cpp-sdk-core/source/client/ClientConfiguration.cpp Outdated

    
                  clientConfig.httpLibOverride = Aws::Http::TransferLibType::DEFAULT_CLIENT;

                  // Users can explicitly set CLIENT_IMPLEMENTATION if their custom client handles chunking

                  clientConfig.httpClientChunkedMode = HttpClientChunkedMode::DEFAULT;

Contributor

sbaluja Dec 12, 2025

This needs to be

clientConfig.httpClientChunkedMode = HttpClientChunkedMode::CLIENT_IMPLEMENTATION;

We don't want to change existing client's HttpClient behavior unless they opt-in to our chunking logic.

sbiscigl approved these changes

View reviewed changes

Contributor

sbiscigl left a comment

alright its in a good place now, please fix the constructor and do some memory tests and im good to ship

src/aws-cpp-sdk-core/include/smithy/client/AwsSmithyClientBase.h Outdated Show resolved Hide resolved

kai-ion force-pushed the chunky branch 2 times, most recently from 6b7ea6b to d3f7c22 Compare

December 15, 2025 18:17

sbiscigl approved these changes

View reviewed changes

kai-ion force-pushed the chunky branch from d3f7c22 to f558e2c Compare

December 15, 2025 20:45


          refactor: centralize aws-chunked encoding in ChunkingInterceptor

39f8832

Move aws-chunked encoding logic from individual HTTP clients to a
centralized ChunkingInterceptor for better separation of concerns.

- Add ChunkingInterceptor to handle aws-chunked encoding at request level
- Remove custom chunking logic from CRT, Curl, and Windows HTTP clients
- Simplify HTTP clients to focus on transport-only responsibilities
- Maintain full backwards compatibility with existing APIs

unit test for chunking stream

added logic to detect custom http client and smart default

reversing logic to check for chunked mode

changing chunking interceptor to use array instead of vector

kai-ion force-pushed the chunky branch from f558e2c to 39f8832 Compare

December 15, 2025 21:09

kai-ion merged commit 2051054 into main

4 of 5 checks passed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet