Hide the MonitorInstance m_state field

The field had the same purpose as MXS_MONITOR->state. Now the field
is only used for checking if the MonitorInstance thread is running.

include/maxscale/monitor.h

@@ -237,7 +237,8 @@ struct mxs_monitor
     SPINLOCK lock;
     MXS_CONFIG_PARAMETER* parameters; /*< configuration parameters */
     MXS_MONITORED_SERVER* monitored_servers; /*< List of servers the monitor monitors */
-    monitor_state_t state;        /**< The state of the monitor */
+    monitor_state_t state;        /**< The state of the monitor. This should ONLY be written to by the admin
+                                   *   thread. */
     int connect_timeout;          /**< Connect timeout in seconds for mysql_real_connect */
     int connect_attempts;         /**< How many times a connection is attempted */
     int read_timeout;             /**< Timeout in seconds to read from the server.

include/maxscale/monitor.hh

@@ -13,6 +13,8 @@
  */
 
 #include <maxscale/ccdefs.hh>
+
+#include <atomic>
 #include <maxscale/monitor.h>
 #include <maxscale/semaphore.hh>
 #include <maxscale/worker.hh>
@@ -32,16 +34,14 @@ public:
     /**
      * @brief Current state of the monitor.
      *
-     * Note that in principle the state of the monitor may already have
+     * Since the state is written to by the admin thread, the value returned in other threads cannot be fully
-     * changed when the current state is returned. The state can be fully
+     * trusted. The state should only be read in the admin thread or operations launched by the admin thread.
-     * trusted only if it is asked in a context when it is known that nobody
-     * else can affect it.
      *
-     * @return @c MXS_MONITOR_RUNNING if the monitor is running,
+     * @return @c MONITOR_STATE_RUNNING if the monitor is running,
-     *         @c MXS_MONITOR_STOPPING if the monitor is stopping, and
+     *         @c MONITOR_STATE_STOPPING if the monitor is stopping, and
-     *         @c MXS_MONITOR_STOPPED of the monitor is stopped.
+     *         @c MONITOR_STATE_STOPPED if the monitor is stopped.
      */
-    int32_t state() const;
+    monitor_state_t monitor_state() const;
 
     /**
      * @brief Find out whether the monitor is running.
@@ -52,7 +52,7 @@ public:
      */
     bool is_running() const
     {
-        return state() == MONITOR_STATE_RUNNING;
+        return monitor_state() == MONITOR_STATE_RUNNING;
     }
 
     /**
@@ -208,7 +208,7 @@ protected:
     MXS_MONITORED_SERVER* m_master;   /**< Master server */
 
 private:
-    int32_t     m_state;       /**< The current state of the monitor. */
+    std::atomic<bool> m_thread_running; /**< Thread state. Only visible inside MonitorInstance. */
     int32_t     m_shutdown;             /**< Non-zero if the monitor should shut down. */
     bool        m_checked;              /**< Whether server access has been checked. */
     Semaphore   m_semaphore;            /**< Semaphore for synchronizing with monitor thread. */

server/core/monitor.cc

@@ -35,6 +35,7 @@
 #include <maxscale/mysql_utils.h>
 #include <maxscale/paths.h>
 #include <maxscale/pcre2.h>
+#include <maxscale/routingworker.h>
 #include <maxscale/secrets.h>
 #include <maxscale/spinlock.h>
 #include <maxscale/utils.hh>
@@ -2456,7 +2457,7 @@ namespace maxscale
 MonitorInstance::MonitorInstance(MXS_MONITOR* pMonitor)
     : m_monitor(pMonitor)
     , m_master(NULL)
-    , m_state(MONITOR_STATE_STOPPED)
+    , m_thread_running(false)
     , m_shutdown(0)
     , m_checked(false)
     , m_loop_called(0)
@@ -2467,27 +2468,25 @@ MonitorInstance::~MonitorInstance()
 {
 }
 
-int32_t MonitorInstance::state() const
+monitor_state_t MonitorInstance::monitor_state() const
 {
-    return atomic_load_int32(&m_state);
+    static_assert(sizeof(monitor_state_t) == 4, "Unexpected size for enum");
+    return (monitor_state_t)atomic_load_uint32((uint32_t*)(&m_monitor->state));
 }
 
 void MonitorInstance::stop()
 {
-    // This is always called in single-thread context.
+    // This should only be called by monitor_stop(). NULL worker is allowed since the main worker may
-    ss_dassert(m_state == MONITOR_STATE_RUNNING);
+    // not exist during program start/stop.
+    ss_dassert(mxs_rworker_get_current() == NULL ||
+               mxs_rworker_get_current() == mxs_rworker_get(MXS_RWORKER_MAIN));
+    ss_dassert(Worker::state() != Worker::STOPPED);
+    ss_dassert(monitor_state() == MONITOR_STATE_STOPPING);
+    ss_dassert(m_thread_running.load() == true);
 
-    if (state() == MONITOR_STATE_RUNNING)
-    {
-        atomic_store_int32(&m_state, MONITOR_STATE_STOPPING);
     Worker::shutdown();
     Worker::join();
-        atomic_store_int32(&m_state, MONITOR_STATE_STOPPED);
+    m_thread_running.store(false, std::memory_order_release);
-    }
-    else
-    {
-        MXS_WARNING("An attempt was made to stop a monitor that is not running.");
-    }
 }
 
 void MonitorInstance::diagnostics(DCB* pDcb) const
@@ -2501,13 +2500,14 @@ json_t* MonitorInstance::diagnostics_json() const
 
 bool MonitorInstance::start(const MXS_CONFIG_PARAMETER* pParams)
 {
-    bool started = false;
+    // This should only be called by monitor_start(). NULL worker is allowed since the main worker may
-
+    // not exist during program start/stop.
+    ss_dassert(mxs_rworker_get_current() == NULL ||
+               mxs_rworker_get_current() == mxs_rworker_get(MXS_RWORKER_MAIN));
     ss_dassert(Worker::state() == Worker::STOPPED);
-    ss_dassert(m_state == MONITOR_STATE_STOPPED);
+    ss_dassert(monitor_state() == MONITOR_STATE_STOPPED);
+    ss_dassert(m_thread_running.load() == false);
 
-    if (state() == MONITOR_STATE_STOPPED)
-    {
     if (!m_checked)
     {
         if (!has_sufficient_permissions())
@@ -2520,6 +2520,7 @@ bool MonitorInstance::start(const MXS_CONFIG_PARAMETER* pParams)
         }
     }
 
+    bool started = false;
     if (m_checked)
     {
         m_master = NULL;
@@ -2538,8 +2539,7 @@ bool MonitorInstance::start(const MXS_CONFIG_PARAMETER* pParams)
                 // state has been updated.
                 m_semaphore.wait();
 
-                    started = (atomic_load_int32(&m_state) == MONITOR_STATE_RUNNING);
+                started = m_thread_running.load(std::memory_order_acquire);
-
                 if (!started)
                 {
                     // Ok, so the initialization failed and the thread will exit.
@@ -2549,15 +2549,6 @@ bool MonitorInstance::start(const MXS_CONFIG_PARAMETER* pParams)
             }
         }
     }
-    }
-    else
-    {
-        MXS_WARNING("An attempt was made to start a monitor that is already running.");
-        // Likely to cause the least amount of damage if we pretend the monitor
-        // was started.
-        started = true;
-    }
-
     return started;
 }
 
@@ -2841,14 +2832,12 @@ bool MonitorInstance::pre_run()
     if (mysql_thread_init() == 0)
     {
         rv = true;
-
+        // Write and post the semaphore to signal the admin thread that the start is succeeding.
-        atomic_store_int32(&m_state, MONITOR_STATE_RUNNING);
+        m_thread_running.store(true, std::memory_order_release);
         m_semaphore.post();
 
         load_server_journal(m_monitor, &m_master);
-
         pre_loop();
-
         delayed_call(1, &MonitorInstance::call_run_one_tick, this);
     }
     else

server/modules/monitor/mariadbmon/mariadbmon.cc

@@ -1014,7 +1014,7 @@ bool MariaDBMonitor::check_sql_files()
 bool MariaDBMonitor::execute_manual_command(std::function<void (void)> command, json_t** error_out)
 {
     bool rval = false;
-    if (state() != MONITOR_STATE_RUNNING)
+    if (monitor_state() != MONITOR_STATE_RUNNING)
     {
         PRINT_MXS_JSON_ERROR(error_out, "The monitor is not running, cannot execute manual command.");
     }