copyable smithy client changes #3236

sbera87 · 2025-01-02T21:55:35Z

Issue #, if available:

Description of changes:

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.

sbiscigl · 2025-01-03T15:10:34Z

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

+                m_httpClient = Aws::Http::CreateHttpClient(*target.m_clientConfig);
+
+                // marshaller could be of different types
+                if (auto jsonMarshaller = std::dynamic_pointer_cast<Aws::Client::JsonErrorMarshaller>(m_errorMarshaller)) {


not really a fan of the the use of dynamic_pointer_cast here, I think this is a symptom of the fact that we shouldnt be declaring a custom copy constructor here, but we should be in AwsSmithyClient.h, because at that point in time we know what that type of error marshaller exists.

Also this will break the cmake arg ENABLE_RTTI if we turn it to false, which some of our customers do.

The custom copy constructor and assignment operator can call down to AwsSmithyClientBase functions that take these things as arguments.

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

sbiscigl · 2025-01-06T15:13:12Z

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

@@ -75,6 +75,29 @@ namespace Client
            return *this;
        }

+        ClientWithAsyncTemplateMethods& operator=(ClientWithAsyncTemplateMethods&& other)


why are we adding a copy ctor for AsyncCRTP? this is unused in the smithy code path?

It is one of the classes in multi inheritance that is used.

class AWS_DYNAMODB_API DynamoDBClient : smithy::client::AwsSmithyClientT<Aws::DynamoDB::SERVICE_NAME, Aws::DynamoDB::DynamoDBClientConfiguration, smithy::SigV4AuthSchemeResolver<>, Aws::Crt::Variant<smithy::SigV4AuthScheme>, DynamoDBEndpointProviderBase, smithy::client::JsonOutcomeSerializer, smithy::client::JsonOutcome, Aws::Client::DynamoDBErrorMarshaller>, Aws::Client::ClientWithAsyncTemplateMethods<DynamoDBClient>

every client in the current SDK inherits from this already. and they are copy constructible currently

#include <aws/core/Aws.h> #include <aws/logs/CloudWatchLogsClient.h> using namespace Aws; using namespace Aws::CloudWatchLogs; auto main() -> int { SDKOptions options; options.loggingOptions.logLevel = Aws::Utils::Logging::LogLevel::Debug; InitAPI(options); { CloudWatchLogsClient client{}; CloudWatchLogsClient dup{client}; (void) dup; } ShutdownAPI(options); return 0; }

Lets not add explicit copy constructors to existing objects that dont require it for a smithy client to be copy constructible. Lets limit this change to only allowing smithy clients to be copy constructible.

sbiscigl · 2025-01-06T15:14:37Z

src/aws-cpp-sdk-core/include/aws/core/utils/Cache.h

@@ -27,6 +27,31 @@ namespace Aws
            explicit Cache(size_t initialSize = 1000) : m_maxSize(initialSize)
            {
            }
+
+            //this can't be default as compiler wont implicitly create copy constructor for const qualified members
+            Cache(const Cache& other):m_entries{other.m_entries},m_maxSize{other.m_maxSize}{


we are NOT making anything with a cache copy constructible, if the client is not copy constructible from before the smithy client migration, it does not need to be copy constructible afterwards.

ok, will remove this

sbiscigl · 2025-01-06T15:15:39Z

src/aws-cpp-sdk-core/include/aws/core/utils/ConcurrentCache.h

@@ -19,6 +19,33 @@ namespace Aws
        public:
            explicit ConcurrentCache(size_t size = 1000) : m_cache(size) { }

+
+            ConcurrentCache(const ConcurrentCache& other) : m_cache(other.m_cache) { 


same comment as above, we are not retroactively making clients copy constructible, just client that are copy constructible we will keep that way. so anything that has a cache, is not copy constructible and we dont plan on changing that.

ok, will remove this one

...in/resources/com/amazonaws/util/awsclientgenerator/velocity/cpp/smithy/SmithyClientHeader.vm

sbiscigl · 2025-01-06T15:58:07Z

tests/aws-cpp-sdk-dynamodb-integration-tests/TableOperationTest.cpp

@@ -130,6 +130,7 @@ class TableOperationTest : public ::testing::Test {
        {
            putItemResultSemaphore.notify_all();
        }
+


remove white space, it removes this file for the PR

...in/resources/com/amazonaws/util/awsclientgenerator/velocity/cpp/smithy/SmithyClientHeader.vm

sbiscigl · 2025-01-06T16:07:34Z

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

@@ -75,6 +75,29 @@ namespace Client
            return *this;
        }

+        ClientWithAsyncTemplateMethods& operator=(ClientWithAsyncTemplateMethods&& other)


every client in the current SDK inherits from this already. and they are copy constructible currently

#include <aws/core/Aws.h> #include <aws/logs/CloudWatchLogsClient.h> using namespace Aws; using namespace Aws::CloudWatchLogs; auto main() -> int { SDKOptions options; options.loggingOptions.logLevel = Aws::Utils::Logging::LogLevel::Debug; InitAPI(options); { CloudWatchLogsClient client{}; CloudWatchLogsClient dup{client}; (void) dup; } ShutdownAPI(options); return 0; }

Lets not add explicit copy constructors to existing objects that dont require it for a smithy client to be copy constructible. Lets limit this change to only allowing smithy clients to be copy constructible.

…ompatibility

…ard compatibility

sbiscigl · 2025-01-07T14:57:40Z

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

@@ -65,6 +67,8 @@ namespace client
    /* Non-template base client class that contains main Aws Client Request pipeline logic */
    class SMITHY_API AwsSmithyClientBase
    {
+    private:
+        const char* ALLOC_TAG{"AwsSmithyClientBase"};


personally would rather just have you inline this string into the MakeShared calls to avoid a private member. or move the ctors to a cpp file, and create string constant there.

sbiscigl · 2025-01-07T15:29:54Z

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

@@ -131,11 +167,11 @@ namespace client
        }

    protected:
-        ServiceClientConfigurationT& m_clientConfiguration;
+        ServiceClientConfigurationT m_clientConfiguration;


i dont think we want to be removing the refence. i think this might root cause to the conversation we were having about deep vs shallow copy ctor. I think this might be more aptly summarized as "lets not share references between constructs in ClientConfiguration via the default copy constructor".

i.e.

#include <aws/core/Aws.h> using namespace Aws; auto main() -> int { SDKOptions options; options.loggingOptions.logLevel = Aws::Utils::Logging::LogLevel::Debug; InitAPI(options); { Client::ClientConfiguration configuration{}; // new_configuration shares all the same shared ptrs as configuration! Client::ClientConfiguration new_configuration{configuration}; } ShutdownAPI(options); return 0; }

For example in client configuration we have a retry strategy that is a shared pointer

std::shared_ptr<RetryStrategy> retryStrategy = nullptr;

which if we call the default copy constructor on will create a shared reference to the same retry strategy that is owned by the original client.

What we want to do is that the new client owns its own retry strategy. i.e. the retry strategy is initialized from the factory that we provide

/** * Retry Strategy factory method. Default is DefaultRetryStrategy (i.e. exponential backoff). */ std::function<std::shared_ptr<RetryStrategy>()> retryStrategyCreateFn;

This isnt limited to specifically the retry strategy but the shared pointers in client configuration that are shared on copy.

So I think the high level ask here is "Lets create a deep copy function on ClientConfiguration that returns a client configuration with the factories called instead of shared ownership over the pointers, that is explicitly called when copying the new smithy clients".

Note: ServiceClientConfigurationT isnt constrainted here, and thats likely a miss, but for all intents and purposes it always a child of ClientConfiguration and we can statically assert on that if we want.

I know thats a mouthful of words, this is touching a lot of legacy stuff and dealing with a lot of things we have got to conveniently ignore for a while, but we're really close to having a better and cleaner design with this change for this moving forward!

sbiscigl · 2025-01-07T20:21:31Z

generated/src/aws-cpp-sdk-dynamodb/include/aws/dynamodb/DynamoDBClient.h

+            return *this;
+        }
+        AwsSmithyClientT::operator=(std::move(other));
+        Aws::Client::ClientWithAsyncTemplateMethods<DynamoDBClient>::operator=(std::move(other));


dont think you can call std::move on a r value twice without entering UB land.

sbiscigl · 2025-01-07T20:29:23Z

generated/src/aws-cpp-sdk-dynamodb/include/aws/dynamodb/DynamoDBClient.h

+        return *this;
+      }
+
+      DynamoDBClient& operator=(DynamoDBClient&& other)


do we even need a custom move ctor/assignment operator? thinking that default would be enough because the target would be the new owner of the all of the resources, that are already intiialized.

thinking default for move should be enough

sbiscigl · 2025-01-07T20:33:36Z

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

        {
            m_serviceName = ServiceNameT;
        }

+        AwsSmithyClientT(const AwsSmithyClientT& other):
+            AwsSmithyClientBase(Aws::MakeShared<ServiceClientConfigurationT>(ServiceNameT, other.m_clientConfig), other.m_serviceName, Aws::Http::CreateHttpClient(*other.m_clientConfig), Aws::MakeShared<MarshallerT>(other.m_serviceName.c_str())),


for the sake of consistency MakeShared should be called with the same log tag, i.e.
Aws::MakeShared<ServiceClientConfigurationT>(ServiceNameT, other.m_clientConfig)
vs
Aws::MakeShared<MarshallerT>(other.m_serviceName.c_str())

other.m_serviceName should be the same ServiceNameT but since this is the class that is calling MakeShared lets make it consistent to ServiceNameT for both

sbiscigl · 2025-01-07T20:39:55Z

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

@@ -65,6 +67,33 @@ namespace client
    /* Non-template base client class that contains main Aws Client Request pipeline logic */
    class SMITHY_API AwsSmithyClientBase
    {
+    private:


lets move private definitions after public per the google style guide

A class definition should usually start with a public: section, followed by protected:, then private:. Omit sections that would be empty.

sbiscigl · 2025-01-07T21:12:39Z

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

+        void deepCopyClientConfiguration(const AwsSmithyClientBase& target)
+        {
+            //first create copy from target
+            m_clientConfig = Aws::MakeShared<Aws::Client::ClientConfiguration>(target.m_serviceName.c_str(), *target.m_clientConfig);


i dont think this is going to work the way we want it to because of slicing here we declare a pointer of ClientConfiguration not ServiceClientConfigurationT. in AwsSmithyClientT we call it as such

m_clientConfiguration{*static_cast<ServiceClientConfigurationT*>(AwsSmithyClientBase::m_clientConfig.get())},

since this is NOT a ServiceClientConfigurationT we enter UB territory.

trying to think of a creative solution around this let me talk to you offline about this

sbera87 · 2025-01-08T20:13:02Z

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

+        }
+
+        void init() {
+          if (!m_clientConfig->retryStrategy)


imo, we should remove these checks and do an unconditional invocation of the functors because when we create the shared ptr from another, it does a shallow copy of the functors too and then it won't reinitialize

I dont think we can do that. consider the code

#include <aws/core/Aws.h> #include <aws/core/client/AdaptiveRetryStrategy.h> #include <aws/dynamodb/DynamoDBClient.h> using namespace Aws; using namespace Aws::DynamoDB; namespace { const char* LOG_TAG = "TAG"; } auto main() -> int { SDKOptions options; options.loggingOptions.logLevel = Aws::Utils::Logging::LogLevel::Debug; InitAPI(options); { Client::ClientConfiguration configuration{}; configuration.retryStrategy = Aws::MakeShared<Client::AdaptiveRetryStrategy>(LOG_TAG); DynamoDBClient client{}; } ShutdownAPI(options); return 0; }

if we remove the check we would unconditionally write over the users specifed retry strategy with the default factory.

So we need to check for the existence of a user specified entity on the configuration before invoking the factory

as for specifically having different logic for copy constructor vs regular constructor, it makes sense to have the same logic so we dont have to update in two places

In the above example
DynamoDBClient client
This calls the constructor which is supposed to do the initialization of shared ptr members because in the very beginning sp is default nullptr.
Now say we copy the client which will invoke the deepcopy method and first it will shallow copy the shared ptr members and then wont reinitialize it using the factory functions. This means 2 clients will have same copy of shared_ptr to retry strategy for example. But in reality each client should have its own separate retry strategy.

if (!m_clientConfig->retryStrategy) will otherwise always be false on a copy.

ooooo good catch let me push a update

sbera87 · 2025-01-08T20:14:41Z

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

+          if (!m_clientConfig->executor)
+          {
+            assert(m_clientConfig->configFactories.executorCreateFn);
+            m_clientConfig->executor = m_clientConfig->configFactories.executorCreateFn();


m_isInitialized should be set true since you have remove the client level init (auto generated)

It is not needed because it is set in the constructor of ClientWithAsyncTemplateMethods which we invoke when we create a dynamo client. Before it was redundant so Im trying to remove redundancy with removing that init call.

class AWS_DYNAMODB_API DynamoDBClient : smithy::client::AwsSmithyClientT<Aws::DynamoDB::SERVICE_NAME, Aws::DynamoDB::DynamoDBClientConfiguration, smithy::SigV4AuthSchemeResolver<>, Aws::Crt::Variant<smithy::SigV4AuthScheme>, DynamoDBEndpointProviderBase, smithy::client::JsonOutcomeSerializer, smithy::client::JsonOutcome, Aws::Client::DynamoDBErrorMarshaller>, Aws::Client::ClientWithAsyncTemplateMethods<DynamoDBClient> { ... } template <typename AwsServiceClientT> class ClientWithAsyncTemplateMethods { public: ClientWithAsyncTemplateMethods(): m_isInitialized(true), m_operationsProcessed(0) { ... } ... }

If we don't need this then its ok

copyable client changes

564f107

sbera87 requested review from sbiscigl and SergeyRyabinin January 2, 2025 21:55

sbera87 changed the title ~~copyable client changes~~ copyable smithy client changes Jan 2, 2025

sbiscigl reviewed Jan 3, 2025

View reviewed changes

copy client changes with test case

4707c81

sbera87 requested a review from sbiscigl January 5, 2025 18:42

sbiscigl reviewed Jan 6, 2025

View reviewed changes

don't make cachec copy constructible

dc62708

sbera87 requested a review from sbiscigl January 6, 2025 15:56

add new line for readablity

1c8aa67

sbiscigl reviewed Jan 6, 2025

View reviewed changes

sbera87 added 2 commits January 6, 2025 11:17

removed assignemnt operators, move constructor to stick to backward c…

f80d6db

…ompatibility

removed move assignemnt operators, move constructor to stick to backw…

c68e7b7

…ard compatibility

sbera87 requested a review from sbiscigl January 6, 2025 16:20

sbera87 added 5 commits January 6, 2025 11:25

remove ws + regenerate client file

961cb3c

cleanup

0b2f1a1

fix warning on windows build

f745ba5

client configuration copy initialization

71bc24e

fix copy constructor logic + cleanup

fb301fe

sbiscigl reviewed Jan 7, 2025

View reviewed changes

config deep copy changes

b7441f0

sbiscigl reviewed Jan 7, 2025

View reviewed changes

SergeyRyabinin approved these changes Jan 8, 2025

View reviewed changes

update to deep copy

cc3e9e4

sbiscigl force-pushed the smithy_copy branch from 7b5df87 to cc3e9e4 Compare January 8, 2025 20:08

sbera87 commented Jan 8, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

copyable smithy client changes #3236

copyable smithy client changes #3236

sbera87 commented Jan 2, 2025 •

edited

Loading

sbiscigl Jan 3, 2025

sbiscigl Jan 6, 2025 •

edited

Loading

sbera87 Jan 6, 2025

sbiscigl Jan 6, 2025

sbiscigl Jan 6, 2025

sbera87 Jan 6, 2025

sbiscigl Jan 6, 2025 •

edited

Loading

sbera87 Jan 6, 2025

sbiscigl Jan 6, 2025

sbiscigl Jan 6, 2025

sbiscigl Jan 7, 2025

sbiscigl Jan 7, 2025

sbiscigl Jan 7, 2025

sbiscigl Jan 7, 2025

sbiscigl Jan 7, 2025

sbiscigl Jan 7, 2025

sbiscigl Jan 7, 2025

sbera87 Jan 8, 2025

sbiscigl Jan 8, 2025 •

edited

Loading

sbera87 Jan 8, 2025

sbiscigl Jan 8, 2025

sbera87 Jan 8, 2025

sbiscigl Jan 8, 2025

sbera87 Jan 8, 2025

copyable smithy client changes #3236

Are you sure you want to change the base?

copyable smithy client changes #3236

Conversation

sbera87 commented Jan 2, 2025 • edited Loading

Choose a reason for hiding this comment

sbiscigl Jan 6, 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

sbiscigl Jan 6, 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

Choose a reason for hiding this comment

sbiscigl Jan 8, 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

sbera87 commented Jan 2, 2025 •

edited

Loading

sbiscigl Jan 6, 2025 •

edited

Loading

sbiscigl Jan 6, 2025 •

edited

Loading

sbiscigl Jan 8, 2025 •

edited

Loading