[C++] logging factory should be stored as static shared_ptr (apache#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 <ikelly@splunk.com>

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;

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

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;

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
 

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");

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) {

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);
 };

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

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
+}  // namespace pulsar

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) {