From 57706e7758d95af098c742c8fe65a8bb7415bc6b Mon Sep 17 00:00:00 2001 From: Esa Korhonen Date: Wed, 15 Aug 2018 15:22:53 +0300 Subject: [PATCH] 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 | 9 +- include/maxscale/monitor.hh | 28 ++--- server/core/monitor.cc | 115 ++++++++---------- .../modules/monitor/mariadbmon/mariadbmon.cc | 2 +- 4 files changed, 72 insertions(+), 82 deletions(-) diff --git a/include/maxscale/monitor.h b/include/maxscale/monitor.h index 769cc5adf..d0b178161 100644 --- a/include/maxscale/monitor.h +++ b/include/maxscale/monitor.h @@ -237,12 +237,13 @@ 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 connect_attempts; /**< How many times a connection is attempted */ int read_timeout; /**< Timeout in seconds to read from the server. - * There are retries and the total effective timeout - * value is three times the option value. + * There are retries and the total effective timeout + * value is three times the option value. */ int write_timeout; /**< Timeout in seconds for each attempt to write to the server. * There are retries and the total effective timeout value is diff --git a/include/maxscale/monitor.hh b/include/maxscale/monitor.hh index fa176d455..72158bc7a 100644 --- a/include/maxscale/monitor.hh +++ b/include/maxscale/monitor.hh @@ -13,6 +13,8 @@ */ #include + +#include #include #include #include @@ -32,16 +34,14 @@ public: /** * @brief Current state of the monitor. * - * Note that in principle the state of the monitor may already have - * changed when the current state is returned. The state can be fully - * trusted only if it is asked in a context when it is known that nobody - * else can affect it. + * Since the state is written to by the admin thread, the value returned in other threads cannot be fully + * trusted. The state should only be read in the admin thread or operations launched by the admin thread. * - * @return @c MXS_MONITOR_RUNNING if the monitor is running, - * @c MXS_MONITOR_STOPPING if the monitor is stopping, and - * @c MXS_MONITOR_STOPPED of the monitor is stopped. + * @return @c MONITOR_STATE_RUNNING if the monitor is running, + * @c MONITOR_STATE_STOPPING if the monitor is stopping, and + * @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,11 +208,11 @@ protected: MXS_MONITORED_SERVER* m_master; /**< Master server */ private: - int32_t m_state; /**< The current state of the monitor. */ - 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. */ - int64_t m_loop_called; /**< When was the loop called the last time. */ + std::atomic 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. */ + int64_t m_loop_called; /**< When was the loop called the last time. */ bool pre_run() final; void post_run() final; diff --git a/server/core/monitor.cc b/server/core/monitor.cc index b4f543713..78a1f4a7d 100644 --- a/server/core/monitor.cc +++ b/server/core/monitor.cc @@ -35,6 +35,7 @@ #include #include #include +#include #include #include #include @@ -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. - ss_dassert(m_state == MONITOR_STATE_RUNNING); + // This should only be called by monitor_stop(). 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(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); - } - else - { - MXS_WARNING("An attempt was made to stop a monitor that is not running."); - } + Worker::shutdown(); + Worker::join(); + m_thread_running.store(false, std::memory_order_release); } void MonitorInstance::diagnostics(DCB* pDcb) const @@ -2501,63 +2500,55 @@ 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 (!m_checked) + if (!has_sufficient_permissions()) { - if (!has_sufficient_permissions()) + MXS_ERROR("Failed to start monitor. See earlier errors for more information."); + } + else + { + m_checked = true; + } + } + + bool started = false; + if (m_checked) + { + m_master = NULL; + + if (configure(pParams)) + { + m_loop_called = 0; + + if (!Worker::start()) { - MXS_ERROR("Failed to start monitor. See earlier errors for more information."); + MXS_ERROR("Failed to start worker for monitor '%s'.", m_monitor->name); } else { - m_checked = true; - } - } + // Ok, so the thread started. Let's wait until we can be certain the + // state has been updated. + m_semaphore.wait(); - if (m_checked) - { - m_master = NULL; - - if (configure(pParams)) - { - m_loop_called = 0; - - if (!Worker::start()) + started = m_thread_running.load(std::memory_order_acquire); + if (!started) { - MXS_ERROR("Failed to start worker for monitor '%s'.", m_monitor->name); - } - else - { - // 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_state) == MONITOR_STATE_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. - Worker::join(); - } + // Ok, so the initialization failed and the thread will exit. + // We need to wait on it so that the thread resources will not leak. + Worker::join(); } } } } - 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; - - atomic_store_int32(&m_state, MONITOR_STATE_RUNNING); + // Write and post the semaphore to signal the admin thread that the start is succeeding. + 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 diff --git a/server/modules/monitor/mariadbmon/mariadbmon.cc b/server/modules/monitor/mariadbmon/mariadbmon.cc index dd995f761..0634bae55 100644 --- a/server/modules/monitor/mariadbmon/mariadbmon.cc +++ b/server/modules/monitor/mariadbmon/mariadbmon.cc @@ -1014,7 +1014,7 @@ bool MariaDBMonitor::check_sql_files() bool MariaDBMonitor::execute_manual_command(std::function 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."); }