Copy bi-directional event stream request #3311

SergeyRyabinin · 2025-02-24T18:33:38Z

Issue #, if available:
Undefined behavior:

on request copying;
undefined behavior if the lifetime of the request is not maintained by user

Description of changes:
Create a request copy for async execution.
Avoid capturing objects by raw pointers into lambdas factories to avoid dangling pointers/reference.

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.

SergeyRyabinin · 2025-02-24T18:48:55Z

src/aws-cpp-sdk-core/source/client/AWSClient.cpp

@@ -305,7 +305,8 @@ HttpResponseOutcome AWSClient::AttemptExhaustively(const Aws::Http::URI& uri,
        coreMetrics.httpClientMetrics = httpRequest->GetRequestMetrics();
        TracingUtils::EmitCoreHttpMetrics(httpRequest->GetRequestMetrics(),
            *m_telemetryProvider->getMeter(this->GetServiceClientName(), {}),
-            {{TracingUtils::SMITHY_METHOD_DIMENSION, request.GetServiceRequestName()},{TracingUtils::SMITHY_SERVICE_DIMENSION, this->GetServiceClientName()}});
+            {{TracingUtils::SMITHY_METHOD_DIMENSION, request.GetServiceRequestName()},


if it crashes again we will know if service client or request is in UB.

SergeyRyabinin · 2025-02-24T18:49:16Z

src/aws-cpp-sdk-core/source/utils/event/EventStreamDecoder.cpp

@@ -53,6 +53,7 @@ namespace Aws

            void EventStreamDecoder::ResetEventStreamHandler(EventStreamHandler* handler)
            {
+                m_eventStreamHandler = handler;


it was a big one.

SergeyRyabinin · 2025-02-24T18:50:22Z

...rator/src/main/resources/com/amazonaws/util/awsclientgenerator/velocity/cpp/RequestHeader.vm

@@ -137,7 +137,7 @@ namespace Model
    /**
     * Underlying Event Stream Handler which is used to define callback functions.
     */
-    inline const ${operation.name}Handler& GetEventStreamHandler() const { return m_handler; }
+    inline ${operation.name}Handler& GetEventStreamHandler() { return m_handler; }


returning object with only non-const methods by const reference is quite meaningless otherwise.

sbiscigl · 2025-02-24T19:56:28Z

src/aws-cpp-sdk-core/include/aws/core/client/AWSClientAsyncCRTP.h

@@ -9,6 +9,7 @@
 #include <aws/core/utils/logging/ErrorMacros.h>
 #include <aws/core/utils/component-registry/ComponentRegistry.h>

+


nit: not changes in file

sbiscigl · 2025-02-24T20:07:13Z

src/aws-cpp-sdk-core/include/aws/core/client/AWSClientEventStreamingAsyncTask.h

+        AWS_LOGSTREAM_FATAL(ClientT::GetAllocationTag(),
+                            "Unexpected nullptr bi-directional streaming request on response streaming factory call!");
+        assert(false);
+        return nullptr;


instead of nullptr should we return a empty string stream? even though we have entered a invalid state, a empty stream might help someone more gracefully exit than a null ptr

ResponseStream is already handling nullptr condition in the c-tor.

aws-sdk-cpp/src/aws-cpp-sdk-core/source/utils/stream/ResponseStream.cpp

Lines 26 to 30 in 9c322a9

ResponseStream::ResponseStream(Aws::IOStream* underlyingStreamToManage) :

m_underlyingStream(underlyingStreamToManage)

{

RegisterStream();

}

sbiscigl · 2025-02-24T20:11:46Z

src/aws-cpp-sdk-core/include/aws/core/client/AWSClientEventStreamingAsyncTask.h

+        if (!request) {
+          AWS_LOGSTREAM_FATAL(ClientT::GetAllocationTag(), "Unexpected nullptr bi-directional streaming request on initial response!");
+          assert(false);
+          return;


nit i prefer no returns on a void function and to rely on if - else behavior i.e.

if (!request) { .... } else if (!response) { .... } else { ..... }

Replaced with ptr check macros.

sbiscigl · 2025-02-24T20:15:21Z

src/aws-cpp-sdk-core/include/aws/core/client/AWSClientEventStreamingAsyncTask.h

+        m_stream(stream),
+        m_method(method),
+        m_signerName(signerName),
+        m_sem(Aws::MakeShared<Aws::Utils::Threading::Semaphore>("BidirectionalEventStreamingTask", 0, 1)) {


why do we want to use a semaphore and not a mutex if max value of semaphore is 1?

That's the original generated design code.
We could switch to a singal, but it will take 2 more member variables (mutex and a conditional_variable) to pass around instead of this single one.

sbiscigl · 2025-02-24T20:18:42Z

tests/aws-cpp-sdk-transcribestreaming-integ-tests/TranscribeErrorCaseTests.cpp

+  };
+  for (size_t i = 0; i < 2; ++i) {
+    client->StartStreamTranscriptionAsync(request, OnStreamReady, OnResponseCallback, nullptr /*context*/);
+    semaphore.WaitOne();


please add a reasonable "drop dead" timeout so that we can avoid locking unit tests if something goes wrong

SergeyRyabinin requested a review from sbiscigl February 24, 2025 18:33

SergeyRyabinin force-pushed the sr/bidirectEventStreamCopyRq branch 2 times, most recently from cad86c2 to f6975df Compare February 24, 2025 18:47

SergeyRyabinin commented Feb 24, 2025

View reviewed changes

SergeyRyabinin force-pushed the sr/bidirectEventStreamCopyRq branch 2 times, most recently from 607e0fe to 602782b Compare February 24, 2025 19:42

sbiscigl reviewed Feb 24, 2025

View reviewed changes

SergeyRyabinin force-pushed the sr/bidirectEventStreamCopyRq branch 2 times, most recently from 9755fd5 to 54ca419 Compare February 24, 2025 21:07

sbiscigl approved these changes Feb 24, 2025

View reviewed changes

SergeyRyabinin force-pushed the sr/bidirectEventStreamCopyRq branch 2 times, most recently from 0ee6fc5 to 2b6fdfb Compare February 25, 2025 08:34

SergeyRyabinin added 2 commits February 25, 2025 08:39

Copy bi-directional event stream operation request

668e0bd

Code generation example

aaaef77

SergeyRyabinin force-pushed the sr/bidirectEventStreamCopyRq branch from 2b6fdfb to aaaef77 Compare February 25, 2025 08:39

sbiscigl approved these changes Feb 25, 2025

View reviewed changes

SergeyRyabinin merged commit 50f3795 into main Feb 25, 2025
3 of 4 checks passed

SergeyRyabinin deleted the sr/bidirectEventStreamCopyRq branch February 25, 2025 16:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Copy bi-directional event stream request #3311

Copy bi-directional event stream request #3311

SergeyRyabinin commented Feb 24, 2025 •

edited

Loading

SergeyRyabinin Feb 24, 2025

SergeyRyabinin Feb 24, 2025

SergeyRyabinin Feb 24, 2025

sbiscigl Feb 24, 2025

sbiscigl Feb 24, 2025

SergeyRyabinin Feb 24, 2025

sbiscigl Feb 24, 2025

SergeyRyabinin Feb 24, 2025

sbiscigl Feb 24, 2025

SergeyRyabinin Feb 24, 2025

sbiscigl Feb 24, 2025

		@@ -9,6 +9,7 @@
		#include <aws/core/utils/logging/ErrorMacros.h>
		#include <aws/core/utils/component-registry/ComponentRegistry.h>

	ResponseStream::ResponseStream(Aws::IOStream* underlyingStreamToManage) :
	m_underlyingStream(underlyingStreamToManage)
	{
	RegisterStream();
	}

Copy bi-directional event stream request #3311

Copy bi-directional event stream request #3311

Conversation

SergeyRyabinin commented Feb 24, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SergeyRyabinin commented Feb 24, 2025 •

edited

Loading