Skip to content

Commit d1b287c

Browse files
author
Masahiro Sakamoto
authored
Fix issue where custom logger setting is ignored (#377)
### Motivation I noticed that there are cases where the specified logger factory is not used. For example, if we compile and run the following code, it expects `FileLoggerFactory` to be used, but it actually uses the default `ConsoleLoggerFactory`. ```cpp ClientConfiguration config = ClientConfiguration(); config.setLogger(new FileLoggerFactory(Logger::LEVEL_INFO, "pulsar-client-cpp.log")); ParamMap params; params["providerDomain"] = "provider.foo.bar"; params["tenantDomain"] = "tenant.foo.bar"; params["tenantService"] = "test-service"; params["privateKey"] = "file:///path/to/private.key"; params["keyId"] = "0"; params["ztsUrl"] = "https://zts.athenz.example.com:443"; AuthenticationPtr auth = AuthAthenz::create(params); config.setAuth(auth); Client client("pulsar://localhost:6650", config); ``` This happens if logging is performed before `LogUtils::setLoggerFactory` is executed in the constructor of `ClientImpl`. https://github.com/apache/pulsar-client-cpp/blob/25ea451eb3a79d48966689ec64460ae03d5d57da/lib/ClientImpl.cc#L94-L99 When logging is performed, `LogUtils::getLoggerFactory` is called. 1. https://github.com/apache/pulsar-client-cpp/blob/25ea451eb3a79d48966689ec64460ae03d5d57da/lib/LogUtils.h#L60 1. https://github.com/apache/pulsar-client-cpp/blob/25ea451eb3a79d48966689ec64460ae03d5d57da/lib/LogUtils.h#L43 If the constructor of `ClientImpl` has not been executed at this point, the default `ConsoleLoggerFactory` is instantiated, set to the static variable `s_loggerFactory`, and returned to the caller. https://github.com/apache/pulsar-client-cpp/blob/25ea451eb3a79d48966689ec64460ae03d5d57da/lib/LogUtils.cc#L38-L44 Loggers generated from the returned `LoggerFactory` will be set to the static and thread_local variable `threadSpecificLogPtr`, and reused from now on. https://github.com/apache/pulsar-client-cpp/blob/25ea451eb3a79d48966689ec64460ae03d5d57da/lib/LogUtils.h#L38-L47 After that, `LogUtils::setLoggerFactory` is executed again in the constructor of `ClientImpl`, but the value can only be set to `s_loggerFactory` once, and it will be ignored from the second time. This is clearly stated in [the comments](https://github.com/apache/pulsar-client-cpp/blob/25ea451eb3a79d48966689ec64460ae03d5d57da/include/pulsar/ClientConfiguration.h#L183-L186). https://github.com/apache/pulsar-client-cpp/blob/25ea451eb3a79d48966689ec64460ae03d5d57da/lib/LogUtils.cc#L30-L36 Even if we can set it again, if a logger is already set to `threadSpecificLogPtr`, a new logger will not be generated from the new `LoggerFactory`. ### Modifications Keep an instance of the default `ConsoleLoggerFactory` in `s_defaultLoggerFactory`, a variable separate from `s_loggerFactory`, and return it if `LoggerFactory` is not set yet. In addition, keep the pointer value of the `LoggerFactory` that generated the logger cached in `threadSpecificLogPtr`, and regenerate the logger if the `LoggerFactory` pointer changes.
1 parent 25ea451 commit d1b287c

File tree

4 files changed

+29
-17
lines changed

4 files changed

+29
-17
lines changed

lib/ClientImpl.cc

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -92,11 +92,9 @@ ClientImpl::ClientImpl(const std::string& serviceUrl, const ClientConfiguration&
9292
consumerIdGenerator_(0),
9393
closingError(ResultOk) {
9494
std::unique_ptr<LoggerFactory> loggerFactory = clientConfiguration_.impl_->takeLogger();
95-
if (!loggerFactory) {
96-
// Use default simple console logger
97-
loggerFactory.reset(new ConsoleLoggerFactory);
95+
if (loggerFactory) {
96+
LogUtils::setLoggerFactory(std::move(loggerFactory));
9897
}
99-
LogUtils::setLoggerFactory(std::move(loggerFactory));
10098

10199
LookupServicePtr underlyingLookupServicePtr;
102100
if (serviceNameResolver_.useHttp()) {

lib/LogUtils.cc

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@
2525

2626
namespace pulsar {
2727

28+
static std::atomic<LoggerFactory*> s_defaultLoggerFactory(new ConsoleLoggerFactory());
2829
static std::atomic<LoggerFactory*> s_loggerFactory(nullptr);
2930

3031
void LogUtils::setLoggerFactory(std::unique_ptr<LoggerFactory> loggerFactory) {
@@ -37,10 +38,10 @@ void LogUtils::setLoggerFactory(std::unique_ptr<LoggerFactory> loggerFactory) {
3738

3839
LoggerFactory* LogUtils::getLoggerFactory() {
3940
if (s_loggerFactory.load() == nullptr) {
40-
std::unique_ptr<LoggerFactory> newFactory(new ConsoleLoggerFactory());
41-
setLoggerFactory(std::move(newFactory));
41+
return s_defaultLoggerFactory.load();
42+
} else {
43+
return s_loggerFactory.load();
4244
}
43-
return s_loggerFactory.load();
4445
}
4546

4647
std::string LogUtils::getLoggerName(const std::string& path) {

lib/LogUtils.h

Lines changed: 13 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -34,16 +34,19 @@ namespace pulsar {
3434
#define PULSAR_UNLIKELY(expr) (expr)
3535
#endif
3636

37-
#define DECLARE_LOG_OBJECT() \
38-
static pulsar::Logger* logger() { \
39-
static thread_local std::unique_ptr<pulsar::Logger> threadSpecificLogPtr; \
40-
pulsar::Logger* ptr = threadSpecificLogPtr.get(); \
41-
if (PULSAR_UNLIKELY(!ptr)) { \
42-
std::string logger = pulsar::LogUtils::getLoggerName(__FILE__); \
43-
threadSpecificLogPtr.reset(pulsar::LogUtils::getLoggerFactory()->getLogger(logger)); \
44-
ptr = threadSpecificLogPtr.get(); \
45-
} \
46-
return ptr; \
37+
#define DECLARE_LOG_OBJECT() \
38+
static pulsar::Logger* logger() { \
39+
static thread_local uintptr_t loggerFactoryPtr = 0; \
40+
static thread_local std::unique_ptr<pulsar::Logger> threadSpecificLogPtr; \
41+
pulsar::Logger* ptr = threadSpecificLogPtr.get(); \
42+
if (PULSAR_UNLIKELY(loggerFactoryPtr != (uintptr_t)pulsar::LogUtils::getLoggerFactory()) || \
43+
PULSAR_UNLIKELY(!ptr)) { \
44+
std::string logger = pulsar::LogUtils::getLoggerName(__FILE__); \
45+
threadSpecificLogPtr.reset(pulsar::LogUtils::getLoggerFactory()->getLogger(logger)); \
46+
ptr = threadSpecificLogPtr.get(); \
47+
loggerFactoryPtr = (uintptr_t)pulsar::LogUtils::getLoggerFactory(); \
48+
} \
49+
return ptr; \
4750
}
4851

4952
#define LOG_DEBUG(message) \

tests/CustomLoggerTest.cc

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -107,3 +107,13 @@ TEST(CustomLoggerTest, testConsoleLoggerFactory) {
107107
ASSERT_FALSE(logger->isEnabled(Logger::LEVEL_WARN));
108108
ASSERT_TRUE(logger->isEnabled(Logger::LEVEL_ERROR));
109109
}
110+
111+
TEST(CustomLoggerTest, testSetAndGetLoggerFactory) {
112+
LoggerFactory *oldFactory = LogUtils::getLoggerFactory();
113+
LoggerFactory *newFactory = new ConsoleLoggerFactory(Logger::LEVEL_ERROR);
114+
std::unique_ptr<LoggerFactory> newFactoryPtr(newFactory);
115+
LogUtils::setLoggerFactory(std::move(newFactoryPtr));
116+
ASSERT_NE(oldFactory, LogUtils::getLoggerFactory());
117+
ASSERT_EQ(newFactory, LogUtils::getLoggerFactory());
118+
LogUtils::resetLoggerFactory();
119+
}

0 commit comments

Comments
 (0)