Skip to content

Commit 11c124f

Browse files
SergeyRyabininSergeyRyabinin
committed
#3084 use factories for client utility classes initialization, improve shutdown behavior
1 parent 9514d1a commit 11c124f

File tree

22 files changed

+195
-122
lines changed

22 files changed

+195
-122
lines changed

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

Lines changed: 14 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -97,7 +97,7 @@ namespace Client
9797
std::unique_lock<std::mutex> lock(pClient->m_shutdownMutex);
9898

9999
pClient->m_isInitialized = false;
100-
100+
pClient->DisableRequestProcessing();
101101

102102
if (timeoutMs == -1)
103103
{
@@ -107,10 +107,15 @@ namespace Client
107107
std::chrono::milliseconds(timeoutMs),
108108
[&](){ return pClient->m_operationsProcessed.load() == 0; });
109109

110-
pClient->m_endpointProvider.reset();
111-
pClient->m_executor.reset();
110+
if (pClient->m_operationsProcessed.load())
111+
{
112+
AWS_LOGSTREAM_FATAL(AwsServiceClientT::GetAllocationTag(), "Service client "
113+
<< AwsServiceClientT::GetServiceName() << " is shutting down while async tasks are present.");
114+
}
115+
112116
pClient->m_clientConfiguration.executor.reset();
113117
pClient->m_clientConfiguration.retryStrategy.reset();
118+
pClient->m_endpointProvider.reset();
114119
}
115120

116121
/**
@@ -124,7 +129,7 @@ namespace Client
124129
const std::shared_ptr<const Aws::Client::AsyncCallerContext>& context = nullptr) const
125130
{
126131
const AwsServiceClientT* clientThis = static_cast<const AwsServiceClientT*>(this);
127-
Aws::Client::MakeAsyncOperation(operationFunc, clientThis, request, handler, context, clientThis->m_executor.get());
132+
Aws::Client::MakeAsyncOperation(operationFunc, clientThis, request, handler, context, clientThis->m_clientConfiguration.executor.get());
128133
}
129134

130135
/**
@@ -139,7 +144,7 @@ namespace Client
139144
const std::shared_ptr<const Aws::Client::AsyncCallerContext>& context = nullptr) const
140145
{
141146
const AwsServiceClientT* clientThis = static_cast<const AwsServiceClientT*>(this);
142-
Aws::Client::MakeAsyncStreamingOperation(operationFunc, clientThis, request, handler, context, clientThis->m_executor.get());
147+
Aws::Client::MakeAsyncStreamingOperation(operationFunc, clientThis, request, handler, context, clientThis->m_clientConfiguration.executor.get());
143148
}
144149

145150
/**
@@ -152,7 +157,7 @@ namespace Client
152157
const std::shared_ptr<const Aws::Client::AsyncCallerContext>& context = nullptr) const
153158
{
154159
const AwsServiceClientT* clientThis = static_cast<const AwsServiceClientT*>(this);
155-
Aws::Client::MakeAsyncOperation(operationFunc, clientThis, handler, context, clientThis->m_executor.get());
160+
Aws::Client::MakeAsyncOperation(operationFunc, clientThis, handler, context, clientThis->m_clientConfiguration.executor.get());
156161
}
157162

158163
/**
@@ -165,7 +170,7 @@ namespace Client
165170
-> std::future<decltype((static_cast<const AwsServiceClientT*>(nullptr)->*operationFunc)(request))>
166171
{
167172
const AwsServiceClientT* clientThis = static_cast<const AwsServiceClientT*>(this);
168-
return Aws::Client::MakeCallableOperation(AwsServiceClientT::GetAllocationTag(), operationFunc, clientThis, request, clientThis->m_executor.get());
173+
return Aws::Client::MakeCallableOperation(AwsServiceClientT::GetAllocationTag(), operationFunc, clientThis, request, clientThis->m_clientConfiguration.executor.get());
169174
}
170175

171176
/**
@@ -178,7 +183,7 @@ namespace Client
178183
-> std::future<decltype((static_cast<const AwsServiceClientT*>(nullptr)->*operationFunc)(request))>
179184
{
180185
const AwsServiceClientT* clientThis = static_cast<const AwsServiceClientT*>(this);
181-
return Aws::Client::MakeCallableStreamingOperation(AwsServiceClientT::GetAllocationTag(), operationFunc, clientThis, request, clientThis->m_executor.get());
186+
return Aws::Client::MakeCallableStreamingOperation(AwsServiceClientT::GetAllocationTag(), operationFunc, clientThis, request, clientThis->m_clientConfiguration.executor.get());
182187
}
183188

184189
/**
@@ -191,7 +196,7 @@ namespace Client
191196
-> std::future<decltype((static_cast<const AwsServiceClientT*>(nullptr)->*operationFunc)())>
192197
{
193198
const AwsServiceClientT* clientThis = static_cast<const AwsServiceClientT*>(this);
194-
return Aws::Client::MakeCallableOperation(AwsServiceClientT::GetAllocationTag(), operationFunc, clientThis, clientThis->m_executor.get());
199+
return Aws::Client::MakeCallableOperation(AwsServiceClientT::GetAllocationTag(), operationFunc, clientThis, clientThis->m_clientConfiguration.executor.get());
195200
}
196201
protected:
197202
std::atomic<bool> m_isInitialized;

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

Lines changed: 55 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,6 @@
1414
#include <aws/core/utils/Array.h>
1515
#include <aws/crt/Optional.h>
1616
#include <smithy/tracing/TelemetryProvider.h>
17-
#include <smithy/tracing/NoopTelemetryProvider.h>
1817
#include <memory>
1918

2019
namespace Aws
@@ -69,12 +68,48 @@ namespace Aws
6968
bool shouldDisableIMDS = false;
7069
};
7170

71+
/**
72+
* Utility struct to hold default factories used in ClientConfiguration.
73+
*/
74+
struct AWS_CORE_API ClientConfigDefaultFactories
75+
{
76+
static std::shared_ptr<Aws::Utils::Threading::Executor> CreateExecutor();
77+
};
78+
7279
/**
7380
* This mutable structure is used to configure any of the AWS clients.
7481
* Default values can only be overwritten prior to passing to the client constructors.
7582
*/
7683
struct AWS_CORE_API ClientConfiguration
7784
{
85+
struct ProviderFactories
86+
{
87+
/**
88+
* Retry Strategy factory method. Default is DefaultRetryStrategy (i.e. exponential backoff).
89+
*/
90+
std::function<std::shared_ptr<RetryStrategy>()> retryStrategyCreateFn;
91+
/**
92+
* Threading Executor factory method. Default creates a factory that creates DefaultExecutor
93+
* (i.e. spawn a separate thread for each task) for backward compatibility reasons.
94+
* Please switch to a better executor such as PooledThreadExecutor.
95+
*/
96+
std::function<std::shared_ptr<Utils::Threading::Executor>()> executorCreateFn;
97+
/**
98+
* Rate Limiter factory for outgoing bandwidth. Default is wide-open.
99+
*/
100+
std::function<std::shared_ptr<Utils::RateLimits::RateLimiterInterface>()> writeRateLimiterCreateFn;
101+
/**
102+
* Rate Limiter factory for incoming bandwidth. Default is wide-open.
103+
*/
104+
std::function<std::shared_ptr<Utils::RateLimits::RateLimiterInterface>()> readRateLimiterCreateFn;
105+
/**
106+
* TelemetryProvider factory. Defaults to Noop provider.
107+
*/
108+
std::function<std::shared_ptr<smithy::components::tracing::TelemetryProvider>()> telemetryProviderCreateFn;
109+
110+
static ProviderFactories defaultFactories;
111+
};
112+
78113
ClientConfiguration();
79114

80115
/**
@@ -104,6 +139,11 @@ namespace Aws
104139
*/
105140
virtual ~ClientConfiguration() = default;
106141

142+
/**
143+
* Client configuration factory methods to init client utility classes such as Executor, Retry Strategy
144+
*/
145+
ProviderFactories configFactories = ProviderFactories::defaultFactories;
146+
107147
/**
108148
* User Agent string user for http calls. This is filled in for you in the constructor. Don't override this unless you have a really good reason.
109149
*/
@@ -165,9 +205,10 @@ namespace Aws
165205
*/
166206
unsigned long lowSpeedLimit = 1;
167207
/**
168-
* Strategy to use in case of failed requests. Default is DefaultRetryStrategy (i.e. exponential backoff)
208+
* Strategy to use in case of failed requests. Default is DefaultRetryStrategy (i.e. exponential backoff).
209+
* Provide retry strategy here or via a factory method.
169210
*/
170-
std::shared_ptr<RetryStrategy> retryStrategy;
211+
std::shared_ptr<RetryStrategy> retryStrategy = nullptr;
171212
/**
172213
* Override the http endpoint used to talk to a service.
173214
*/
@@ -227,9 +268,10 @@ namespace Aws
227268
*/
228269
Aws::Utils::Array<Aws::String> nonProxyHosts;
229270
/**
230-
* Threading Executor implementation. Default uses std::thread::detach()
231-
*/
232-
std::shared_ptr<Aws::Utils::Threading::Executor> executor;
271+
* Threading Executor implementation. Default uses std::thread::detach()
272+
* Provide executor here or via a factory method.
273+
*/
274+
std::shared_ptr<Aws::Utils::Threading::Executor> executor = nullptr;
233275
/**
234276
* If you need to test and want to get around TLS validation errors, do that here.
235277
* You probably shouldn't use this flag in a production scenario.
@@ -263,12 +305,14 @@ namespace Aws
263305
Aws::String proxyCaFile;
264306
/**
265307
* Rate Limiter implementation for outgoing bandwidth. Default is wide-open.
308+
* Provide limiter here or via a factory method.
266309
*/
267-
std::shared_ptr<Aws::Utils::RateLimits::RateLimiterInterface> writeRateLimiter;
310+
std::shared_ptr<Aws::Utils::RateLimits::RateLimiterInterface> writeRateLimiter = nullptr;
268311
/**
269312
* Rate Limiter implementation for incoming bandwidth. Default is wide-open.
313+
* Provide limiter here or via a factory method.
270314
*/
271-
std::shared_ptr<Aws::Utils::RateLimits::RateLimiterInterface> readRateLimiter;
315+
std::shared_ptr<Aws::Utils::RateLimits::RateLimiterInterface> readRateLimiter = nullptr;
272316
/**
273317
* Override the http implementation the default factory returns.
274318
*/
@@ -374,10 +418,10 @@ namespace Aws
374418
const Aws::String& defaultValue);
375419

376420
/**
377-
* A wrapper for interfacing with telemetry functionality.
421+
* A wrapper for interfacing with telemetry functionality. Defaults to Noop provider.
422+
* Provide TelemetryProvider here or via a factory method.
378423
*/
379-
std::shared_ptr<smithy::components::tracing::TelemetryProvider> telemetryProvider =
380-
smithy::components::tracing::NoopTelemetryProvider::CreateProvider();
424+
std::shared_ptr<smithy::components::tracing::TelemetryProvider> telemetryProvider;
381425
};
382426

383427
/**

‎src/aws-cpp-sdk-core/include/aws/core/utils/logging/ErrorMacros.h

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -66,3 +66,10 @@ if(!m_isInitialized) \
6666
} \
6767
Aws::Utils::RAIICounter(this->m_operationsProcessed, &this->m_shutdownSignal)
6868

69+
#define AWS_ASYNC_OPERATION_GUARD(OPERATION) \
70+
if(!m_isInitialized) \
71+
{ \
72+
AWS_LOGSTREAM_ERROR(#OPERATION, "Unable to call " #OPERATION ": client is not initialized (or already terminated)"); \
73+
return handler(this, request, Aws::Client::AWSError<CoreErrors>(CoreErrors::NOT_INITIALIZED, "NOT_INITIALIZED", "Client is not initialized or already terminated", false), handlerContext); \
74+
} \
75+
Aws::Utils::RAIICounter(this->m_operationsProcessed, &this->m_shutdownSignal)

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

Lines changed: 22 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -119,13 +119,19 @@ AWSClient::AWSClient(const Aws::Client::ClientConfiguration& configuration,
119119
const std::shared_ptr<Aws::Client::AWSAuthSigner>& signer,
120120
const std::shared_ptr<AWSErrorMarshaller>& errorMarshaller) :
121121
m_region(configuration.region),
122-
m_telemetryProvider(configuration.telemetryProvider),
122+
m_telemetryProvider(configuration.telemetryProvider ? configuration.telemetryProvider : configuration.configFactories.telemetryProviderCreateFn()),
123123
m_signerProvider(Aws::MakeUnique<Aws::Auth::DefaultAuthSignerProvider>(AWS_CLIENT_LOG_TAG, signer)),
124-
m_httpClient(CreateHttpClient(configuration)),
124+
m_httpClient(CreateHttpClient(
125+
[&configuration, this]()
126+
{
127+
ClientConfiguration tempConfig(configuration);
128+
tempConfig.telemetryProvider = m_telemetryProvider;
129+
return tempConfig;
130+
}())),
125131
m_errorMarshaller(errorMarshaller),
126-
m_retryStrategy(configuration.retryStrategy),
127-
m_writeRateLimiter(configuration.writeRateLimiter),
128-
m_readRateLimiter(configuration.readRateLimiter),
132+
m_retryStrategy(configuration.retryStrategy ? configuration.retryStrategy : configuration.configFactories.retryStrategyCreateFn()),
133+
m_writeRateLimiter(configuration.writeRateLimiter ? configuration.writeRateLimiter : configuration.configFactories.writeRateLimiterCreateFn()),
134+
m_readRateLimiter(configuration.readRateLimiter ? configuration.readRateLimiter : configuration.configFactories.readRateLimiterCreateFn()),
129135
m_userAgent(Aws::Client::ComputeUserAgentString(&configuration)),
130136
m_hash(Aws::Utils::Crypto::CreateMD5Implementation()),
131137
m_requestTimeoutMs(configuration.requestTimeoutMs),
@@ -138,13 +144,19 @@ AWSClient::AWSClient(const Aws::Client::ClientConfiguration& configuration,
138144
const std::shared_ptr<Aws::Auth::AWSAuthSignerProvider>& signerProvider,
139145
const std::shared_ptr<AWSErrorMarshaller>& errorMarshaller) :
140146
m_region(configuration.region),
141-
m_telemetryProvider(configuration.telemetryProvider),
147+
m_telemetryProvider(configuration.telemetryProvider ? configuration.telemetryProvider : configuration.configFactories.telemetryProviderCreateFn()),
142148
m_signerProvider(signerProvider),
143-
m_httpClient(CreateHttpClient(configuration)),
149+
m_httpClient(CreateHttpClient(
150+
[&configuration, this]()
151+
{
152+
ClientConfiguration tempConfig(configuration);
153+
tempConfig.telemetryProvider = m_telemetryProvider;
154+
return tempConfig;
155+
}())),
144156
m_errorMarshaller(errorMarshaller),
145-
m_retryStrategy(configuration.retryStrategy),
146-
m_writeRateLimiter(configuration.writeRateLimiter),
147-
m_readRateLimiter(configuration.readRateLimiter),
157+
m_retryStrategy(configuration.retryStrategy ? configuration.retryStrategy : configuration.configFactories.retryStrategyCreateFn()),
158+
m_writeRateLimiter(configuration.writeRateLimiter ? configuration.writeRateLimiter : configuration.configFactories.writeRateLimiterCreateFn()),
159+
m_readRateLimiter(configuration.readRateLimiter ? configuration.readRateLimiter : configuration.configFactories.readRateLimiterCreateFn()),
148160
m_userAgent(Aws::Client::ComputeUserAgentString(&configuration)),
149161
m_hash(Aws::Utils::Crypto::CreateMD5Implementation()),
150162
m_requestTimeoutMs(configuration.requestTimeoutMs),

‎src/aws-cpp-sdk-core/source/client/ClientConfiguration.cpp

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717
#include <aws/core/Version.h>
1818
#include <aws/core/config/AWSProfileConfigLoader.h>
1919
#include <aws/core/utils/logging/LogMacros.h>
20+
#include <smithy/tracing/NoopTelemetryProvider.h>
2021

2122
#include <aws/crt/Config.h>
2223

@@ -39,6 +40,19 @@ static const char* AWS_EXECUTION_ENV = "AWS_EXECUTION_ENV";
3940
static const char* DISABLE_IMDSV1_CONFIG_VAR = "AWS_EC2_METADATA_V1_DISABLED";
4041
static const char* DISABLE_IMDSV1_ENV_VAR = "ec2_metadata_v1_disabled";
4142

43+
ClientConfiguration::ProviderFactories ClientConfiguration::ProviderFactories::defaultFactories = []()
44+
{
45+
ProviderFactories factories;
46+
47+
factories.retryStrategyCreateFn = [](){return InitRetryStrategy();};
48+
factories.executorCreateFn = [](){return Aws::MakeShared<Aws::Utils::Threading::DefaultExecutor>(CLIENT_CONFIG_TAG);};
49+
factories.writeRateLimiterCreateFn = [](){return nullptr;};
50+
factories.readRateLimiterCreateFn = [](){return nullptr;};
51+
factories.telemetryProviderCreateFn = [](){return smithy::components::tracing::NoopTelemetryProvider::CreateProvider();};
52+
53+
return factories;
54+
}();
55+
4256
Aws::String FilterUserAgentToken(char const * const source)
4357
{
4458
// Tokens are short textual identifiers that do not include whitespace or delimiters.
@@ -131,7 +145,6 @@ void setLegacyClientConfigurationParameters(ClientConfiguration& clientConfig)
131145
clientConfig.lowSpeedLimit = 1;
132146
clientConfig.proxyScheme = Aws::Http::Scheme::HTTP;
133147
clientConfig.proxyPort = 0;
134-
clientConfig.executor = Aws::MakeShared<Aws::Utils::Threading::DefaultExecutor>(CLIENT_CONFIG_TAG);
135148
clientConfig.verifySSL = true;
136149
clientConfig.writeRateLimiter = nullptr;
137150
clientConfig.readRateLimiter = nullptr;

‎src/aws-cpp-sdk-transfer/include/aws/transfer/TransferManager.h

Lines changed: 13 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -39,10 +39,7 @@ namespace Aws
3939
*/
4040
struct TransferManagerConfiguration
4141
{
42-
TransferManagerConfiguration(Aws::Utils::Threading::Executor* executor) : s3Client(nullptr), transferExecutor(executor), computeContentMD5(false), transferBufferMaxHeapSize(10 * MB5), bufferSize(MB5)
43-
{
44-
}
45-
42+
TransferManagerConfiguration(Aws::Utils::Threading::Executor* executor);
4643
/**
4744
* S3 Client to use for transfers. You are responsible for setting this.
4845
*/
@@ -53,6 +50,18 @@ namespace Aws
5350
* It is not a bug to use the same executor, but at least be aware that this is how the manager will be used.
5451
*/
5552
Aws::Utils::Threading::Executor* transferExecutor = nullptr;
53+
54+
/**
55+
* Threading Executor shared pointer.
56+
* Created and owned by Transfer manager if no raw pointer `transferExecutor` is provided.
57+
*/
58+
std::shared_ptr<Aws::Utils::Threading::Executor> spExecutor = nullptr;
59+
60+
/**
61+
* Threading Executor factory method. Default creates a factory that creates DefaultExecutor
62+
*/
63+
std::function<std::shared_ptr<Utils::Threading::Executor>()> executorCreateFn;
64+
5665
/**
5766
* When true, TransferManager will calculate the MD5 digest of the content being uploaded.
5867
* The digest is sent to S3 via an HTTP header enabling the service to perform integrity checks.

‎src/aws-cpp-sdk-transfer/source/transfer/TransferManager.cpp

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,15 @@ namespace Aws
2525
{
2626
namespace Transfer
2727
{
28+
TransferManagerConfiguration::TransferManagerConfiguration(Aws::Utils::Threading::Executor* executor)
29+
: s3Client(nullptr),
30+
transferExecutor(executor),
31+
computeContentMD5(false),
32+
transferBufferMaxHeapSize(10 * MB5),
33+
bufferSize(MB5)
34+
{
35+
}
36+
2837
static inline bool IsS3KeyPrefix(const Aws::String& path)
2938
{
3039
return (path.find_last_of('/') == path.size() - 1 || path.find_last_of('\\') == path.size() - 1);
@@ -57,6 +66,18 @@ namespace Aws
5766
TransferManager::TransferManager(const TransferManagerConfiguration& configuration) : m_transferConfig(configuration)
5867
{
5968
assert(m_transferConfig.s3Client);
69+
if (!m_transferConfig.transferExecutor)
70+
{
71+
if(!m_transferConfig.spExecutor && m_transferConfig.executorCreateFn)
72+
{
73+
m_transferConfig.spExecutor = m_transferConfig.executorCreateFn();
74+
}
75+
m_transferConfig.transferExecutor = m_transferConfig.spExecutor.get();
76+
}
77+
if (!m_transferConfig.transferExecutor)
78+
{
79+
AWS_LOGSTREAM_FATAL(CLASS_TAG, "Failed to init TransferManager: transferExecutor is null");
80+
}
6081
assert(m_transferConfig.transferExecutor);
6182
m_transferConfig.s3Client->AppendToUserAgent("ft/s3-transfer");
6283
for (uint64_t i = 0; i < m_transferConfig.transferBufferMaxHeapSize; i += m_transferConfig.bufferSize)

0 commit comments

Comments
 (0)