copyable smithy client changes

[sbera87]

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.

• 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.

[sbiscigl]

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.

[sbera87]

ok, will remove this

[sbiscigl]

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]

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

[sbiscigl]

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]

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]

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]

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]

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

[sbiscigl]

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

[sbera87]

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.

[sbiscigl]

ooooo good catch let me push a update

[sbera87]

lgtm