[master] Merge branch 'trac3823'

src/lib/log/message_initializer.cc

@@ -14,40 +14,37 @@
 
 #include <log/message_dictionary.h>
 #include <log/message_initializer.h>
-#include <boost/array.hpp>
 #include <algorithm>
 #include <cassert>
 #include <cstdlib>
 
 
-
-// As explained in the header file, initialization of the message dictionary
-// is a two-stage process:
-// 1) In the MessageInitializer constructor, a pointer to the array of
-//    messages is stored in a pre-defined array.  Since the MessageInitializers
-//    are declared statically outside a program unit, this takes place before
-//    main() is called.  As no heap storage is allocated in this process, we
-//    should avoid the static initialization fiasco in cases where
-//    initialization of system libraries is also carried out at the same time.
-// 2) After main() starts executing, loadDictionary() is called.
-//
-//
-
 namespace {
 
-/// Type definition for the list of pointers to messages.
-typedef std::list<const char**> LoggerValuesList;
-
-/// Return reference to the list of log messages.
-LoggerValuesList& getNonConstLoggerValues() {
-    static LoggerValuesList logger_values;
+using namespace isc::log;
+
+/// @brief Returns the shared pointer to the list of pointers to the
+/// log messages defined.
+///
+/// The returned pointer must be held in the \c MessageInitializer object
+/// throughout its lifetime to make sure that the object doesn't outlive
+/// the list and may still access it in the destructor. The returned
+/// pointer is shared between all \c MessageInitializer instances.
+LoggerValuesListPtr
+getNonConstLoggerValues() {
+    static LoggerValuesListPtr logger_values(new LoggerValuesList());
     return (logger_values);
 }
 
-// Return the duplicates singleton version (non-const for local use)
-std::list<std::string>&
+/// @brief Returns the pointer to the list of message duplicates.
+///
+/// The returned pointer must be held in the \c MessageInitializer object
+/// throughout its lifetime to make sure that the object doesn't outlive
+/// the list and may still access it in the destructor. The returned
+/// pointer is shared between all \c MessageInitializer instances.
+LoggerDuplicatesListPtr
 getNonConstDuplicates() {
-    static std::list<std::string> duplicates;
+    static LoggerDuplicatesListPtr duplicates(new LoggerDuplicatesList());
     return (duplicates);
 }
 } // end unnamed namespace
@@ -56,36 +53,42 @@ getNonConstDuplicates() {
 namespace isc {
 namespace log {
 
-// Constructor.  Add the pointer to the message array to the global array.
-// This method will trigger an assertion failure if the array overflows.
-
 MessageInitializer::MessageInitializer(const char* values[])
     : values_(values),
-      global_dictionary_(MessageDictionary::globalDictionary()) {
-    assert(getNonConstLoggerValues().size() < MAX_MESSAGE_ARRAYS);
-    getNonConstLoggerValues().push_back(values);
+      global_dictionary_(MessageDictionary::globalDictionary()),
+      global_logger_values_(getNonConstLoggerValues()),
+      global_logger_duplicates_(getNonConstDuplicates()) {
+    global_logger_values_->push_back(values);
 }
 
 MessageInitializer::~MessageInitializer() {
-    LoggerValuesList& logger_values = getNonConstLoggerValues();
-
     // Search for the pointer to pending messages belonging to our instance.
-    LoggerValuesList::iterator my_messages = std::find(logger_values.begin(),
-                                                       logger_values.end(),
+    LoggerValuesList::iterator my_messages = std::find(global_logger_values_->begin(),
+                                                       global_logger_values_->end(),
                                                        values_);
-    bool pending = (my_messages != logger_values.end());
+    bool pending = (my_messages != global_logger_values_->end());
     // Our messages are still pending, so let's remove them from the list
     // of pending messages.
     if (pending) {
-        logger_values.erase(my_messages);
+        global_logger_values_->erase(my_messages);
 
     } else {
         // Our messages are not pending, so they might have been loaded to
         // the dictionary and/or duplicates.
         int i = 0;
         while (values_[i]) {
-            getNonConstDuplicates().remove(values_[i]);
-            global_dictionary_->erase(values_[i], values_[i + 1]);
+            // Check if the unloaded message is registered as duplicate. If it is,
+            // remove it from the duplicates list.
+            LoggerDuplicatesList::const_iterator dup =
+                std::find(global_logger_duplicates_->begin(),
+                          global_logger_duplicates_->end(),
+                          values_[i]);
+            if (dup != global_logger_duplicates_->end()) {
+                global_logger_duplicates_->erase(dup);
+
+            } else {
+                global_dictionary_->erase(values_[i], values_[i + 1]);
+            }
             i += 2;
         }
     }
@@ -95,7 +98,7 @@ MessageInitializer::~MessageInitializer() {
 
 size_t
 MessageInitializer::getPendingCount() {
-    return (getNonConstLoggerValues().size());
+    return (getNonConstLoggerValues()->size());
 }
 
 // Load the messages in the arrays registered in the logger_values array
@@ -104,37 +107,36 @@ MessageInitializer::getPendingCount() {
 void
 MessageInitializer::loadDictionary(bool ignore_duplicates) {
     const MessageDictionaryPtr& global = MessageDictionary::globalDictionary();
-    LoggerValuesList& logger_values = getNonConstLoggerValues(); 
+    const LoggerValuesListPtr& logger_values = getNonConstLoggerValues();
 
-    for (LoggerValuesList::const_iterator values = logger_values.begin();
-         values != logger_values.end(); ++values) {
+    for (LoggerValuesList::const_iterator values = logger_values->begin();
+         values != logger_values->end(); ++values) {
         std::vector<std::string> repeats = global->load(*values);
 
         // Append the IDs in the list just loaded (the "repeats") to the
         // global list of duplicate IDs.
         if (!ignore_duplicates && !repeats.empty()) {
-            std::list<std::string>& duplicates = getNonConstDuplicates();
-            duplicates.insert(duplicates.end(), repeats.begin(),
-                              repeats.end());
+            const LoggerDuplicatesListPtr& duplicates = getNonConstDuplicates();
+            duplicates->insert(duplicates->end(), repeats.begin(), repeats.end());
         }
     }
 
     // ... and mark that the messages have been loaded.  (This avoids a lot
     // of "duplicate message ID" messages in some of the unit tests where the
     // logging initialization code may be called multiple times.)
-    logger_values.clear();
+    logger_values->clear();
 }
 
 // Return reference to duplicates vector
 const std::list<std::string>&
 MessageInitializer::getDuplicates() {
-    return (getNonConstDuplicates());
+    return (*getNonConstDuplicates());
 }
 
 // Clear the duplicates vector
 void
 MessageInitializer::clearDuplicates() {
-    getNonConstDuplicates().clear();
+    getNonConstDuplicates()->clear();
 }
 
 } // namespace log

src/lib/log/message_initializer.h

@@ -17,6 +17,7 @@
 
 #include <log/message_dictionary.h>
 #include <boost/noncopyable.hpp>
+#include <boost/shared_ptr.hpp>
 #include <cstdlib>
 #include <list>
 #include <string>
@@ -24,6 +25,20 @@
 namespace isc {
 namespace log {
 
+/// @name Type definitions for containers shared among instances of the class.
+///
+//\{
+/// \brief List of pointers to the messages.
+typedef std::list<const char**> LoggerValuesList;
+/// \brief Shared pointer to the list of pointers to the messages.
+typedef boost::shared_ptr<LoggerValuesList> LoggerValuesListPtr;
+
+/// \brief List of duplicated messages.
+typedef std::list<std::string> LoggerDuplicatesList;
+/// \brief Shared pointer to the list of duplicated messages.
+typedef boost::shared_ptr<LoggerDuplicatesList> LoggerDuplicatesListPtr;
+//\}
+
 /// \brief Initialize Message Dictionary
 ///
 /// This is a helper class to add a set of message IDs and associated text to
@@ -47,20 +62,13 @@ namespace log {
 /// Dynamically loaded modules should call the initializer as well on the
 /// moment they are instantiated.
 ///
-/// To avoid static initialization fiasco problems, the initialization is
-/// carried out in two stages:
-/// - The constructor adds a pointer to the values array to a pre-defined array
-///   of pointers.
-/// - During the run-time initialization of the logging system, the static
-///   method loadDictionary() is called to load the message dictionary.
-/// This way, no heap storage is allocated during the static initialization,
-/// something that may give problems on some operating systems.
-///
-/// \note The maximum number of message arrays that can be added to the
-/// dictionary in this way is defined by the constant
-/// MessageInitializer::MAX_MESSAGE_ARRAYS.  This is set to 256 as a compromise
-/// between wasted space and allowing for future expansion, but can be
-/// changed (by editing the source file) to any value desired.
+/// To avoid static initialization fiasco problems, the containers shared by
+/// all instances of this class are dynamically allocated on first use, and
+/// held in the smart pointers which are de-allocated only when all instances
+/// of the class are destructred. After the object has been created with the
+/// constructor, the \c MessageInitializer::loadDictionary static function is
+/// called to populate the messages defined in various instances of the
+/// \c MessageInitializer class to the global dictionary.
 ///
 /// When messages are added to the dictionary, the are added via the
 /// MessageDictionary::add() method, so any duplicates are stored in the
@@ -69,8 +77,6 @@ namespace log {
 
 class MessageInitializer : public boost::noncopyable {
 public:
-    /// Maximum number of message arrays that can be initialized in this way
-    static const size_t MAX_MESSAGE_ARRAYS = 256;
 
     /// \brief Constructor
     ///
@@ -144,6 +150,24 @@ private:
     /// before the \c MessageInitializer destructor is called, causing the
     /// static initialization order fiasco.
     MessageDictionaryPtr global_dictionary_;
+
+    /// \brief Holds the shared pointer to the list of pointers to the
+    /// log messages defined by various instances of this class.
+    ///
+    /// This pointer must be initialized in the constructor and held
+    /// throughout the lifetime of the \c MessageInitializer object. This
+    /// prevents static deinitialization fiasco when trying to access the
+    /// values in the list from the destructor of this class.
+    LoggerValuesListPtr global_logger_values_;
+
+    /// \brief Holds the shared pointer to the collection od duplicated
+    /// messages.
+    ///
+    /// This pointer must be initialized in the constructor and held
+    /// throughout the lifetime of the \c MessageInitializer object. This
+    /// prevents static deinitialization fiasco when trying to access the
+    /// values in the list from the destructor of this class.
+    LoggerDuplicatesListPtr global_logger_duplicates_;
 };
 
 } // namespace log

src/lib/log/tests/Makefile.am

@@ -120,10 +120,6 @@ initializer_unittests_1_CXXFLAGS = $(AM_CXXFLAGS)
 initializer_unittests_1_LDADD    = $(AM_LDADD)
 initializer_unittests_1_LDFLAGS  = $(AM_LDFLAGS)
 
-TESTS += initializer_unittests_2
-initializer_unittests_2_SOURCES  = run_initializer_unittests.cc
-initializer_unittests_2_SOURCES += message_initializer_2_unittest.cc
-
 initializer_unittests_2_CPPFLAGS = $(AM_CPPFLAGS)
 initializer_unittests_2_CXXFLAGS = $(AM_CXXFLAGS)
 initializer_unittests_2_LDADD    = $(AM_LDADD)

src/lib/log/tests/message_initializer_1_unittest.cc

@@ -45,7 +45,7 @@ const char* values3[] = {
 };
 
 const char* values4[] = {
-    "GLOBAL8", "global message eight bis",
+    "GLOBAL8", "global message eight",
     "GLOBAL9", "global message nine",
     NULL
 };
@@ -119,11 +119,12 @@ TEST(MessageInitializerTest1, dynamicLoadUnload) {
     EXPECT_EQ("global message eight", global->getText("GLOBAL8"));
     EXPECT_EQ("global message nine", global->getText("GLOBAL9"));
 
-    // Destroy the first initializer. The first two messages should
-    // be unregistered.
+    // Destroy the first initializer. The first message should be removed.
+    // The second message should not be removed because it is also held
+    // by another object.
     init1.reset();
     EXPECT_TRUE(global->getText("GLOBAL7").empty());
-    EXPECT_TRUE(global->getText("GLOBAL8").empty());
+    EXPECT_EQ("global message eight", global->getText("GLOBAL8"));
     EXPECT_EQ("global message nine", global->getText("GLOBAL9"));
 
     // Destroy the second initializer. Now, all messages should be

src/lib/log/tests/message_initializer_2_unittest.cc

@@ -1,61 +0,0 @@
-// Copyright (C) 2012,2015  Internet Systems Consortium, Inc. ("ISC")
-//
-// Permission to use, copy, modify, and/or distribute this software for any
-// purpose with or without fee is hereby granted, provided that the above
-// copyright notice and this permission notice appear in all copies.
-//
-// THE SOFTWARE IS PROVIDED "AS IS" AND ISC DISCLAIMS ALL WARRANTIES WITH
-// REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY
-// AND FITNESS.  IN NO EVENT SHALL ISC BE LIABLE FOR ANY SPECIAL, DIRECT,
-// INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM
-// LOSS OF USE, DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE
-// OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR
-// PERFORMANCE OF THIS SOFTWARE.
-
-#include <config.h>
-
-#include <log/message_initializer.h>
-#include <gtest/gtest.h>
-
-#include <util/unittests/resource.h>
-#include <util/unittests/check_valgrind.h>
-
-using namespace isc::log;
-
-// Declare a set of messages to go into the global dictionary.
-
-namespace {
-const char* values[] = {
-    "GLOBAL1", "global message one",
-    "GLOBAL2", "global message two",
-    NULL
-};
-}
-
-TEST(MessageInitializerTest2, MessageLoadTest) {
-    // Create the list where the initializers will be held.
-    std::list<boost::shared_ptr<MessageInitializer> > initializers;
-
-    // Load the maximum number of message arrays allowed.  Some arrays may
-    // already have been loaded because of static initialization from modules
-    // in libraries linked against the test program, hence the reason for the
-    // loop starting from the value returned by getPendingCount() instead of 0
-    for (size_t i = MessageInitializer::getPendingCount();
-         i < MessageInitializer::MAX_MESSAGE_ARRAYS; ++i) {
-        boost::shared_ptr<MessageInitializer> initializer(new MessageInitializer(values));
-        initializers.push_back(initializer);
-    }
-
-    // Note: Not all systems have EXPECT_DEATH.  As it is a macro we can just
-    // test for its presence and bypass the test if not available.
-#ifdef EXPECT_DEATH
-    // Adding one more should take us over the limit.
-    if (!isc::util::unittests::runningOnValgrind()) {
-        EXPECT_DEATH({
-            isc::util::unittests::dontCreateCoreDumps();
-
-            MessageInitializer initializer(values);
-          }, ".*");
-    }
-#endif
-}