From e3c75faabc233e997c8f0a7463dd44455e87c694 Mon Sep 17 00:00:00 2001 From: Gunnar Beutner Date: Wed, 11 Nov 2015 10:21:30 +0100 Subject: [PATCH] Implement support for recursive object locks fixes #10596 --- lib/base/CMakeLists.txt | 2 +- lib/base/application.cpp | 2 - lib/base/array.cpp | 11 --- lib/base/configobject.cpp | 6 -- lib/base/configtype.cpp | 4 +- lib/base/dictionary.cpp | 9 --- lib/base/object.cpp | 5 -- lib/base/object.hpp | 12 +-- lib/base/objectlock.hpp | 5 +- lib/base/thinmutex.cpp | 66 ---------------- lib/base/thinmutex.hpp | 128 ------------------------------- lib/base/timer.cpp | 14 ---- lib/config/configitem.cpp | 2 - lib/db_ido/dbtype.cpp | 2 - lib/icinga/checkable-check.cpp | 5 +- lib/icinga/icingaapplication.cpp | 2 - lib/icinga/notification.cpp | 6 -- 17 files changed, 6 insertions(+), 275 deletions(-) delete mode 100644 lib/base/thinmutex.cpp delete mode 100644 lib/base/thinmutex.hpp diff --git a/lib/base/CMakeLists.txt b/lib/base/CMakeLists.txt index 4689c0677..9f21ae19c 100644 --- a/lib/base/CMakeLists.txt +++ b/lib/base/CMakeLists.txt @@ -34,7 +34,7 @@ set(base_SOURCES function.cpp function-script.cpp functionwrapper.cpp scriptglobal.cpp scriptutils.cpp serializer.cpp socket.cpp socketevents.cpp stacktrace.cpp statsfunction.cpp stdiostream.cpp stream.cpp streamlogger.cpp streamlogger.thpp string.cpp string-script.cpp - sysloglogger.cpp sysloglogger.thpp tcpsocket.cpp thinmutex.cpp threadpool.cpp timer.cpp + sysloglogger.cpp sysloglogger.thpp tcpsocket.cpp threadpool.cpp timer.cpp tlsstream.cpp tlsutility.cpp type.cpp typetype-script.cpp unixsocket.cpp utility.cpp value.cpp value-operators.cpp workqueue.cpp ) diff --git a/lib/base/application.cpp b/lib/base/application.cpp index 3866836b4..1000c1a67 100644 --- a/lib/base/application.cpp +++ b/lib/base/application.cpp @@ -982,7 +982,6 @@ int Application::Run(void) */ void Application::UpdatePidFile(const String& filename, pid_t pid) { - ASSERT(!OwnsLock()); ObjectLock olock(this); if (m_PidFile != NULL) @@ -1038,7 +1037,6 @@ void Application::UpdatePidFile(const String& filename, pid_t pid) */ void Application::ClosePidFile(bool unlink) { - ASSERT(!OwnsLock()); ObjectLock olock(this); if (m_PidFile != NULL) diff --git a/lib/base/array.cpp b/lib/base/array.cpp index 638f16340..4beb809ab 100644 --- a/lib/base/array.cpp +++ b/lib/base/array.cpp @@ -37,7 +37,6 @@ REGISTER_PRIMITIVE_TYPE(Array, Object, Array::GetPrototype()); */ Value Array::Get(unsigned int index) const { - ASSERT(!OwnsLock()); ObjectLock olock(this); return m_Data.at(index); @@ -51,7 +50,6 @@ Value Array::Get(unsigned int index) const */ void Array::Set(unsigned int index, const Value& value) { - ASSERT(!OwnsLock()); ObjectLock olock(this); m_Data.at(index) = value; @@ -64,7 +62,6 @@ void Array::Set(unsigned int index, const Value& value) */ void Array::Add(const Value& value) { - ASSERT(!OwnsLock()); ObjectLock olock(this); m_Data.push_back(value); @@ -77,7 +74,6 @@ void Array::Add(const Value& value) */ size_t Array::GetLength(void) const { - ASSERT(!OwnsLock()); ObjectLock olock(this); return m_Data.size(); @@ -91,7 +87,6 @@ size_t Array::GetLength(void) const */ bool Array::Contains(const Value& value) const { - ASSERT(!OwnsLock()); ObjectLock olock(this); return (std::find(m_Data.begin(), m_Data.end(), value) != m_Data.end()); @@ -105,7 +100,6 @@ bool Array::Contains(const Value& value) const */ void Array::Insert(unsigned int index, const Value& value) { - ASSERT(!OwnsLock()); ObjectLock olock(this); ASSERT(index <= m_Data.size()); @@ -120,7 +114,6 @@ void Array::Insert(unsigned int index, const Value& value) */ void Array::Remove(unsigned int index) { - ASSERT(!OwnsLock()); ObjectLock olock(this); m_Data.erase(m_Data.begin() + index); @@ -140,7 +133,6 @@ void Array::Remove(Array::Iterator it) void Array::Resize(size_t new_size) { - ASSERT(!OwnsLock()); ObjectLock olock(this); m_Data.resize(new_size); @@ -148,7 +140,6 @@ void Array::Resize(size_t new_size) void Array::Clear(void) { - ASSERT(!OwnsLock()); ObjectLock olock(this); m_Data.clear(); @@ -156,7 +147,6 @@ void Array::Clear(void) void Array::Reserve(size_t new_size) { - ASSERT(!OwnsLock()); ObjectLock olock(this); m_Data.reserve(new_size); @@ -164,7 +154,6 @@ void Array::Reserve(size_t new_size) void Array::CopyTo(const Array::Ptr& dest) const { - ASSERT(!OwnsLock()); ObjectLock olock(this); ObjectLock xlock(dest); diff --git a/lib/base/configobject.cpp b/lib/base/configobject.cpp index ff23965f2..9b59f69f0 100644 --- a/lib/base/configobject.cpp +++ b/lib/base/configobject.cpp @@ -378,7 +378,6 @@ void ConfigObject::Start(bool runtimeCreated) { ObjectImpl::Start(runtimeCreated); - ASSERT(!OwnsLock()); ObjectLock olock(this); SetStartCalled(true); @@ -388,8 +387,6 @@ void ConfigObject::Activate(bool runtimeCreated) { CONTEXT("Activating object '" + GetName() + "' of type '" + GetType()->GetName() + "'"); - ASSERT(!OwnsLock()); - Start(runtimeCreated); ASSERT(GetStartCalled()); @@ -409,7 +406,6 @@ void ConfigObject::Stop(bool runtimeRemoved) { ObjectImpl::Stop(runtimeRemoved); - ASSERT(!OwnsLock()); ObjectLock olock(this); SetStopCalled(true); @@ -419,8 +415,6 @@ void ConfigObject::Deactivate(bool runtimeRemoved) { CONTEXT("Deactivating object '" + GetName() + "' of type '" + GetType()->GetName() + "'"); - ASSERT(!OwnsLock()); - SetAuthority(false); { diff --git a/lib/base/configtype.cpp b/lib/base/configtype.cpp index 90555f992..aede58dbe 100644 --- a/lib/base/configtype.cpp +++ b/lib/base/configtype.cpp @@ -28,9 +28,7 @@ using namespace icinga; ConfigType::ConfigType(const String& name) : m_Name(name) -{ - InflateMutex(); -} +{ } ConfigType::Ptr ConfigType::GetByName(const String& name) { diff --git a/lib/base/dictionary.cpp b/lib/base/dictionary.cpp index 372604e63..a62bddb3d 100644 --- a/lib/base/dictionary.cpp +++ b/lib/base/dictionary.cpp @@ -36,7 +36,6 @@ REGISTER_PRIMITIVE_TYPE(Dictionary, Object, Dictionary::GetPrototype()); */ Value Dictionary::Get(const String& key) const { - ASSERT(!OwnsLock()); ObjectLock olock(this); std::map::const_iterator it = m_Data.find(key); @@ -56,7 +55,6 @@ Value Dictionary::Get(const String& key) const */ bool Dictionary::Get(const String& key, Value *result) const { - ASSERT(!OwnsLock()); ObjectLock olock(this); std::map::const_iterator it = m_Data.find(key); @@ -76,7 +74,6 @@ bool Dictionary::Get(const String& key, Value *result) const */ void Dictionary::Set(const String& key, const Value& value) { - ASSERT(!OwnsLock()); ObjectLock olock(this); m_Data[key] = value; @@ -90,7 +87,6 @@ void Dictionary::Set(const String& key, const Value& value) */ size_t Dictionary::GetLength(void) const { - ASSERT(!OwnsLock()); ObjectLock olock(this); return m_Data.size(); @@ -104,7 +100,6 @@ size_t Dictionary::GetLength(void) const */ bool Dictionary::Contains(const String& key) const { - ASSERT(!OwnsLock()); ObjectLock olock(this); return (m_Data.find(key) != m_Data.end()); @@ -117,7 +112,6 @@ bool Dictionary::Contains(const String& key) const */ void Dictionary::Remove(const String& key) { - ASSERT(!OwnsLock()); ObjectLock olock(this); Dictionary::Iterator it; @@ -134,7 +128,6 @@ void Dictionary::Remove(const String& key) */ void Dictionary::Clear(void) { - ASSERT(!OwnsLock()); ObjectLock olock(this); m_Data.clear(); @@ -142,7 +135,6 @@ void Dictionary::Clear(void) void Dictionary::CopyTo(const Dictionary::Ptr& dest) const { - ASSERT(!OwnsLock()); ObjectLock olock(this); BOOST_FOREACH(const Dictionary::Pair& kv, m_Data) { @@ -188,7 +180,6 @@ Object::Ptr Dictionary::Clone(void) const */ std::vector Dictionary::GetKeys(void) const { - ASSERT(!OwnsLock()); ObjectLock olock(this); std::vector keys; diff --git a/lib/base/object.cpp b/lib/base/object.cpp index b15965ca7..267f0ddcd 100644 --- a/lib/base/object.cpp +++ b/lib/base/object.cpp @@ -71,11 +71,6 @@ bool Object::OwnsLock(void) const } #endif /* I2_DEBUG */ -void Object::InflateMutex(void) -{ - m_Mutex.Inflate(); -} - void Object::SetField(int id, const Value&, bool, const Value&) { if (id == 0) diff --git a/lib/base/object.hpp b/lib/base/object.hpp index 12bd2d6c1..e3fb48194 100644 --- a/lib/base/object.hpp +++ b/lib/base/object.hpp @@ -22,16 +22,8 @@ #include "base/i2-base.hpp" #include "base/debug.hpp" -#include "base/thinmutex.hpp" - -#ifndef I2_DEBUG -#include -#else /* I2_DEBUG */ #include -#endif /* I2_DEBUG */ - #include - #include using boost::intrusive_ptr; @@ -115,8 +107,6 @@ public: bool OwnsLock(void) const; #endif /* I2_DEBUG */ - void InflateMutex(void); - static Object::Ptr GetPrototype(void); virtual Object::Ptr Clone(void) const; @@ -128,7 +118,7 @@ private: Object& operator=(const Object& rhs); uintptr_t m_References; - mutable ThinMutex m_Mutex; + mutable boost::recursive_mutex m_Mutex; #ifdef I2_DEBUG # ifndef _WIN32 diff --git a/lib/base/objectlock.hpp b/lib/base/objectlock.hpp index 1e4dda07b..b59b65fa6 100644 --- a/lib/base/objectlock.hpp +++ b/lib/base/objectlock.hpp @@ -56,9 +56,8 @@ public: inline void Lock(void) { ASSERT(!m_Locked && m_Object != NULL); - ASSERT(!m_Object->OwnsLock()); - m_Object->m_Mutex.Lock(); + m_Object->m_Mutex.lock(); m_Locked = true; #ifdef I2_DEBUG @@ -83,7 +82,7 @@ public: #endif /* I2_DEBUG */ if (m_Locked) { - m_Object->m_Mutex.Unlock(); + m_Object->m_Mutex.unlock(); m_Locked = false; } } diff --git a/lib/base/thinmutex.cpp b/lib/base/thinmutex.cpp deleted file mode 100644 index 29f9826df..000000000 --- a/lib/base/thinmutex.cpp +++ /dev/null @@ -1,66 +0,0 @@ -/****************************************************************************** - * Icinga 2 * - * Copyright (C) 2012-2013 Icinga Development Team (http://www.icinga.org/) * - * * - * This program is free software; you can redistribute it and/or * - * modify it under the terms of the GNU General Public License * - * as published by the Free Software Foundation; either version 2 * - * of the License, or (at your option) any later version. * - * * - * This program is distributed in the hope that it will be useful, * - * but WITHOUT ANY WARRANTY; without even the implied warranty of * - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the * - * GNU General Public License for more details. * - * * - * You should have received a copy of the GNU General Public License * - * along with this program; if not, write to the Free Software Foundation * - * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301, USA. * - ******************************************************************************/ - -#include "base/thinmutex.hpp" -#include "base/timer.hpp" -#include "base/convert.hpp" -#include "base/logger.hpp" - -using namespace icinga; - -/** - * Locks the mutex and inflates the lock. - */ -void ThinMutex::LockSlowPath(void) -{ - unsigned int it = 0; - -#ifdef _WIN32 -# ifdef _WIN64 - while (InterlockedCompareExchange64(&m_Data, THINLOCK_LOCKED, THINLOCK_UNLOCKED) != THINLOCK_UNLOCKED) { -# else /* _WIN64 */ - while (InterlockedCompareExchange(&m_Data, THINLOCK_LOCKED, THINLOCK_UNLOCKED) != THINLOCK_UNLOCKED) { -# endif /* _WIN64 */ -#else /* _WIN32 */ - while (!__sync_bool_compare_and_swap(&m_Data, THINLOCK_UNLOCKED, THINLOCK_LOCKED)) { -#endif /* _WIN32 */ - if (m_Data > THINLOCK_LOCKED) { - boost::mutex *mtx = reinterpret_cast(m_Data); - mtx->lock(); - - return; - } - - Spin(it); - it++; - } - - boost::mutex *mtx = new boost::mutex(); - mtx->lock(); -#ifdef _WIN32 -# ifdef _WIN64 - InterlockedCompareExchange64(&m_Data, reinterpret_cast(mtx), THINLOCK_LOCKED); -# else /* _WIN64 */ - InterlockedCompareExchange(&m_Data, reinterpret_cast(mtx), THINLOCK_LOCKED); -# endif /* _WIN64 */ -#else /* _WIN32 */ - __sync_bool_compare_and_swap(&m_Data, THINLOCK_LOCKED, reinterpret_cast(mtx)); -#endif /* _WIN32 */ -} - diff --git a/lib/base/thinmutex.hpp b/lib/base/thinmutex.hpp deleted file mode 100644 index 6d03781de..000000000 --- a/lib/base/thinmutex.hpp +++ /dev/null @@ -1,128 +0,0 @@ -/****************************************************************************** - * Icinga 2 * - * Copyright (C) 2012-2013 Icinga Development Team (http://www.icinga.org/) * - * * - * This program is free software; you can redistribute it and/or * - * modify it under the terms of the GNU General Public License * - * as published by the Free Software Foundation; either version 2 * - * of the License, or (at your option) any later version. * - * * - * This program is distributed in the hope that it will be useful, * - * but WITHOUT ANY WARRANTY; without even the implied warranty of * - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the * - * GNU General Public License for more details. * - * * - * You should have received a copy of the GNU General Public License * - * along with this program; if not, write to the Free Software Foundation * - * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301, USA. * - ******************************************************************************/ - -#ifndef THINMUTEX_H -#define THINMUTEX_H - -#include "base/i2-base.hpp" -#include -#ifndef _WIN32 -#include -#endif /* _WIN32 */ - -#if defined(_MSC_VER) && _MSC_VER >= 1310 && (defined(_M_IX86) || defined(_M_X64)) -extern "C" void _mm_pause(); -#pragma intrinsic(_mm_pause) -#define SPIN_PAUSE() _mm_pause() -#elif defined(__GNUC__) && (defined(__i386__) || defined(__x86_64__)) -#define SPIN_PAUSE() __asm__ __volatile__("rep; nop" : : : "memory") -#endif - -#define THINLOCK_UNLOCKED 0 -#define THINLOCK_LOCKED 1 - -namespace icinga { - -class I2_BASE_API ThinMutex -{ -public: - inline ThinMutex(void) - : m_Data(THINLOCK_UNLOCKED) - { } - - inline ~ThinMutex(void) - { - if (m_Data > THINLOCK_LOCKED) - delete reinterpret_cast(m_Data); - } - - inline void Lock(void) - { -#ifdef _WIN32 -# ifdef _WIN64 - if (InterlockedCompareExchange64(&m_Data, THINLOCK_LOCKED, THINLOCK_UNLOCKED) != THINLOCK_UNLOCKED) { -# else /* _WIN64 */ - if (InterlockedCompareExchange(&m_Data, THINLOCK_LOCKED, THINLOCK_UNLOCKED) != THINLOCK_UNLOCKED) { -# endif /* _WIN64 */ -#else /* _WIN32 */ - if (!__sync_bool_compare_and_swap(&m_Data, THINLOCK_UNLOCKED, THINLOCK_LOCKED)) { -#endif /* _WIN32 */ - LockSlowPath(); - } - } - - inline void Unlock(void) - { -#ifdef _WIN32 -# ifdef _WIN64 - if (InterlockedCompareExchange64(&m_Data, THINLOCK_UNLOCKED, THINLOCK_LOCKED) != THINLOCK_LOCKED) { -# else /* _WIN64 */ - if (InterlockedCompareExchange(&m_Data, THINLOCK_UNLOCKED, THINLOCK_LOCKED) != THINLOCK_LOCKED) { -# endif /* _WIN64 */ -#else /* _WIN32 */ - if (!__sync_bool_compare_and_swap(&m_Data, THINLOCK_LOCKED, THINLOCK_UNLOCKED)) { -#endif /* _WIN32 */ - boost::mutex *mtx = reinterpret_cast(m_Data); - mtx->unlock(); - } - } - - inline void Inflate(void) - { - LockSlowPath(); - Unlock(); - } - -private: - inline void Spin(unsigned int it) - { - if (it < 8) { - /* Do nothing. */ - } -#ifdef SPIN_PAUSE - else if (it < 16) { - SPIN_PAUSE(); - } -#endif /* SPIN_PAUSE */ - else { -#ifdef _WIN32 - Sleep(0); -#else /* _WIN32 */ - sched_yield(); -#endif /* _WIN32 */ - } - } - -private: -#ifdef _WIN32 -# ifdef _WIN64 - LONGLONG m_Data; -# else /* _WIN64 */ - LONG m_Data; -# endif /* _WIN64 */ -#else /* _WIN32 */ - uintptr_t m_Data; -#endif /* _WIN32 */ - - void LockSlowPath(void); -}; - -} - -#endif /* THINMUTEX_H */ diff --git a/lib/base/timer.cpp b/lib/base/timer.cpp index 89cb3d79f..8f9b3a904 100644 --- a/lib/base/timer.cpp +++ b/lib/base/timer.cpp @@ -90,8 +90,6 @@ void Timer::Uninitialize(void) */ void Timer::Call(void) { - ASSERT(!OwnsLock()); - try { OnTimerExpired(Timer::Ptr(this)); } catch (...) { @@ -110,8 +108,6 @@ void Timer::Call(void) */ void Timer::SetInterval(double interval) { - ASSERT(!OwnsLock()); - boost::mutex::scoped_lock lock(l_TimerMutex); m_Interval = interval; } @@ -123,8 +119,6 @@ void Timer::SetInterval(double interval) */ double Timer::GetInterval(void) const { - ASSERT(!OwnsLock()); - boost::mutex::scoped_lock lock(l_TimerMutex); return m_Interval; } @@ -134,8 +128,6 @@ double Timer::GetInterval(void) const */ void Timer::Start(void) { - ASSERT(!OwnsLock()); - { boost::mutex::scoped_lock lock(l_TimerMutex); m_Started = true; @@ -149,8 +141,6 @@ void Timer::Start(void) */ void Timer::Stop(bool wait) { - ASSERT(!OwnsLock()); - if (l_StopTimerThread) return; @@ -180,8 +170,6 @@ void Timer::Reschedule(double next) */ void Timer::InternalReschedule(bool completed, double next) { - ASSERT(!OwnsLock()); - boost::mutex::scoped_lock lock(l_TimerMutex); if (completed) @@ -214,8 +202,6 @@ void Timer::InternalReschedule(bool completed, double next) */ double Timer::GetNext(void) const { - ASSERT(!OwnsLock()); - boost::mutex::scoped_lock lock(l_TimerMutex); return m_Next; } diff --git a/lib/config/configitem.cpp b/lib/config/configitem.cpp index 621c75675..4bc0e6418 100644 --- a/lib/config/configitem.cpp +++ b/lib/config/configitem.cpp @@ -157,8 +157,6 @@ public: */ ConfigObject::Ptr ConfigItem::Commit(bool discard) { - ASSERT(!OwnsLock()); - #ifdef I2_DEBUG Log(LogDebug, "ConfigItem") << "Commit called for ConfigItem Type=" << GetType() << ", Name=" << GetName(); diff --git a/lib/db_ido/dbtype.cpp b/lib/db_ido/dbtype.cpp index d4890b13f..8e5284e57 100644 --- a/lib/db_ido/dbtype.cpp +++ b/lib/db_ido/dbtype.cpp @@ -83,8 +83,6 @@ DbType::Ptr DbType::GetByID(long tid) DbObject::Ptr DbType::GetOrCreateObjectByName(const String& name1, const String& name2) { - ASSERT(!OwnsLock()); - ObjectLock olock(this); DbType::ObjectMap::const_iterator it = m_Objects.find(std::make_pair(name1, name2)); diff --git a/lib/icinga/checkable-check.cpp b/lib/icinga/checkable-check.cpp index 17c213191..39da8495c 100644 --- a/lib/icinga/checkable-check.cpp +++ b/lib/icinga/checkable-check.cpp @@ -151,7 +151,6 @@ void Checkable::ProcessCheckResult(const CheckResult::Ptr& cr, const MessageOrig bool reachable = IsReachable(); bool notification_reachable = IsReachable(DependencyNotification); - ASSERT(!OwnsLock()); ObjectLock olock(this); CheckResult::Ptr old_cr = GetLastCheckResult(); @@ -383,12 +382,10 @@ void Checkable::ExecuteRemoteCheck(const Dictionary::Ptr& resolvedMacros) GetCheckCommand()->Execute(this, cr, resolvedMacros, true); } -void Checkable::ExecuteCheck() +void Checkable::ExecuteCheck(void) { CONTEXT("Executing check for object '" + GetName() + "'"); - ASSERT(!OwnsLock()); - UpdateNextCheck(); bool reachable = IsReachable(); diff --git a/lib/icinga/icingaapplication.cpp b/lib/icinga/icingaapplication.cpp index 0320460c3..36543e725 100644 --- a/lib/icinga/icingaapplication.cpp +++ b/lib/icinga/icingaapplication.cpp @@ -125,8 +125,6 @@ int IcingaApplication::Main(void) void IcingaApplication::OnShutdown(void) { - ASSERT(!OwnsLock()); - { ObjectLock olock(this); l_RetentionTimer->Stop(); diff --git a/lib/icinga/notification.cpp b/lib/icinga/notification.cpp index d61586f27..2b40d2215 100644 --- a/lib/icinga/notification.cpp +++ b/lib/icinga/notification.cpp @@ -240,8 +240,6 @@ String Notification::NotificationTypeToString(NotificationType type) void Notification::BeginExecuteNotification(NotificationType type, const CheckResult::Ptr& cr, bool force, const String& author, const String& text) { - ASSERT(!OwnsLock()); - Log(LogNotice, "Notification") << "Attempting to send notifications for notification object '" << GetName() << "'."; @@ -398,8 +396,6 @@ void Notification::BeginExecuteNotification(NotificationType type, const CheckRe bool Notification::CheckNotificationUserFilters(NotificationType type, const User::Ptr& user, bool force) { - ASSERT(!OwnsLock()); - if (!force) { TimePeriod::Ptr tp = user->GetPeriod(); @@ -465,8 +461,6 @@ bool Notification::CheckNotificationUserFilters(NotificationType type, const Use void Notification::ExecuteNotificationHelper(NotificationType type, const User::Ptr& user, const CheckResult::Ptr& cr, bool force, const String& author, const String& text) { - ASSERT(!OwnsLock()); - try { NotificationCommand::Ptr command = GetCommand(); -- 2.40.0