Merge branch 'wip-deadlock' into 'master'

🐛 fix mutex deadlock

See merge request !15

src/ADDH.cpp

@@ -28,10 +28,15 @@ void ADDH::initConfigurations()
     handleExtraConfig();
 
     // Listen on updateServicesSignal in order to update extra config
-    ADDHExtraService::instance().updateServicesSignal.connect(
+    m_extraConn = ADDHExtraService::instance().updateServicesSignal.connect(
         [this]() { handleExtraConfig(); });
 }
 
+ADDH::~ADDH()
+{
+    m_extraConn.disconnect();
+}
+
 void ADDH::handleGlobalConfig()
 {
     LOG(INFO) << "Creating ADDH Clients from global configuration";
@@ -75,7 +80,7 @@ void ADDH::createClients(const ClientsConfigList &configs,
         {
             // Initialization of the handler for the client
             std::shared_ptr<ADDHClientHandler> handler(
-                new ADDHClientHandler(index++, data));
+                std::make_shared<ADDHClientHandler>(index++, data));
             // Connect error and warning signals
             handler->errorSignal.connect(errorSignal);
             handler->warningSignal.connect(warningSignal);
@@ -84,9 +89,9 @@ void ADDH::createClients(const ClientsConfigList &configs,
 
             if (data.type == DataSourceType::VALUE)
             {
-                std::shared_ptr<DimInfo> client(
-                    new DimInfo(data.fromService.c_str(), ((void *) NULL), 0,
-                                handler.get()));
+                std::shared_ptr<DimInfo> client(std::make_shared<DimInfo>(
+                    data.fromService.c_str(), ((void *) NULL), 0,
+                    handler.get()));
                 // client->itsHandler = handler.get();
                 addhClient = {handler, nullptr, client};
             }
@@ -94,7 +99,8 @@ void ADDH::createClients(const ClientsConfigList &configs,
             {
                 // Initialization of the client
                 std::shared_ptr<ntof::dim::DIMProxyClient> client(
-                    new ntof::dim::DIMProxyClient(data.fromService));
+                    std::make_shared<ntof::dim::DIMProxyClient>(
+                        data.fromService));
                 client->setHandler(handler.get());
 
                 addhClient = {handler, client, nullptr};

src/ADDH.h

@@ -3,8 +3,8 @@
 
 #include <mutex>
 
+#include <DIMUtils.hpp>
 #include <DaqTypes.h>
-#include <Synchro.h>
 
 #include "ADDHTypes.hpp"
 
@@ -20,6 +20,7 @@ class ADDH
 {
 public:
     ADDH() = default;
+    ~ADDH();
 
     /**
      *  @brief It loads global and extra configurations
@@ -65,6 +66,7 @@ protected:
     mutable std::mutex m_mutex; //!< Mutex used to avoid concurrent access
     ClientsMap m_clients;       //!< List of the clients
     ClientsMap m_extraClients;  //!< List of the clients of extra configuration
+    ntof::dim::scoped_connection m_extraConn; //!< Extra service connection
 };
 
 } // namespace addh

src/ADDHWriter.cpp

@@ -27,20 +27,34 @@ ADDHWriter::ADDHWriter() : m_commands("ADDHCommand", 100)
     DimClient::setDnsNode(Config::instance().getDimDns().c_str());
 
     DimServer::start(Config::instance().getValue("serverName", "ADDH").c_str());
+    initServices();
 }
 
 ADDHWriter::~ADDHWriter()
 {
     stop();
-    m_addhCmd.reset();
-    m_writer.reset();
-    m_addh.reset();
-    m_state.reset();
+    clearServices();
+}
+
+void ADDHWriter::clearServices()
+{
+    {
+        // safely disable any connections
+        std::lock_guard<std::mutex> lock(m_mutex);
+        m_conn.clear();
+    }
+
+    {
+        m_addhCmd.reset();
+        m_writer.reset();
+        m_addh.reset();
+        m_state.reset();
+    }
 }
 
 void ADDHWriter::initServices()
 {
-    // initialization
+    clearServices();
 
     // State
     m_state.reset(new ntof::dim::DIMState(
@@ -73,9 +87,6 @@ void ADDHWriter::initServices()
 
 void ADDHWriter::thread_func()
 {
-    if (!m_addh)
-        initServices();
-
     setState(STARTED);
 
     while (m_started.load())
@@ -96,13 +107,11 @@ void ADDHWriter::thread_func()
 
 void ADDHWriter::setState(int32_t val)
 {
-    std::lock_guard<std::mutex> lock(m_mutex);
     m_state->setValue(val);
 }
 
 void ADDHWriter::setError(int32_t code, const std::string &message)
 {
-    std::lock_guard<std::mutex> lock(m_mutex);
     if (message.empty())
     {
         if (m_state->removeError(code))
@@ -120,13 +129,11 @@ void ADDHWriter::setError(int32_t code, const std::string &message)
 
 void ADDHWriter::clearErrors()
 {
-    std::lock_guard<std::mutex> lock(m_mutex);
     m_state->clearErrors();
 }
 
 void ADDHWriter::setWarning(int32_t code, const std::string &message)
 {
-    std::lock_guard<std::mutex> lock(m_mutex);
     if (message.empty())
     {
         if (m_state->removeWarning(code))
@@ -144,7 +151,6 @@ void ADDHWriter::setWarning(int32_t code, const std::string &message)
 
 void ADDHWriter::clearWarnings()
 {
-    std::lock_guard<std::mutex> lock(m_mutex);
     m_state->clearWarnings();
 }
 

src/ADDHWriter.h

@@ -110,6 +110,11 @@ protected:
      */
     void actionWrite(ADDHCommand *command);
 
+    /**
+     * @brief clear internals
+     */
+    void clearServices();
+
     ntof::utils::Queue<ADDHCommand *> m_commands; //!< The list of the
                                                   //!< commands received by
                                                   //!< the ADDH Writer
@@ -120,6 +125,8 @@ protected:
 
     std::function<ErrorSignal::signature_type> m_errSlot;
     std::function<WarningSignal::signature_type> m_warnSlot;
+
+    // protected with Thread::m_mutex
     std::list<ntof::dim::scoped_connection> m_conn;
 };
 

src/main.cpp

@@ -57,7 +57,6 @@ int main(int argc, char **argv)
         // Run ADDH Writer
         LOG(INFO) << "ADDH Writer starting";
         ntof::addh::ADDHWriter addhWriter;
-        addhWriter.initServices();
         addhWriter.run();
     }
     catch (const ntof::addh::ADDHException &ex)

tests/test_helpers_dim.cpp

@@ -70,7 +70,14 @@ DimTestHelper::DimTestHelper()
 {
     const std::string dnsExe =
         (bfs::path(BUILDDIR) / "instroot" / "bin" / "dns").string();
-    m_pid = ::spawn(dnsExe.c_str(), "-d");
+    if (std::getenv("DNS_DEBUG") != nullptr)
+    {
+        m_pid = ::spawn(dnsExe.c_str(), "-d");
+    }
+    else
+    {
+        m_pid = ::spawn(dnsExe.c_str());
+    }
     CPPUNIT_ASSERT_MESSAGE("Failed to start DIM DNS", m_pid >= 0);
     boost::this_thread::sleep_for(boost::chrono::seconds(3));