Skip to content

Commit bd1a11b

Browse files
SergeyRyabininSergeyRyabinin
authored
Fix smithy client copy assignment (#3327)
1 parent 3d0cdc2 commit bd1a11b

File tree

10 files changed

+504
-77
lines changed

10 files changed

+504
-77
lines changed

‎cmake/sdksCommon.cmake

+1-1
Original file line numberDiff line numberDiff line change
@@ -95,7 +95,7 @@ list(APPEND SDK_TEST_PROJECT_LIST "elasticfilesystem:tests/aws-cpp-sdk-elasticfi
9595
list(APPEND SDK_TEST_PROJECT_LIST "identity-management:tests/aws-cpp-sdk-identity-management-tests")
9696
list(APPEND SDK_TEST_PROJECT_LIST "kinesis:tests/aws-cpp-sdk-kinesis-integration-tests")
9797
list(APPEND SDK_TEST_PROJECT_LIST "lambda:tests/aws-cpp-sdk-lambda-integration-tests")
98-
list(APPEND SDK_TEST_PROJECT_LIST "logs:tests/aws-cpp-sdk-logs-integration-tests")
98+
list(APPEND SDK_TEST_PROJECT_LIST "logs:tests/aws-cpp-sdk-logs-integration-tests,tests/aws-cpp-sdk-logs-unit-tests")
9999
list(APPEND SDK_TEST_PROJECT_LIST "mediastore-data:tests/aws-cpp-sdk-mediastore-data-integration-tests")
100100
list(APPEND SDK_TEST_PROJECT_LIST "monitoring:tests/aws-cpp-sdk-monitoring-integration-tests")
101101
list(APPEND SDK_TEST_PROJECT_LIST "rds:tests/aws-cpp-sdk-rds-integration-tests")

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

+53-26
Original file line numberDiff line numberDiff line change
@@ -60,55 +60,78 @@ namespace client
6060
m_authSchemes(authSchemes),
6161
m_serializer(Aws::MakeShared<SerializerT>(ServiceNameT, m_clientConfiguration.telemetryProvider))
6262
{
63-
m_serviceName = ServiceNameT;
6463
initClient();
6564
}
6665

67-
AwsSmithyClientT(const AwsSmithyClientT& other):
68-
AwsSmithyClientBase(other,
69-
Aws::MakeUnique<ServiceClientConfigurationT>(ServiceNameT, other.m_clientConfiguration),
70-
ServiceNameT,
71-
other.m_serviceUserAgentName,
72-
Aws::Http::CreateHttpClient(other.m_clientConfiguration),
73-
Aws::MakeShared<ErrorMarshallerT>(ServiceNameT)),
74-
m_clientConfiguration{*static_cast<ServiceClientConfigurationT*>(m_clientConfig.get())},
75-
m_endpointProvider{other.m_endpointProvider},
76-
m_authSchemeResolver{Aws::MakeShared<ServiceAuthSchemeResolverT>(ServiceNameT)},
77-
m_authSchemes{other.m_authSchemes},
78-
m_serializer{Aws::MakeShared<SerializerT>(ServiceNameT, m_clientConfiguration.telemetryProvider)}
66+
AwsSmithyClientT(const AwsSmithyClientT& other)
67+
: AwsSmithyClientBase(other,
68+
Aws::MakeUnique<ServiceClientConfigurationT>(ServiceNameT, other.m_clientConfiguration),
69+
Aws::Http::CreateHttpClient(other.m_clientConfiguration),
70+
Aws::MakeShared<ErrorMarshallerT>(ServiceNameT)),
71+
m_clientConfiguration(*static_cast<ServiceClientConfigurationT*>(m_clientConfig.get()))
7972
{
80-
initClient();
73+
m_endpointProvider = other.m_endpointProvider; /* shallow copy */
74+
m_authSchemeResolver = other.m_authSchemeResolver; /* shallow copy */
75+
m_authSchemes = other.m_authSchemes;
76+
m_serializer = Aws::MakeShared<SerializerT>(ServiceNameT, m_clientConfiguration.telemetryProvider);
77+
initClient();
8178
}
8279

8380
AwsSmithyClientT& operator=(const AwsSmithyClientT& other)
8481
{
8582
if(this != &other)
8683
{
87-
AwsSmithyClientBase::deepCopy(Aws::MakeUnique<ServiceClientConfigurationT>(ServiceNameT, other.m_clientConfiguration),
88-
ServiceNameT,
89-
Aws::Http::CreateHttpClient(other.m_clientConfiguration),
84+
m_clientConfiguration = other.m_clientConfiguration;
85+
AwsSmithyClientBase::baseCopyAssign(other,
86+
Aws::Http::CreateHttpClient(m_clientConfiguration),
9087
Aws::MakeShared<ErrorMarshallerT>(ServiceNameT));
91-
m_clientConfiguration = *static_cast<ServiceClientConfigurationT*>(m_clientConfig.get());
92-
m_endpointProvider = other.m_endpointProvider;
93-
m_authSchemeResolver = Aws::MakeShared<ServiceAuthSchemeResolverT>(ServiceNameT);
88+
89+
m_endpointProvider = other.m_endpointProvider; /* shallow copy */
90+
m_authSchemeResolver = other.m_authSchemeResolver; /* shallow copy */
9491
m_authSchemes = other.m_authSchemes;
9592
m_serializer = Aws::MakeShared<SerializerT>(ServiceNameT, m_clientConfiguration.telemetryProvider);
96-
m_errorMarshaller = Aws::MakeShared<ErrorMarshallerT>(ServiceNameT);
9793
initClient();
9894
}
9995
return *this;
10096
}
10197

102-
AwsSmithyClientT (AwsSmithyClientT&&) = default;
98+
AwsSmithyClientT(AwsSmithyClientT&& other) :
99+
AwsSmithyClientBase(std::move(static_cast<AwsSmithyClientBase&&>(other)),
100+
Aws::MakeUnique<ServiceClientConfigurationT>(ServiceNameT, std::move(other.m_clientConfiguration))),
101+
m_clientConfiguration{*static_cast<ServiceClientConfigurationT*>(m_clientConfig.get())},
102+
m_endpointProvider(std::move(other.m_endpointProvider)),
103+
m_authSchemeResolver(std::move(other.m_authSchemeResolver)),
104+
m_authSchemes(std::move(other.m_authSchemes)),
105+
m_serializer(std::move(other.m_serializer))
106+
{
107+
}
103108

104-
AwsSmithyClientT& operator=(AwsSmithyClientT&&) = default;
109+
AwsSmithyClientT& operator=(AwsSmithyClientT&& other)
110+
{
111+
if(this != &other)
112+
{
113+
m_clientConfiguration = std::move(other.m_clientConfiguration);
114+
AwsSmithyClientBase::baseMoveAssign(std::move(static_cast<AwsSmithyClientBase&&>(other)));
115+
116+
m_endpointProvider = std::move(other.m_endpointProvider);
117+
m_authSchemeResolver = std::move(other.m_authSchemeResolver);
118+
m_authSchemes = std::move(other.m_authSchemes);
119+
m_serializer = std::move(other.m_serializer);
120+
}
121+
return *this;
122+
}
105123

106124
virtual ~AwsSmithyClientT() = default;
107125

108126
protected:
109127
void initClient() {
110-
m_endpointProvider->InitBuiltInParameters(m_clientConfiguration);
111-
m_authSchemeResolver->Init(m_clientConfiguration);
128+
if (m_endpointProvider && m_authSchemeResolver) {
129+
m_endpointProvider->InitBuiltInParameters(m_clientConfiguration);
130+
m_authSchemeResolver->Init(m_clientConfiguration);
131+
} else {
132+
AWS_LOGSTREAM_FATAL(ServiceNameT, "Unable to init client: endpoint provider=" << m_endpointProvider
133+
<< " or " << "authSchemeResolver=" << m_authSchemeResolver << " are null!");
134+
}
112135
}
113136

114137
inline const char* GetServiceClientName() const override { return m_serviceName.c_str(); }
@@ -226,7 +249,7 @@ namespace client
226249
const Aws::Http::URI& uri = endpoint.GetURI();
227250
auto signerRegionOverride = region;
228251
auto signerServiceNameOverride = serviceName;
229-
//signer name is needed for some identity resolvers
252+
// signer name is needed for some identity resolvers
230253
if (endpoint.GetAttributes()) {
231254
if (endpoint.GetAttributes()->authScheme.GetSigningRegion()) {
232255
signerRegionOverride = endpoint.GetAttributes()->authScheme.GetSigningRegion()->c_str();
@@ -242,6 +265,10 @@ namespace client
242265
}
243266

244267
protected:
268+
/* Service client specific config, the actual object is stored in AwsSmithyClientBase by pointer
269+
* In order to avoid config object duplication, smithy template client access it by a reference.
270+
* So that base client has it by base config pointer, child smithy client has it by child config reference.
271+
*/
245272
ServiceClientConfigurationT& m_clientConfiguration;
246273
std::shared_ptr<EndpointProviderT> m_endpointProvider{};
247274
std::shared_ptr<ServiceAuthSchemeResolverT> m_authSchemeResolver{};

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

+33-26
Original file line numberDiff line numberDiff line change
@@ -85,6 +85,7 @@ namespace client
8585
using ResolveEndpointOutcome = Aws::Utils::Outcome<Aws::Endpoint::AWSEndpoint, AWSError>;
8686
using StreamOutcome = Aws::Utils::Outcome<Aws::AmazonWebServiceResult<Aws::Utils::Stream::ResponseStream>, AWSError >;
8787

88+
/* primary constructor */
8889
AwsSmithyClientBase(Aws::UniquePtr<Aws::Client::ClientConfiguration>&& clientConfig,
8990
Aws::String serviceName,
9091
Aws::String serviceUserAgentName,
@@ -100,27 +101,31 @@ namespace client
100101
baseInit();
101102
}
102103

104+
/* copy constructor substitute */
103105
AwsSmithyClientBase(const AwsSmithyClientBase& other,
104106
Aws::UniquePtr<Aws::Client::ClientConfiguration>&& clientConfig,
105-
Aws::String serviceName,
106-
Aws::String serviceUserAgentName,
107107
std::shared_ptr<Aws::Http::HttpClient> httpClient,
108108
std::shared_ptr<Aws::Client::AWSErrorMarshaller> errorMarshaller) :
109-
m_clientConfig(std::move(clientConfig)),
110-
m_serviceName(std::move(serviceName)),
111-
m_serviceUserAgentName(std::move(serviceUserAgentName)),
112-
m_httpClient(std::move(httpClient)),
113-
m_errorMarshaller(std::move(errorMarshaller)),
114-
m_interceptors{Aws::MakeShared<ChecksumInterceptor>("AwsSmithyClientBase", *m_clientConfig)}
109+
m_clientConfig(std::move(clientConfig))
110+
{
111+
// this c-tor needs httpClient and errorMarshaller passed explicitly because this base class stores them
112+
// by their parent pointer classes
113+
// and base client class has no idea how to re-create or copy them, and "lombok toBuilder" is not a thing in cpp.
114+
baseCopyAssign(other, std::move(httpClient), std::move(errorMarshaller));
115+
}
116+
117+
/* move constructor substitute */
118+
AwsSmithyClientBase(AwsSmithyClientBase&& other,
119+
Aws::UniquePtr<Aws::Client::ClientConfiguration>&& clientConfig) :
120+
m_clientConfig(std::move(clientConfig))
115121
{
116-
AWS_UNREFERENCED_PARAM(other);
117-
baseCopyInit();
122+
baseMoveAssign(std::move(other));
118123
}
119124

120125
AwsSmithyClientBase(AwsSmithyClientBase& target) = delete;
121126
AwsSmithyClientBase& operator=(AwsSmithyClientBase& target) = delete;
122-
AwsSmithyClientBase(AwsSmithyClientBase&& target) = default;
123-
AwsSmithyClientBase& operator=(AwsSmithyClientBase&& target) = default;
127+
AwsSmithyClientBase(AwsSmithyClientBase&& target) = delete;
128+
AwsSmithyClientBase& operator=(AwsSmithyClientBase&& target) = delete;
124129

125130
virtual ~AwsSmithyClientBase() = default;
126131

@@ -144,19 +149,6 @@ namespace client
144149
void AppendToUserAgent(const Aws::String& valueToAppend);
145150

146151
protected:
147-
void deepCopy(Aws::UniquePtr<Aws::Client::ClientConfiguration>&& clientConfig,
148-
const Aws::String& serviceName,
149-
std::shared_ptr<Aws::Http::HttpClient> httpClient,
150-
std::shared_ptr<Aws::Client::AWSErrorMarshaller> errorMarshaller)
151-
{
152-
m_clientConfig = std::move(clientConfig);
153-
m_serviceName = serviceName;
154-
m_httpClient = std::move(httpClient);
155-
m_errorMarshaller = std::move(errorMarshaller);
156-
m_interceptors = Aws::Vector<std::shared_ptr<interceptor::Interceptor>>{Aws::MakeShared<ChecksumInterceptor>("AwsSmithyClientBase")};
157-
baseCopyInit();
158-
}
159-
160152
/**
161153
* Initialize client configuration with their factory method, unless the user has explicitly set the
162154
* configuration, and it is to be shallow copied between different clients, in which case, delete the
@@ -170,6 +162,18 @@ namespace client
170162
*/
171163
void baseCopyInit();
172164

165+
/**
166+
* A helper utility to re-initialize on copy assignment
167+
*/
168+
void baseCopyAssign(const AwsSmithyClientBase& other,
169+
std::shared_ptr<Aws::Http::HttpClient> httpClient,
170+
std::shared_ptr<Aws::Client::AWSErrorMarshaller> errorMarshaller);
171+
172+
/**
173+
* A helper utility to move assign base client
174+
*/
175+
void baseMoveAssign(AwsSmithyClientBase&& other);
176+
173177
/**
174178
* Transforms the AmazonWebServicesResult object into an HttpRequest.
175179
*/
@@ -191,7 +195,10 @@ namespace client
191195
virtual SigningOutcome SignHttpRequest(std::shared_ptr<HttpRequest> httpRequest, const AuthSchemeOption& targetAuthSchemeOption) const = 0;
192196
virtual bool AdjustClockSkew(HttpResponseOutcome& outcome, const AuthSchemeOption& authSchemeOption) const = 0;
193197

194-
std::shared_ptr<Aws::Client::ClientConfiguration> m_clientConfig;
198+
/* AwsSmithyClientT class binds its config reference to this pointer, so don't remove const and don't re-allocate it.
199+
* This is done to avoid duplication of config object between this base and actual service template classes.
200+
*/
201+
const Aws::UniquePtr<Aws::Client::ClientConfiguration> m_clientConfig;
195202
Aws::String m_serviceName;
196203
Aws::String m_serviceUserAgentName;
197204

‎src/aws-cpp-sdk-core/source/smithy/client/AwsSmithyClientBase.cpp

+36-11
Original file line numberDiff line numberDiff line change
@@ -7,20 +7,22 @@
77
#include <smithy/client/AwsSmithyClientAsyncRequestContext.h>
88
#include <smithy/client/features/RecursionDetection.h>
99
#include <smithy/client/features/RequestPayloadCompression.h>
10+
#include <smithy/identity/signer/built-in/SignerProperties.h>
11+
#include <smithy/tracing/TracingUtils.h>
1012

11-
#include "aws/core/client/AWSErrorMarshaller.h"
12-
#include "aws/core/client/RetryStrategy.h"
13-
#include "aws/core/http/HttpClientFactory.h"
14-
#include "aws/core/monitoring/CoreMetrics.h"
15-
#include "aws/core/monitoring/MonitoringManager.h"
16-
#include "aws/core/utils/DNS.h"
17-
#include "aws/core/utils/threading/Executor.h"
18-
#include "aws/core/utils/threading/SameThreadExecutor.h"
19-
#include "smithy/tracing/TracingUtils.h"
13+
#include <aws/core/client/AWSErrorMarshaller.h>
14+
#include <aws/core/client/CoreErrors.h>
15+
#include <aws/core/client/RetryStrategy.h>
16+
#include <aws/core/http/HttpClientFactory.h>
17+
#include <aws/core/monitoring/CoreMetrics.h>
18+
#include <aws/core/monitoring/MonitoringManager.h>
19+
#include <aws/core/utils/DNS.h>
20+
#include <aws/core/utils/logging/ErrorMacros.h>
2021
#include <aws/core/utils/stream/ResponseStream.h>
22+
#include <aws/core/utils/threading/Executor.h>
23+
#include <aws/core/utils/threading/SameThreadExecutor.h>
2124
#include <aws/crt/Variant.h>
22-
#include <aws/core/client/CoreErrors.h>
23-
#include <smithy/identity/signer/built-in/SignerProperties.h>
25+
2426

2527
using namespace smithy::client;
2628
using namespace smithy::interceptor;
@@ -62,6 +64,7 @@ void createFromFactoriesIfPresent(T& entity, std::function<T()>& factory) {
6264
} // namespace
6365

6466
void AwsSmithyClientBase::baseInit() {
67+
AWS_CHECK_PTR(AWS_SMITHY_CLIENT_LOG, m_clientConfig);
6568
createFromFactories(m_clientConfig->retryStrategy, m_clientConfig->configFactories.retryStrategyCreateFn);
6669
createFromFactories(m_clientConfig->executor, m_clientConfig->configFactories.executorCreateFn);
6770
createFromFactories(m_clientConfig->writeRateLimiter, m_clientConfig->configFactories.writeRateLimiterCreateFn);
@@ -77,6 +80,7 @@ void AwsSmithyClientBase::baseInit() {
7780
}
7881

7982
void AwsSmithyClientBase::baseCopyInit() {
83+
AWS_CHECK_PTR(AWS_SMITHY_CLIENT_LOG, m_clientConfig);
8084
createFromFactoriesIfPresent(m_clientConfig->retryStrategy, m_clientConfig->configFactories.retryStrategyCreateFn);
8185
createFromFactoriesIfPresent(m_clientConfig->executor, m_clientConfig->configFactories.executorCreateFn);
8286
createFromFactoriesIfPresent(m_clientConfig->writeRateLimiter, m_clientConfig->configFactories.writeRateLimiterCreateFn);
@@ -91,6 +95,27 @@ void AwsSmithyClientBase::baseCopyInit() {
9195
}
9296
}
9397

98+
void AwsSmithyClientBase::baseCopyAssign(const AwsSmithyClientBase& other,
99+
std::shared_ptr<Aws::Http::HttpClient> httpClient,
100+
std::shared_ptr<Aws::Client::AWSErrorMarshaller> errorMarshaller) {
101+
m_serviceName = other.m_serviceName;
102+
m_serviceUserAgentName = other.m_serviceUserAgentName;
103+
m_httpClient = std::move(httpClient);
104+
m_errorMarshaller = std::move(errorMarshaller);
105+
m_interceptors = Aws::Vector<std::shared_ptr<interceptor::Interceptor>>{Aws::MakeShared<ChecksumInterceptor>("AwsSmithyClientBase")};
106+
107+
baseCopyInit();
108+
}
109+
110+
void AwsSmithyClientBase::baseMoveAssign(AwsSmithyClientBase&& other) {
111+
m_serviceName = std::move(other.m_serviceName);
112+
m_serviceUserAgentName = std::move(other.m_serviceUserAgentName);
113+
m_httpClient = std::move(other.m_httpClient);
114+
m_errorMarshaller = std::move(other.m_errorMarshaller);
115+
m_interceptors = std::move(other.m_interceptors);
116+
m_userAgentInterceptor = std::move(other.m_userAgentInterceptor);
117+
}
118+
94119
std::shared_ptr<Aws::Http::HttpRequest>
95120
AwsSmithyClientBase::BuildHttpRequest(const std::shared_ptr<AwsSmithyClientAsyncRequestContext>& pRequestCtx,
96121
const Aws::Http::URI& uri,

0 commit comments

Comments
 (0)