Skip to content

Commit 65bf755

Browse files
committedMar 11, 2024
Remove multi-thread synchronization from CRT logger wrapper
1 parent cdc553f commit 65bf755

File tree

10 files changed

+163
-192
lines changed

10 files changed

+163
-192
lines changed
 

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

Lines changed: 3 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -24,34 +24,25 @@ namespace Aws
2424
{
2525
public:
2626
DefaultCRTLogSystem(LogLevel logLevel);
27-
virtual ~DefaultCRTLogSystem();
27+
virtual ~DefaultCRTLogSystem() = default;
2828

2929
DefaultCRTLogSystem(DefaultCRTLogSystem&&) = delete;
3030
DefaultCRTLogSystem(const DefaultCRTLogSystem&) = delete;
3131

3232
/**
3333
* Gets the currently configured log level.
3434
*/
35-
LogLevel GetLogLevel() const override { return m_logLevel; }
35+
LogLevel GetLogLevel() const override;
3636
/**
3737
* Set a new log level. This has the immediate effect of changing the log output to the new level.
3838
*/
39-
void SetLogLevel(LogLevel logLevel) override { m_logLevel.store(logLevel); }
39+
void SetLogLevel(LogLevel logLevel) override;
4040

4141
/**
4242
* Handle the logging information from common runtime libraries.
4343
* Redirect them to C++ SDK logging system by default.
4444
*/
4545
virtual void Log(LogLevel logLevel, const char* subjectName, const char* formatStr, va_list args) override;
46-
47-
protected:
48-
std::atomic<LogLevel> m_logLevel;
49-
50-
// destruction synchronization
51-
mutable std::atomic<bool> m_stopLogging;
52-
mutable std::atomic<size_t> m_logsProcessed;
53-
mutable std::condition_variable m_stopSignal;
54-
mutable std::mutex m_stopMutex;
5546
};
5647

5748
} // namespace Logging

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

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,11 @@ namespace Aws
4545
*/
4646
virtual void Log(LogLevel logLevel, const char* tag, const char* formatStr, ...) override;
4747

48+
/**
49+
* Does a printf style output to ProcessFormattedStatement. Don't use this, it's unsafe. See LogStream
50+
*/
51+
virtual void vaLog(LogLevel logLevel, const char* tag, const char* formatStr, va_list args) override;
52+
4853
/**
4954
* Writes the stream to ProcessFormattedStatement.
5055
*/

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

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,10 @@ namespace Aws
3636
* Does a printf style output to the output stream. Don't use this, it's unsafe. See LogStream
3737
*/
3838
virtual void Log(LogLevel logLevel, const char* tag, const char* formatStr, ...) = 0;
39+
/**
40+
* va_list overload for Log, avoid using this as well.
41+
*/
42+
virtual void vaLog(LogLevel logLevel, const char* tag, const char* formatStr, va_list args) = 0;
3943
/**
4044
* Writes the stream to the output stream.
4145
*/

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

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,13 @@ namespace Aws
3636
AWS_UNREFERENCED_PARAM(formatStr);
3737
}
3838

39+
virtual void vaLog(LogLevel logLevel, const char* tag, const char* formatStr, va_list args) override
40+
{
41+
AWS_UNREFERENCED_PARAM(logLevel);
42+
AWS_UNREFERENCED_PARAM(tag);
43+
AWS_UNREFERENCED_PARAM(formatStr);
44+
}
45+
3946
virtual void LogStream(LogLevel logLevel, const char* tag, const Aws::OStringStream &messageStream) override
4047
{
4148
AWS_UNREFERENCED_PARAM(logLevel);

‎src/aws-cpp-sdk-core/source/Globals.cpp

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77
#include <aws/crt/io/TlsOptions.h>
88
#include <aws/crt/io/Bootstrap.h>
99
#include <aws/core/Globals.h>
10+
#include <aws/core/utils/logging/LogMacros.h>
1011
#include <aws/core/utils/EnumParseOverflowContainer.h>
1112
#include <aws/core/utils/memory/AWSMemory.h>
1213
#include <aws/auth/auth.h>
@@ -47,6 +48,10 @@ namespace Aws
4748
void InitializeCrt()
4849
{
4950
g_apiHandle = Aws::New<Aws::Crt::ApiHandle>(TAG, Aws::get_aws_allocator());
51+
AWS_FATAL_ASSERT(g_apiHandle);
52+
auto crtVersion = g_apiHandle->GetCrtVersion();
53+
AWS_LOGSTREAM_INFO(TAG, "Initialized AWS-CRT-CPP with version "
54+
<< crtVersion.major << "." << crtVersion.minor << "." << crtVersion.patch);
5055
}
5156

5257
void CleanupCrt()
Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,27 @@
1+
/**
2+
* Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
3+
* SPDX-License-Identifier: Apache-2.0.
4+
*/
5+
6+
#include <aws/core/utils/logging/CRTLogSystem.h>
7+
8+
using namespace Aws::Utils;
9+
using namespace Aws::Utils::Logging;
10+
11+
namespace Aws
12+
{
13+
namespace Utils
14+
{
15+
namespace Logging
16+
{
17+
static_assert(Aws::Utils::Logging::LogLevel::Off == static_cast<LogLevel>(aws_log_level::AWS_LL_NONE), "Different underlying values between SDK and CRT log level enums");
18+
static_assert(Aws::Utils::Logging::LogLevel::Fatal == static_cast<LogLevel>(aws_log_level::AWS_LL_FATAL), "Different underlying values between SDK and CRT log level enums");
19+
static_assert(Aws::Utils::Logging::LogLevel::Error == static_cast<LogLevel>(aws_log_level::AWS_LL_ERROR), "Different underlying values between SDK and CRT log level enums");
20+
static_assert(Aws::Utils::Logging::LogLevel::Warn == static_cast<LogLevel>(aws_log_level::AWS_LL_WARN), "Different underlying values between SDK and CRT log level enums");
21+
static_assert(Aws::Utils::Logging::LogLevel::Info == static_cast<LogLevel>(aws_log_level::AWS_LL_INFO), "Different underlying values between SDK and CRT log level enums");
22+
static_assert(Aws::Utils::Logging::LogLevel::Debug == static_cast<LogLevel>(aws_log_level::AWS_LL_DEBUG), "Different underlying values between SDK and CRT log level enums");
23+
static_assert(Aws::Utils::Logging::LogLevel::Trace == static_cast<LogLevel>(aws_log_level::AWS_LL_TRACE), "Different underlying values between SDK and CRT log level enums");
24+
}
25+
}
26+
}
27+

‎src/aws-cpp-sdk-core/source/utils/logging/CRTLogging.cpp

Lines changed: 28 additions & 73 deletions
Original file line numberDiff line numberDiff line change
@@ -24,11 +24,7 @@ namespace Utils
2424
namespace Logging {
2525

2626
static std::shared_ptr<CRTLogSystemInterface> CRTLogSystem(nullptr);
27-
static Aws::Utils::Threading::ReaderWriterLock CRTLogSystemLock;
28-
29-
static bool s_CRTLogsRedirectionIsSet(false);
3027
static aws_logger s_sdkCrtLogger;
31-
static aws_logger s_previousCrtLogger;
3228

3329
static int s_aws_logger_redirect_log(
3430
struct aws_logger *logger,
@@ -37,84 +33,53 @@ static int s_aws_logger_redirect_log(
3733
const char *format, ...)
3834
{
3935
AWS_UNREFERENCED_PARAM(logger);
40-
Aws::Utils::Threading::ReaderLockGuard guard(CRTLogSystemLock);
41-
if (CRTLogSystem)
42-
{
43-
assert(logger->p_impl == &s_sdkCrtLogger);
44-
Aws::Utils::Logging::LogLevel logLevel = static_cast<LogLevel>(log_level);
45-
const char* subjectName = aws_log_subject_name(subject);
46-
va_list args;
47-
va_start(args, format);
48-
CRTLogSystem->Log(logLevel, subjectName, format, args);
49-
va_end(args);
50-
return AWS_OP_SUCCESS;
51-
}
52-
else if (s_CRTLogsRedirectionIsSet)
36+
if (!CRTLogSystem)
5337
{
54-
// SDK CRT logger is terminated, fallback to an original/previous CRT logger
55-
assert(s_previousCrtLogger.vtable->log);
56-
va_list args;
57-
va_start(args, format);
58-
auto ret = s_previousCrtLogger.vtable->log(&s_previousCrtLogger, log_level, subject, format, args);
59-
va_end(args);
60-
return ret;
38+
return AWS_OP_ERR;
6139
}
62-
assert(!"Unreachable state: this method is called without redirection set");
63-
return AWS_OP_ERR;
40+
assert(logger->p_impl == &s_sdkCrtLogger);
41+
Aws::Utils::Logging::LogLevel logLevel = static_cast<LogLevel>(log_level);
42+
const char* subjectName = aws_log_subject_name(subject);
43+
va_list args;
44+
va_start(args, format);
45+
CRTLogSystem->Log(logLevel, subjectName, format, args);
46+
va_end(args);
47+
return AWS_OP_SUCCESS;
6448
}
6549

6650
static enum aws_log_level s_aws_logger_redirect_get_log_level(struct aws_logger *logger, aws_log_subject_t subject)
6751
{
6852
AWS_UNREFERENCED_PARAM(logger);
69-
Aws::Utils::Threading::ReaderLockGuard guard(CRTLogSystemLock);
53+
AWS_UNREFERENCED_PARAM(subject);
7054
if (CRTLogSystem)
7155
{
7256
assert(logger->p_impl == &s_sdkCrtLogger);
7357
return (aws_log_level) (CRTLogSystem->GetLogLevel());
7458
}
75-
else if (s_CRTLogsRedirectionIsSet)
76-
{
77-
assert(s_previousCrtLogger.vtable->get_log_level);
78-
return s_previousCrtLogger.vtable->get_log_level(&s_previousCrtLogger, subject);
79-
}
80-
assert(!"Unreachable state: this method is called without redirection set");
8159
return AWS_LL_NONE;
8260
}
8361

8462
static void s_aws_logger_redirect_clean_up(struct aws_logger *logger)
8563
{
8664
AWS_UNREFERENCED_PARAM(logger);
87-
Aws::Utils::Threading::ReaderLockGuard guard(CRTLogSystemLock);
8865
if (CRTLogSystem)
8966
{
9067
assert(logger->p_impl == &s_sdkCrtLogger);
9168
return CRTLogSystem->CleanUp();
9269
}
93-
else if (s_CRTLogsRedirectionIsSet)
94-
{
95-
assert(s_previousCrtLogger.vtable->clean_up);
96-
return s_previousCrtLogger.vtable->clean_up(&s_previousCrtLogger);
97-
}
98-
assert(!"Unreachable state: this method is called without redirection set");
9970
}
10071

10172
static int s_aws_logger_redirect_set_log_level(struct aws_logger *logger, enum aws_log_level log_level)
10273
{
10374
AWS_UNREFERENCED_PARAM(logger);
104-
Aws::Utils::Threading::ReaderLockGuard guard(CRTLogSystemLock);
105-
if (CRTLogSystem)
106-
{
107-
assert(logger->p_impl == &s_sdkCrtLogger);
108-
CRTLogSystem->SetLogLevel(static_cast<LogLevel>(log_level));
109-
return AWS_OP_SUCCESS;
110-
}
111-
else if (s_CRTLogsRedirectionIsSet)
75+
if (!CRTLogSystem)
11276
{
113-
assert(s_previousCrtLogger.vtable->set_log_level);
114-
return s_previousCrtLogger.vtable->set_log_level(&s_previousCrtLogger, log_level);
77+
return AWS_OP_ERR;
11578
}
116-
assert(!"Unreachable state: this method is called without redirection set");
117-
return AWS_OP_ERR;
79+
assert(logger->p_impl == &s_sdkCrtLogger);
80+
CRTLogSystem->SetLogLevel(static_cast<LogLevel>(log_level));
81+
return AWS_OP_SUCCESS;
82+
11883
}
11984

12085
static struct aws_logger_vtable s_aws_logger_redirect_vtable = {
@@ -128,39 +93,29 @@ static struct aws_logger_vtable s_aws_logger_redirect_vtable = {
12893
/**
12994
* Installs SDK wrapper over CRT logger.
13095
* This wrapper will redirect all CRT logger calls to the set SDK "CRTLogSystem"
131-
* (or back to an original CRT logger if SDK API is terminated).
13296
* This method is not thread-safe (as the most other global Init/Shutdown APIs of the SDK).
13397
*/
13498
void SetUpCrtLogsRedirection()
13599
{
136-
if (!s_CRTLogsRedirectionIsSet)
137-
{
138-
static std::once_flag flag;
139-
std::call_once(flag, [&]()
140-
{
141-
s_sdkCrtLogger.vtable = &s_aws_logger_redirect_vtable;
142-
s_sdkCrtLogger.allocator = Aws::get_aws_allocator();
143-
s_sdkCrtLogger.p_impl = &s_sdkCrtLogger;
144-
145-
auto pPrevLogger = aws_logger_get();
146-
if (pPrevLogger) {
147-
s_previousCrtLogger = *pPrevLogger;
148-
}
149-
aws_logger_set(&s_sdkCrtLogger);
150-
s_CRTLogsRedirectionIsSet = true;
151-
});
152-
}
100+
s_sdkCrtLogger.vtable = &s_aws_logger_redirect_vtable;
101+
s_sdkCrtLogger.allocator = Aws::get_aws_allocator();
102+
s_sdkCrtLogger.p_impl = &s_sdkCrtLogger;
103+
104+
aws_logger_set(&s_sdkCrtLogger);
153105
}
154106

155107
void InitializeCRTLogging(const std::shared_ptr<CRTLogSystemInterface>& inputCrtLogSystem) {
156-
Aws::Utils::Threading::WriterLockGuard g(CRTLogSystemLock);
157108
SetUpCrtLogsRedirection();
158109
CRTLogSystem = inputCrtLogSystem;
159110
}
160111

161112
void ShutdownCRTLogging() {
162-
Aws::Utils::Threading::WriterLockGuard g(CRTLogSystemLock);
163-
CRTLogSystem.reset();
113+
// GetLogSystem returns a raw pointer
114+
// so this is a hack to let all other threads finish their log statement after getting a LogSystem pointer
115+
// otherwise we would need to perform ref-counting on each logging statement
116+
auto tmpLogger = std::move(CRTLogSystem);
117+
std::this_thread::sleep_for(std::chrono::milliseconds(1));
118+
tmpLogger.reset();
164119
}
165120

166121
} // namespace Logging

‎src/aws-cpp-sdk-core/source/utils/logging/DefaultCRTLogSystem.cpp

Lines changed: 16 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -22,57 +22,32 @@ namespace Aws
2222
{
2323
namespace Logging
2424
{
25-
DefaultCRTLogSystem::DefaultCRTLogSystem(LogLevel logLevel) :
26-
m_logLevel(logLevel),
27-
m_stopLogging(false),
28-
m_logsProcessed(0)
25+
DefaultCRTLogSystem::DefaultCRTLogSystem(LogLevel logLevel)
2926
{
27+
AWS_UNREFERENCED_PARAM(logLevel); // will use one from the main SDK logger
3028
}
3129

32-
DefaultCRTLogSystem::~DefaultCRTLogSystem()
30+
LogLevel DefaultCRTLogSystem::GetLogLevel() const
3331
{
34-
m_stopLogging = true;
32+
LogSystemInterface* pLogSystem = Logging::GetLogSystem();
33+
if (pLogSystem)
34+
{
35+
return pLogSystem->GetLogLevel();
36+
}
37+
return Aws::Utils::Logging::LogLevel::Off;
38+
}
3539

36-
// Allow all other threads running Log(...) to return
37-
std::unique_lock<std::mutex> lock(m_stopMutex);
38-
m_stopSignal.wait_for(lock,
39-
std::chrono::milliseconds(200),
40-
[&](){ return m_logsProcessed.load() == 0; });
40+
void DefaultCRTLogSystem::SetLogLevel(LogLevel logLevel)
41+
{
42+
AWS_UNREFERENCED_PARAM(logLevel); // will use one from the main SDK logger
4143
}
4244

4345
void DefaultCRTLogSystem::Log(LogLevel logLevel, const char* subjectName, const char* formatStr, va_list args)
4446
{
45-
if(m_stopLogging)
46-
{
47-
return;
48-
}
49-
m_logsProcessed++;
50-
va_list tmp_args;
51-
va_copy(tmp_args, args);
52-
#ifdef _WIN32
53-
const int requiredLength = _vscprintf(formatStr, tmp_args) + 1;
54-
#else
55-
const int requiredLength = vsnprintf(nullptr, 0, formatStr, tmp_args) + 1;
56-
#endif
57-
va_end(tmp_args);
58-
59-
Aws::OStringStream logStream;
60-
if (requiredLength > 1)
61-
{
62-
Array<char> outputBuff(requiredLength);
63-
#ifdef _WIN32
64-
vsnprintf_s(outputBuff.GetUnderlyingData(), requiredLength, _TRUNCATE, formatStr, args);
65-
#else
66-
vsnprintf(outputBuff.GetUnderlyingData(), requiredLength, formatStr, args);
67-
#endif // _WIN32
68-
logStream << outputBuff.GetUnderlyingData();
69-
}
70-
Logging::GetLogSystem()->LogStream(logLevel, subjectName, logStream);
71-
m_logsProcessed--;
72-
if(m_logsProcessed == 0 && m_stopLogging)
47+
LogSystemInterface* pLogSystem = Logging::GetLogSystem();
48+
if (pLogSystem)
7349
{
74-
std::unique_lock<std::mutex> lock(m_stopMutex);
75-
m_stopSignal.notify_all();
50+
pLogSystem->vaLog(logLevel, subjectName, formatStr, args);
7651
}
7752
}
7853
}

0 commit comments

Comments
 (0)