From 9d526332d8963d057e70704fd928507b63e053bb Mon Sep 17 00:00:00 2001
From: Johan Wikman <johan.wikman@mariadb.com>
Date: Thu, 17 May 2018 14:21:34 +0300
Subject: [PATCH] MXS-1775 Ensure MonitorInstance::start() returns correct
 value

Since we need to call mysql_thread_init(), which can fail, in
the monitor thread, we need to wait for the outcome of that
before we can return from start().
---
 include/maxscale/monitor.hh | 14 ++++++++------
 server/core/monitor.cc      | 28 ++++++++++++++++++++--------
 2 files changed, 28 insertions(+), 14 deletions(-)

diff --git a/include/maxscale/monitor.hh b/include/maxscale/monitor.hh
index ff81b07c4..88bdf0e44 100644
--- a/include/maxscale/monitor.hh
+++ b/include/maxscale/monitor.hh
@@ -14,6 +14,7 @@
 
 #include <maxscale/cppdefs.hh>
 #include <maxscale/monitor.h>
+#include <maxscale/semaphore.hh>
 #include <maxscale/thread.h>
 
 namespace maxscale
@@ -44,12 +45,13 @@ protected:
     MXS_MONITORED_SERVER* m_master;   /**< Master server */
 
 private:
-    int32_t     m_status;   /**< The current status of the monitor. */
-    THREAD      m_thread;   /**< The thread handle of the monitoring thread. */
-    int32_t     m_shutdown; /**< Non-zero if the monitor should shut down. */
-    bool        m_checked;  /**< Whether server access has been checked. */
-    std::string m_script;   /**< Launchable script. */
-    uint64_t    m_events;   /**< Enabled monitor events. */
+    int32_t     m_status;    /**< The current status of the monitor. */
+    THREAD      m_thread;    /**< The thread handle of the monitoring thread. */
+    int32_t     m_shutdown;  /**< Non-zero if the monitor should shut down. */
+    bool        m_checked;   /**< Whether server access has been checked. */
+    std::string m_script;    /**< Launchable script. */
+    uint64_t    m_events;    /**< Enabled monitor events. */
+    Semaphore   m_semaphore; /**< Semaphore for synchronizing with monitor thread. */
 
     void main();
 
diff --git a/server/core/monitor.cc b/server/core/monitor.cc
index d982b106f..8725250fd 100644
--- a/server/core/monitor.cc
+++ b/server/core/monitor.cc
@@ -2542,6 +2542,7 @@ bool MonitorInstance::start(const MXS_CONFIG_PARAMETER* pParams)
 
     ss_dassert(!m_shutdown);
     ss_dassert(!m_thread);
+    ss_dassert(m_status == MXS_MONITOR_STOPPED);
 
     if (!m_checked)
     {
@@ -2569,7 +2570,19 @@ bool MonitorInstance::start(const MXS_CONFIG_PARAMETER* pParams)
         }
         else
         {
-            started = true;
+            // Ok, so the thread started. Let's wait until we can be certain the
+            // state has been updated.
+            m_semaphore.wait();
+
+            started = (atomic_load_int32(&m_status) == MXS_MONITOR_RUNNING);
+
+            if (!started)
+            {
+                // Ok, so the initialization failed and the thread will exit.
+                // We need to wait on it so that the thread resources will not leak.
+                thread_wait(m_thread);
+                m_thread = 0;
+            }
         }
     }
 
@@ -2587,8 +2600,6 @@ void MonitorInstance::configure(const MXS_CONFIG_PARAMETER* pParams)
 
 void MonitorInstance::main()
 {
-    atomic_store_int32(&m_status, MXS_MONITOR_RUNNING);
-
     load_server_journal(m_monitor, &m_master);
 
     while (!m_shutdown)
@@ -2622,8 +2633,6 @@ void MonitorInstance::main()
             ms += MXS_MON_BASE_INTERVAL_MS;
         }
     }
-
-    atomic_store_int32(&m_status, MXS_MONITOR_STOPPED);
 }
 
 //static
@@ -2633,17 +2642,20 @@ void MonitorInstance::main(void* pArg)
 
     if (mysql_thread_init() == 0)
     {
+        atomic_store_int32(&pThis->m_status, MXS_MONITOR_RUNNING);
+        pThis->m_semaphore.post();
+
         pThis->main();
 
+        atomic_store_int32(&pThis->m_status, MXS_MONITOR_STOPPED);
+
         mysql_thread_end();
     }
     else
     {
         MXS_ERROR("mysql_thread_init() failed for %s. The monitor cannot start.",
                   pThis->m_monitor->name);
-
-        // TODO: MaxScale now thinks the monitor is running, as startMonitor()
-        // TODO: already returned true.
+        pThis->m_semaphore.post();
     }
 }