You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by mm...@apache.org on 2020/06/02 22:15:18 UTC
[pulsar] branch master updated: [C++] logging factory should be
stored as static shared_ptr (#7132)
This is an automated email from the ASF dual-hosted git repository.
mmerli pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/pulsar.git
The following commit(s) were added to refs/heads/master by this push:
new c74414c [C++] logging factory should be stored as static shared_ptr (#7132)
c74414c is described below
commit c74414c219b465bf8278de30c22f5bad950529da
Author: Matteo Merli <mm...@apache.org>
AuthorDate: Tue Jun 2 15:15:01 2020 -0700
[C++] logging factory should be stored as static shared_ptr (#7132)
C++ references and thus cleans up all statics on exit. This can happen
when there are still threads active. If a thread tries to access
logging when the process is shutting down, this will possibly use a
dead object and corrupt the heap, triggering a segfault.
The solution is to make the logger factory immortal. Once it is set,
it is assigned to an atomic static pointer, and this object is
thereafter never cleaned up. This does change the signature for
passing in logger factories, as shared_ptr is no longer valid.
Co-authored-by: Ivan Kelly <ik...@splunk.com>
---
.../include/pulsar/ClientConfiguration.h | 9 ++++++---
pulsar-client-cpp/include/pulsar/Logger.h | 1 -
pulsar-client-cpp/lib/ClientConfiguration.cc | 6 ++----
pulsar-client-cpp/lib/ClientConfigurationImpl.h | 4 +++-
pulsar-client-cpp/lib/ClientImpl.cc | 16 +++++++---------
pulsar-client-cpp/lib/LogUtils.cc | 20 ++++++++++++++------
pulsar-client-cpp/lib/LogUtils.h | 4 ++--
pulsar-client-cpp/lib/SimpleLoggerImpl.cc | 8 ++++++--
pulsar-client-cpp/lib/SimpleLoggerImpl.h | 4 ++--
pulsar-client-cpp/lib/c/c_ClientConfiguration.cc | 2 +-
10 files changed, 43 insertions(+), 31 deletions(-)
diff --git a/pulsar-client-cpp/include/pulsar/ClientConfiguration.h b/pulsar-client-cpp/include/pulsar/ClientConfiguration.h
index 056f7a7..e65aecc 100644
--- a/pulsar-client-cpp/include/pulsar/ClientConfiguration.h
+++ b/pulsar-client-cpp/include/pulsar/ClientConfiguration.h
@@ -121,10 +121,13 @@ class PULSAR_PUBLIC ClientConfiguration {
* to a different logger implementation.
*
* By default, log messages are printed on standard output.
+ *
+ * When passed in, the configuration takes ownership of the loggerFactory object.
+ * The logger factory can only be set once per process. Any subsequent calls to
+ * set the logger factory will have no effect, though the logger factory object
+ * will be cleaned up.
*/
- ClientConfiguration& setLogger(LoggerFactoryPtr loggerFactory);
-
- LoggerFactoryPtr getLogger() const;
+ ClientConfiguration& setLogger(LoggerFactory* loggerFactory);
ClientConfiguration& setUseTls(bool useTls);
bool isUseTls() const;
diff --git a/pulsar-client-cpp/include/pulsar/Logger.h b/pulsar-client-cpp/include/pulsar/Logger.h
index 7351b3a..ab80851 100644
--- a/pulsar-client-cpp/include/pulsar/Logger.h
+++ b/pulsar-client-cpp/include/pulsar/Logger.h
@@ -48,5 +48,4 @@ class PULSAR_PUBLIC LoggerFactory {
virtual Logger* getLogger(const std::string& fileName) = 0;
};
-typedef std::shared_ptr<LoggerFactory> LoggerFactoryPtr;
} // namespace pulsar
diff --git a/pulsar-client-cpp/lib/ClientConfiguration.cc b/pulsar-client-cpp/lib/ClientConfiguration.cc
index 920b168..1733dde 100644
--- a/pulsar-client-cpp/lib/ClientConfiguration.cc
+++ b/pulsar-client-cpp/lib/ClientConfiguration.cc
@@ -103,13 +103,11 @@ ClientConfiguration& ClientConfiguration::setLogConfFilePath(const std::string&
const std::string& ClientConfiguration::getLogConfFilePath() const { return impl_->logConfFilePath; }
-ClientConfiguration& ClientConfiguration::setLogger(LoggerFactoryPtr loggerFactory) {
- impl_->loggerFactory = loggerFactory;
+ClientConfiguration& ClientConfiguration::setLogger(LoggerFactory* loggerFactory) {
+ impl_->loggerFactory.reset(loggerFactory);
return *this;
}
-LoggerFactoryPtr ClientConfiguration::getLogger() const { return impl_->loggerFactory; }
-
ClientConfiguration& ClientConfiguration::setStatsIntervalInSeconds(
const unsigned int& statsIntervalInSeconds) {
impl_->statsIntervalInSeconds = statsIntervalInSeconds;
diff --git a/pulsar-client-cpp/lib/ClientConfigurationImpl.h b/pulsar-client-cpp/lib/ClientConfigurationImpl.h
index e2f72fb..11cd8c9 100644
--- a/pulsar-client-cpp/lib/ClientConfigurationImpl.h
+++ b/pulsar-client-cpp/lib/ClientConfigurationImpl.h
@@ -34,7 +34,7 @@ struct ClientConfigurationImpl {
std::string tlsTrustCertsFilePath;
bool tlsAllowInsecureConnection;
unsigned int statsIntervalInSeconds;
- LoggerFactoryPtr loggerFactory;
+ std::unique_ptr<LoggerFactory> loggerFactory;
bool validateHostName;
unsigned int partitionsUpdateInterval;
@@ -52,6 +52,8 @@ struct ClientConfigurationImpl {
validateHostName(false),
partitionsUpdateInterval(60) // 1 minute
{}
+
+ std::unique_ptr<LoggerFactory> takeLogger() { return std::move(loggerFactory); }
};
} // namespace pulsar
diff --git a/pulsar-client-cpp/lib/ClientImpl.cc b/pulsar-client-cpp/lib/ClientImpl.cc
index cb9bf67..ccf7620 100644
--- a/pulsar-client-cpp/lib/ClientImpl.cc
+++ b/pulsar-client-cpp/lib/ClientImpl.cc
@@ -17,7 +17,7 @@
* under the License.
*/
#include "ClientImpl.h"
-
+#include "ClientConfigurationImpl.h"
#include "LogUtils.h"
#include "ConsumerImpl.h"
#include "ProducerImpl.h"
@@ -96,24 +96,22 @@ ClientImpl::ClientImpl(const std::string& serviceUrl, const ClientConfiguration&
consumerIdGenerator_(0),
requestIdGenerator_(0),
closingError(ResultOk) {
- if (clientConfiguration_.getLogger()) {
- // A logger factory was explicitely configured. Let's just use that
- LogUtils::setLoggerFactory(clientConfiguration_.getLogger());
- } else {
+ std::unique_ptr<LoggerFactory> loggerFactory = clientConfiguration_.impl_->takeLogger();
+ if (!logger) {
#ifdef USE_LOG4CXX
if (!clientConfiguration_.getLogConfFilePath().empty()) {
// A log4cxx log file was passed through deprecated parameter. Use that to configure Log4CXX
- LogUtils::setLoggerFactory(
- Log4CxxLoggerFactory::create(clientConfiguration_.getLogConfFilePath()));
+ loggerFactory = Log4CxxLoggerFactory::create(clientConfiguration_.getLogConfFilePath());
} else {
// Use default simple console logger
- LogUtils::setLoggerFactory(SimpleLoggerFactory::create());
+ loggerFactory = SimpleLoggerFactory::create();
}
#else
// Use default simple console logger
- LogUtils::setLoggerFactory(SimpleLoggerFactory::create());
+ loggerFactory = SimpleLoggerFactory::create();
#endif
}
+ LogUtils::setLoggerFactory(std::move(loggerFactory));
if (serviceUrl_.compare(0, 4, "http") == 0) {
LOG_DEBUG("Using HTTP Lookup");
diff --git a/pulsar-client-cpp/lib/LogUtils.cc b/pulsar-client-cpp/lib/LogUtils.cc
index e4f6a17..9de6ff4 100644
--- a/pulsar-client-cpp/lib/LogUtils.cc
+++ b/pulsar-client-cpp/lib/LogUtils.cc
@@ -18,6 +18,7 @@
*/
#include "LogUtils.h"
+#include <atomic>
#include <iostream>
#include "SimpleLoggerImpl.h"
@@ -37,15 +38,22 @@ void LogUtils::init(const std::string& logfilePath) {
#endif // USE_LOG4CXX
}
-static LoggerFactoryPtr s_loggerFactory;
+static std::atomic<LoggerFactory*> s_loggerFactory(nullptr);
-void LogUtils::setLoggerFactory(LoggerFactoryPtr loggerFactory) { s_loggerFactory = loggerFactory; }
+void LogUtils::setLoggerFactory(std::unique_ptr<LoggerFactory> loggerFactory) {
+ LoggerFactory* oldFactory = nullptr;
+ LoggerFactory* newFactory = loggerFactory.release();
+ if (!s_loggerFactory.compare_exchange_strong(oldFactory, newFactory)) {
+ delete newFactory; // there's already a factory set
+ }
+}
-LoggerFactoryPtr LogUtils::getLoggerFactory() {
- if (!s_loggerFactory) {
- s_loggerFactory.reset(new SimpleLoggerFactory());
+LoggerFactory* LogUtils::getLoggerFactory() {
+ if (s_loggerFactory.load() == nullptr) {
+ std::unique_ptr<LoggerFactory> newFactory(new SimpleLoggerFactory());
+ setLoggerFactory(std::move(newFactory));
}
- return s_loggerFactory;
+ return s_loggerFactory.load();
}
std::string LogUtils::getLoggerName(const std::string& path) {
diff --git a/pulsar-client-cpp/lib/LogUtils.h b/pulsar-client-cpp/lib/LogUtils.h
index b2d327c..61d3e63 100644
--- a/pulsar-client-cpp/lib/LogUtils.h
+++ b/pulsar-client-cpp/lib/LogUtils.h
@@ -86,9 +86,9 @@ class PULSAR_PUBLIC LogUtils {
public:
static void init(const std::string& logConfFilePath);
- static void setLoggerFactory(LoggerFactoryPtr loggerFactory);
+ static void setLoggerFactory(std::unique_ptr<LoggerFactory> loggerFactory);
- static LoggerFactoryPtr getLoggerFactory();
+ static LoggerFactory* getLoggerFactory();
static std::string getLoggerName(const std::string& path);
};
diff --git a/pulsar-client-cpp/lib/SimpleLoggerImpl.cc b/pulsar-client-cpp/lib/SimpleLoggerImpl.cc
index 211fa88..a8553e2 100644
--- a/pulsar-client-cpp/lib/SimpleLoggerImpl.cc
+++ b/pulsar-client-cpp/lib/SimpleLoggerImpl.cc
@@ -21,6 +21,7 @@
#include <iostream>
#include <sstream>
+#include <thread>
#include <boost/date_time/posix_time/posix_time.hpp>
#include <boost/format.hpp>
@@ -57,7 +58,8 @@ class SimpleLogger : public Logger {
std::stringstream ss;
printTimestamp(ss);
- ss << " " << level << " " << _logger << ":" << line << " | " << message << "\n";
+ ss << " " << level << " [" << std::this_thread::get_id() << "] " << _logger << ":" << line << " | "
+ << message << "\n";
std::cout << ss.str();
std::cout.flush();
@@ -80,5 +82,7 @@ class SimpleLogger : public Logger {
Logger *SimpleLoggerFactory::getLogger(const std::string &file) { return new SimpleLogger(file); }
-LoggerFactoryPtr SimpleLoggerFactory::create() { return LoggerFactoryPtr(new SimpleLoggerFactory); }
+std::unique_ptr<LoggerFactory> SimpleLoggerFactory::create() {
+ return std::unique_ptr<LoggerFactory>(new SimpleLoggerFactory());
+}
} // namespace pulsar
diff --git a/pulsar-client-cpp/lib/SimpleLoggerImpl.h b/pulsar-client-cpp/lib/SimpleLoggerImpl.h
index 2b81060..3a18c0d 100644
--- a/pulsar-client-cpp/lib/SimpleLoggerImpl.h
+++ b/pulsar-client-cpp/lib/SimpleLoggerImpl.h
@@ -27,7 +27,7 @@ class SimpleLoggerFactory : public LoggerFactory {
public:
Logger* getLogger(const std::string& fileName);
- static LoggerFactoryPtr create();
+ static std::unique_ptr<LoggerFactory> create();
};
-} // namespace pulsar
\ No newline at end of file
+} // namespace pulsar
diff --git a/pulsar-client-cpp/lib/c/c_ClientConfiguration.cc b/pulsar-client-cpp/lib/c/c_ClientConfiguration.cc
index fbcf2af..4f4a4b7 100644
--- a/pulsar-client-cpp/lib/c/c_ClientConfiguration.cc
+++ b/pulsar-client-cpp/lib/c/c_ClientConfiguration.cc
@@ -99,7 +99,7 @@ class PulsarCLoggerFactory : public pulsar::LoggerFactory {
void pulsar_client_configuration_set_logger(pulsar_client_configuration_t *conf, pulsar_logger logger,
void *ctx) {
- conf->conf.setLogger(pulsar::LoggerFactoryPtr(new PulsarCLoggerFactory(logger, ctx)));
+ conf->conf.setLogger(new PulsarCLoggerFactory(logger, ctx));
}
void pulsar_client_configuration_set_use_tls(pulsar_client_configuration_t *conf, int useTls) {