[checkmk-commits] Make TimeperiodsCache implementation less surprising.

Module: check_mk Branch: master Commit: f4bcc803f1eb9911c802d415dad5a23dfe9d5974 URL: http://git.mathias-kettner.de/git/?p=check_mk.git;a=commit;h=f4bcc803f1eb99… Author: Sven Panne &lt;sp(a)mathias-kettner.de&gt; Date: Thu Aug 17 09:19:32 2017 +0200 Make TimeperiodsCache implementation less surprising. Using time_t for *minutes* instead of the intended seconds is highly confusing. Furhtermore, modern C++ uses std::chrono facilities for stuff in this area. Various minor cleanups on the way. Change-Id: I4bf5082828ea64d392b563b8792b62708f73b398 --- livestatus/src/ChronoUtils.h | 6 +++ livestatus/src/TimeperiodsCache.cc | 76 +++++++++++++++----------------------- livestatus/src/TimeperiodsCache.h | 14 +++---- livestatus/src/module.cc | 5 +-- 4 files changed, 45 insertions(+), 56 deletions(-) diff --git a/livestatus/src/ChronoUtils.h b/livestatus/src/ChronoUtils.h index f1de078..1073138 100644 --- a/livestatus/src/ChronoUtils.h +++ b/livestatus/src/ChronoUtils.h @@ -64,6 +64,12 @@ inline timeval to_timeval(std::chrono::duration<Rep, Period> dur) { return tv; } +inline std::chrono::system_clock::time_point from_timeval(const timeval &tv) { + return std::chrono::system_clock::time_point( + std::chrono::seconds(tv.tv_sec) + + std::chrono::microseconds(tv.tv_usec)); +} + inline std::chrono::system_clock::time_point parse_time_t( const std::string &str) { return std::chrono::system_clock::from_time_t(atoi(str.c_str())); diff --git a/livestatus/src/TimeperiodsCache.cc b/livestatus/src/TimeperiodsCache.cc index 3921431..6742ee2 100644 --- a/livestatus/src/TimeperiodsCache.cc +++ b/livestatus/src/TimeperiodsCache.cc @@ -23,8 +23,8 @@ // Boston, MA 02110-1301 USA. #include "TimeperiodsCache.h" -#include <cstring> #include <ostream> +#include <ratio> #include <string> #include <utility> #include "Logger.h" @@ -32,24 +32,22 @@ using std::lock_guard; using std::mutex; using std::string; +using std::chrono::system_clock; +using std::chrono::minutes; extern timeperiod *timeperiod_list; -TimeperiodsCache::TimeperiodsCache(Logger *logger) - : _logger(logger), _cache_time(0) {} - -TimeperiodsCache::~TimeperiodsCache() = default; +TimeperiodsCache::TimeperiodsCache(Logger *logger) : _logger(logger) {} void TimeperiodsCache::logCurrentTimeperiods() { lock_guard<mutex> lg(_mutex); - time_t now = time(nullptr); - // Loop over all timeperiods and compute if we are - // currently in. Detect the case where no time periods - // are known (yet!). This might be the case when a timed - // event broker message arrives *before* the start of the - // event loop. + // Loop over all timeperiods and compute if we are currently in. Detect the + // case where no time periods are known (yet!). This might be the case when + // a timed event broker message arrives *before* the start of the event + // loop. + auto now = system_clock::to_time_t(system_clock::now()); for (timeperiod *tp = timeperiod_list; tp != nullptr; tp = tp->next) { - bool is_in = 0 == check_time_against_period(now, tp); + bool is_in = check_time_against_period(now, tp) == 0; // check previous state and log transition if state has changed auto it = _cache.find(tp); if (it == _cache.end()) { // first entry @@ -60,26 +58,22 @@ void TimeperiodsCache::logCurrentTimeperiods() { } } -void TimeperiodsCache::update(time_t now) { +void TimeperiodsCache::update(system_clock::time_point now) { lock_guard<mutex> lg(_mutex); - - // update cache only once a minute. The timeperiod - // definitions have 1 minute as granularity, so a - // 1sec resultion is not needed. - int minutes = now / 60; - if (minutes == _cache_time) { + // Update cache only once a minute. The timeperiod definitions have a + // 1-minute granularity, so a 1-second resultion is not needed. + if (now < _last_update + minutes(1)) { return; } + _last_update = now; - // Loop over all timeperiods and compute if we are - // currently in. Detect the case where no time periods - // are known (yet!). This might be the case when a timed - // event broker message arrives *before* the start of the - // event loop. - int num_periods = 0; + // Loop over all timeperiods and compute if we are currently in. Detect the + // case where no time periods are known (yet!). This might be the case when + // a timed event broker message arrives *before* the start of the event + // loop. for (timeperiod *tp = timeperiod_list; tp != nullptr; tp = tp->next) { - bool is_in = 0 == check_time_against_period(now, tp); - + bool is_in = + check_time_against_period(system_clock::to_time_t(now), tp) == 0; // check previous state and log transition if state has changed auto it = _cache.find(tp); if (it == _cache.end()) { // first entry @@ -89,43 +83,33 @@ void TimeperiodsCache::update(time_t now) { logTransition(tp->name, it->second ? 1 : 0, is_in ? 1 : 0); it->second = is_in; } - - num_periods++; } - if (num_periods > 0) { - _cache_time = minutes; - } else { + if (timeperiod_list != nullptr) { Informational(_logger) << "Timeperiod cache not updated, there are no timeperiods (yet)"; } } -bool TimeperiodsCache::inTimeperiod(const char *tpname) const { +bool TimeperiodsCache::inTimeperiod(const string &tpname) const { for (timeperiod *tp = timeperiod_list; tp != nullptr; tp = tp->next) { - if (strcmp(tpname, tp->name) == 0) { + if (tpname == tp->name) { return inTimeperiod(tp); } } return true; // unknown timeperiod is assumed to be 7X24 } -bool TimeperiodsCache::inTimeperiod(timeperiod *tp) const { +bool TimeperiodsCache::inTimeperiod(const timeperiod *tp) const { lock_guard<mutex> lg(_mutex); auto it = _cache.find(tp); - bool is_in; - if (it != _cache.end()) { - is_in = it->second; - } else { + if (it == _cache.end()) { + // Problem: check_time_against_period is not thread safe, so we can't + // use it here. Informational(_logger) << "No timeperiod information available for " << tp->name << ". Assuming out of period."; - is_in = false; - // Problem: The method check_time_against_period is to a high - // degree not thread safe. In the current situation Icinga is - // very probable to hang up forever. - // time_t now = time(0); - // is_in = 0 == check_time_against_period(now, tp); + return false; } - return is_in; + return it->second; } void TimeperiodsCache::logTransition(char *name, int from, int to) const { diff --git a/livestatus/src/TimeperiodsCache.h b/livestatus/src/TimeperiodsCache.h index 01c7440..b227169 100644 --- a/livestatus/src/TimeperiodsCache.h +++ b/livestatus/src/TimeperiodsCache.h @@ -26,27 +26,27 @@ #define TimeperiodsCache_h #include "config.h" // IWYU pragma: keep -#include <ctime> +#include <chrono> #include <map> #include <mutex> +#include <string> #include "nagios.h" class Logger; class TimeperiodsCache { public: explicit TimeperiodsCache(Logger *logger); - ~TimeperiodsCache(); - void update(time_t now); - bool inTimeperiod(timeperiod *tp) const; - bool inTimeperiod(const char *tpname) const; + void update(std::chrono::system_clock::time_point now); + bool inTimeperiod(const timeperiod *tp) const; + bool inTimeperiod(const std::string &tpname) const; void logCurrentTimeperiods(); private: Logger *const _logger; - // The mutex protects _cache_time and _cache. + // The mutex protects _last_update and _cache. mutable std::mutex _mutex; - time_t _cache_time; + std::chrono::system_clock::time_point _last_update; std::map<const timeperiod *, bool> _cache; void logTransition(char *name, int from, int to) const; diff --git a/livestatus/src/module.cc b/livestatus/src/module.cc index 1f8bbab..5040d89 100644 --- a/livestatus/src/module.cc +++ b/livestatus/src/module.cc @@ -32,7 +32,6 @@ #include <pthread.h> #include <sys/socket.h> #include <sys/stat.h> -#include <sys/time.h> #include <sys/un.h> #include <unistd.h> #include <chrono> @@ -575,7 +574,7 @@ int broker_event(int event_type __attribute__((__unused__)), void *data) { Informational(fl_logger_nagios) << "logging initial states"; } } - g_timeperiods_cache->update(ts->timestamp.tv_sec); + g_timeperiods_cache->update(from_timeval(ts->timestamp)); return 0; } @@ -758,7 +757,7 @@ int broker_process(int event_type __attribute__((__unused__)), void *data) { g_timeperiods_cache = new TimeperiodsCache(fl_logger_nagios); break; case NEBTYPE_PROCESS_EVENTLOOPSTART: - g_timeperiods_cache->update(time(nullptr)); + g_timeperiods_cache->update(from_timeval(ps->timestamp)); start_threads(); break; default: