Issue-3364: Reproduction + fix(Proposal )

[rajanyadav0307]

Issue #, if available:

Issue-3364: Fix Proposal

---

Description of changes:

This PR fixes an issue in CurlHttpClient::WriteData where querying the output position of the response body stream could cause the HTTP transfer to abort when the stream does not support positioning (e.g., boost::iostreams::filtering_stream with compression/decompression filters).

Problem

The existing implementation attempted to query the current write position of the response body stream using positioning APIs (tellp() / seek operations) for diagnostic purposes. For non-seekable output streams, this operation may fail and set the stream into a bad state, which caused the SDK to treat the write as a fatal error and abort the transfer, even though the actual write() operation itself was valid and succeeded.

This behavior is expected for certain stream types and should not be treated as a write failure.

Fix

• Avoid treating unsupported output position queries as fatal errors.
• Introduce a best-effort, non-intrusive position probe that detects whether the stream supports positioning without modifying stream state.
• Preserve existing behavior for seekable streams while allowing non-seekable streams to function correctly.
• Continue using the existing byte counter (m_numBytesResponseReceived) for accurate response size tracking.

This change improves compatibility with clients that use non-seekable or filtering output streams without altering public APIs or behavior for existing seekable stream use cases.

---

Check all that applies:

• Did a review by yourself.
• Added proper tests to cover this PR.
  (Added a regression test guarded by ENABLE_HTTP_CLIENT_TESTING that validates non-seekable response streams do not abort transfers. This follows the existing CurlHttpClient test pattern and requires the local dummy HTTP server for execution.)
• 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.

---

Platforms verified:

• Linux
• Windows
• Android
• MacOS
• IOS
• Other Platforms

---

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

Sign-off:
Rajan Y. (rajanyadav0307@gmail.com)

[rajanyadav0307]

@sbiscigl ,
Please help reviewing the fix proposal.

Thanks

[rajanyadav0307]

@sbiscigl ,

Please can you review this change?

Thanks

[kai-ion]

@rajanyadav0307 Thanks for the detail report and code update, I was able to verify the fix worked as you intended. Could you extend this fix to the other HTTP client as well?

[rajanyadav0307]

@kai-ion
Thanks for reviewing the fix.

Sure I will extend this fix to the other HTTP client as well.

[rajanyadav0307]

Thanks @kai-ion for taking a look and confirming the CurlHttpClient fix.

For this PR, I’ve focused on extending the same behavior to CRTHttpClient, aligning it with CurlHttpClient’s handling of response output streams:

1. Avoid assuming the response stream is seekable
2. Fail fast when the output stream enters a bad state
3. Rely on byte counts provided by the transport rather than tellp()-style position queries

I’ve verified that the CRT path already tracks bytes received via the transport callbacks, so no position-based querying is required there, and the fix is limited to improving write/flush error handling consistency.

While reviewing this, I also noticed that IXmlHttpRequest2HttpClient follows a very similar pattern (stream writes via a sequential adapter and an unconditional final flush). It would benefit from the same defensive checks and error propagation for non-seekable or filtering streams.

I intentionally did not include that change in this PR to keep the scope tight and focused on Curl + CRT parity. If you’re comfortable with the CRT approach here, I’m happy to extend the same pattern to IXmlHttpRequest2HttpClient in a follow-up (or in this PR if you prefer).

Please let me know how you’d like to proceed.

[rajanyadav0307]

@kai-ion ,
Please can you help with next steps?

Thanks

[sbaluja]

Thanks for your PR. Lets try to scope the changes down here. The original issue was that the call to response->GetResponseBody().tellp() fails. Let's only change that behavior. If the tellp() fail behavior exists in the other HttpClients aside from curl (Crt, windows), we should apply the changes there as well. If not, I'm reluctant on a refactor for such core functionality with no proven reason/issue. I'd like this PR to only be related to the original issue of seeking on non-seekable streams, and apply to all our httpclients (if they also have this behavior). This PR should have a much smaller diff.

[rajanyadav0307]

Thanks @sbaluja for bringing here. From my understanding, I don't see other HTTP clinet using tellp() other than CurlHttpClient. So, this brings us to the change related to only CurlHttpClient changes. I will make the PR just addressing for the CurlHttpClient.

[rajanyadav0307]

@sbaluja ,
I have rebased the changes just picking up to for the CurlHttpClient as we discussed. Please help to review and merge the changes.

Thanks.

[sbaluja]

Is this required? I believe we have other curl specific tests in this suite already.

[rajanyadav0307]

Thanks @sbaluja ,

The reason for adding this was because I encounter errors for CURLHttpClientTest:

#7 216.0 /work/aws-sdk-cpp/tests/aws-cpp-sdk-core-tests/http/HttpClientTest.cpp:370:8: error: 'CURLHttpClientTest' was not declared in this scope; did you mean 'HttpClientTest'? #7 216.0 370 | TEST_F(CURLHttpClientTest, TestHttpRequestWorksFine)
:
:
All other CURLHttpClientTest.

I have explicitly compiled the core component, and tried to ran the core-test.

Dockerfile:

tellp-fix.zip

[sbaluja]

Thanks for the update. Have you tested what the behavior of non-seekable streams is for the other http clients? The test you've written is currently only for the curl client. If the behavior is the same across the clients, this test should be written similar to other tests in the suite that use different clients based on platform. Let me know.

[rajanyadav0307]

@sbaluja ,
Thanks for bringing the test code. I added the class explicitly because I got the errors(trimmed from the error starting part):
Errors.rtf

Please point out, if I need to change anything while compile and running.

My build steps:

1. Configure:
   cmake .. \ -DLEGACY_BUILD=ON \ -DENABLE_TESTING=ON \ -DENABLE_CURL_CLIENT=ON \ -DENABLE_HTTP_CLIENT_TESTING=ON \ -DAUTORUN_UNIT_TESTS=OFF

2. Build the core test binary
   cmake --build . --target aws-cpp-sdk-core-tests -- -j

Couple of things I would like to bring:

1. Currently, as you mentioned the test covers non-seekable stream test case only for CURLHttpClient.
2. Since, we are not updating any other Http Client, I thought of not adding any unrelated coverage for them.
3. I think as you mentioned in the last discussion we can use this PR to target only CurlHttpClient and primarily focus on relevant test.

If you want me to adopt any other plan, please let me know. I would be happy to align accordingly.
Thanks.