From c37871ca783e92038d1bec4532423723d76973e7 Mon Sep 17 00:00:00 2001 From: Ichthyostega Date: Fri, 13 Oct 2023 23:46:38 +0200 Subject: [PATCH] Library/Application: switch Locking from POSIX to C++14 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit While not directly related to the thread handling framework, it seems indicated to clean-up this part of the application alongside. For »everyday« locking concerns, an Object Monitor abstraction was built several years ago and together with the thread-wrapper, both at that time based on direct usage of POSIX. This changeset does a mere literal replacement of the POSIX calls with the corresponding C++ wrappers on the lowest level. The resulting code is needlessly indirect, yet at API-level this change is totally a drop-in replacment. --- src/common/subsystem-runner.hpp | 11 +- src/lib/sync.cpp | 72 ---- src/lib/sync.hpp | 187 +++------- src/lib/test/microbenchmark.hpp | 2 +- tests/basics/diagnostic-context-test.cpp | 13 +- tests/library/sync-timedwait-test.cpp | 98 ++---- wiki/thinkPad.ichthyo.mm | 420 ++++++++++++++++++++++- 7 files changed, 510 insertions(+), 293 deletions(-) delete mode 100644 src/lib/sync.cpp diff --git a/src/common/subsystem-runner.hpp b/src/common/subsystem-runner.hpp index 09b99aa79..6f72fdac6 100644 --- a/src/common/subsystem-runner.hpp +++ b/src/common/subsystem-runner.hpp @@ -166,6 +166,9 @@ namespace lumiera { wait () { Lock wait_blocking(this, &SubsystemRunner::allDead); + ////////////////////////////////////////////////////////////OOO Emergency-Exit richtig implementieren + if (isEmergencyExit()) + usleep(2*1000*1000); return isEmergencyExit(); } @@ -223,9 +226,11 @@ namespace lumiera { { if (isEmergencyExit()) { - Lock sync(this); - if (!sync.isTimedWait()) - sync.setTimeout(EMERGENCYTIMEOUT); +// Lock sync(this); +// if (!sync.isTimedWait()) +// sync.setTimeout(EMERGENCYTIMEOUT); + ////////////////////////////////////////////////////////////OOO Emergency-Exit richtig implementieren + return true; } return isnil (running_); // end wait if no running subsystem left diff --git a/src/lib/sync.cpp b/src/lib/sync.cpp deleted file mode 100644 index 590932906..000000000 --- a/src/lib/sync.cpp +++ /dev/null @@ -1,72 +0,0 @@ -/* - Sync - generic helper for object based locking and synchronisation - - Copyright (C) Lumiera.org - 2011, Hermann Vosseler - - 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., 675 Mass Ave, Cambridge, MA 02139, USA. - -* *****************************************************/ - -/** @file sync.cpp - ** This compilation unit holds the static attribute struct - ** for initialising pthread's recursive mutex. - ** - */ - - -#include "lib/sync.hpp" - - -namespace lib { -namespace sync { - - - namespace { // private pthread attributes - - pthread_mutexattr_t attribute_; - - void - initAttribute() - { - pthread_mutexattr_init (&attribute_); - pthread_mutexattr_settype (&attribute_, PTHREAD_MUTEX_RECURSIVE); - } - - - inline pthread_mutexattr_t* - recursive_flag() - { - static pthread_once_t _is_init_sync_mutex_attribute_(PTHREAD_ONCE_INIT); - pthread_once (&_is_init_sync_mutex_attribute_, initAttribute); - return &attribute_; - } - } - - - - /** - * @internal creating a recursive mutex. - * Defined here in a separate compilation unit, - * so it can refer to a single mutex attribute flag. - */ - Wrapped_RecursiveMutex::Wrapped_RecursiveMutex() - { - pthread_mutex_init (&mutex_, recursive_flag()); - } - - - -}}// lib::sync diff --git a/src/lib/sync.hpp b/src/lib/sync.hpp index c709aeec1..1871e1149 100644 --- a/src/lib/sync.hpp +++ b/src/lib/sync.hpp @@ -14,7 +14,7 @@ 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., 675 Mass Ave, Cambridge, MA 02139, USA. @@ -46,16 +46,17 @@ ** - You can't use the Lock#wait and Lock#notify functions unless you pick ** a parametrisation including a condition variable. ** - The "this" pointer is fed to the ctor of the Lock guard object. Thus - ** you may use any object's monitor as appropriate, especially in cases + ** you may use any object's monitor as appropriate, especially in cases ** when adding the monitor to a given class may cause size problems. ** - For sake of completeness, this implementation provides the ability for ** timed waits. But please consider that in most cases there are better ** solutions for running an operation with given timeout by utilising the ** Lumiera scheduler. Thus use of timed waits is \b discouraged. ** - There is a special variant of the Lock guard called ClassLock, which - ** can be used to lock based on a type, not an instance. + ** can be used to lock based on a type, not an instance. ** - in DEBUG mode, the implementation includes NoBug resource tracking. ** + ** @todo WIP-WIP 10/2023 switch from POSIX to C++14 ///////////////////////////////////////////////////////TICKET #1279 : also clean-up the Object-Monitor implementation ** @see mutex.h ** @see sync-locking-test.cpp ** @see sync-waiting-test.cpp @@ -70,14 +71,10 @@ #include "lib/error.hpp" #include "lib/nocopy.hpp" #include "lib/util.hpp" - -extern "C" { -#include "lib/lockerror.h" -} - -#include -#include -#include + +#include +#include +#include @@ -92,62 +89,47 @@ namespace lib { /* ========== adaptation layer for accessing backend/system level code ============== */ struct Wrapped_ExclusiveMutex + : util::NonCopyable { - pthread_mutex_t mutex_; + std::mutex mutex_; protected: - Wrapped_ExclusiveMutex() - { - pthread_mutex_init (&mutex_, NULL); - } - ~Wrapped_ExclusiveMutex() - { - if (pthread_mutex_destroy (&mutex_)) - ERROR (sync, "Failure destroying mutex."); - } // shouldn't happen in a correct program + Wrapped_ExclusiveMutex() = default; void lock() { - if (pthread_mutex_lock (&mutex_)) - throw lumiera::error::Fatal ("Mutex acquire failed"); - } // shouldn't happen in a correct program + mutex_.lock(); + } void unlock() { - if (pthread_mutex_unlock (&mutex_)) - ERROR (sync, "Failure unlocking mutex."); - } // shouldn't happen in a correct program + mutex_.unlock(); + } }; struct Wrapped_RecursiveMutex + : util::NonCopyable { - pthread_mutex_t mutex_; + std::recursive_mutex mutex_; protected: - Wrapped_RecursiveMutex(); - ~Wrapped_RecursiveMutex() - { - if (pthread_mutex_destroy (&mutex_)) - ERROR (sync, "Failure destroying (rec)mutex."); - } // shouldn't happen in a correct program + Wrapped_RecursiveMutex() = default; void lock() { - if (pthread_mutex_lock (&mutex_)) - throw lumiera::error::Fatal ("(rec)Mutex acquire failed"); - } // shouldn't happen in a correct program + mutex_.lock(); + } void unlock() { - if (pthread_mutex_unlock (&mutex_)) - ERROR (sync, "Failure unlocking (rec)mutex."); - } // shouldn't happen in a correct program + mutex_.unlock(); + } }; @@ -155,44 +137,27 @@ namespace lib { struct Wrapped_Condition : MTX { - pthread_cond_t cond_; + std::condition_variable_any cond_; protected: - Wrapped_Condition() - { - pthread_cond_init (&cond_, NULL); - } - ~Wrapped_Condition() - { - if (pthread_cond_destroy (&cond_)) - ERROR (sync, "Failure destroying condition variable."); - } // shouldn't happen in a correct program + Wrapped_Condition() = default; - bool + void wait() { - int err; - do { err = pthread_cond_wait (&this->cond_, &this->mutex_); - } while(err == EINTR); - - if (err) lumiera_lockerror_set (err, &NOBUG_FLAG(sync), NOBUG_CONTEXT_NOFUNC); - return not err; + cond_.wait (this->mutex_); } - + + template bool - timedwait (const struct timespec* timeout) + timedwait (std::chrono::duration const& timeout) { - int err; - do { err = pthread_cond_timedwait (&this->cond_, &this->mutex_, timeout); - } while(err == EINTR); - - if (err) lumiera_lockerror_set (err, &NOBUG_FLAG(sync), NOBUG_CONTEXT_NOFUNC); - return not err; + auto ret = cond_.wait_for (this->mutex_, timeout); + return (std::cv_status::no_timeout == ret); } - - void signal() { pthread_cond_signal (&cond_); } - void broadcast() { pthread_cond_broadcast (&cond_); } + void signal() { cond_.notify_one(); } + void broadcast() { cond_.notify_all(); } }; @@ -208,10 +173,7 @@ namespace lib { : protected MTX { protected: - ~Mutex () { } - Mutex () { } - Mutex (const Mutex&); ///< noncopyable... - const Mutex& operator= (const Mutex&); + Mutex () = default; public: void @@ -229,43 +191,6 @@ namespace lib { - /** - * helper for specifying an optional timeout for an timed wait. - * Wrapping a timespec-struct, it allows for easy initialisation - * by a given relative offset. - * @todo integrate with std::chrono //////////////////////////TICKET #1055 - */ - struct Timeout - : timespec - { - Timeout() { reset(); } - - void reset() { tv_sec=tv_nsec=0; } - - /** initialise to NOW() + offset (in milliseconds) */ - Timeout& - setOffset (ulong offs) - { - if (offs) - { - clock_gettime(CLOCK_REALTIME, this); //////////////////////////TICKET #886 - tv_sec += offs / 1000; - tv_nsec += 1000000 * (offs % 1000); - if (tv_nsec >= 1000000000) - { - tv_sec += tv_nsec / 1000000000; - tv_nsec %= 1000000000; - } } - else - reset(); - return *this; - } - - explicit operator bool() { return 0 != tv_sec; } // allows if (timeout_).... - }; - - - template class Condition : public Mutex> @@ -283,20 +208,19 @@ namespace lib { } + /** @return `false` in case of timeout */ template bool - wait (BF& predicate, Timeout& waitEndTime) + wait (BF& predicate, uint timeout_ms =0) { - bool ok = true; - while (ok and !predicate()) - if (waitEndTime) - ok = Cond::timedwait (&waitEndTime); + while (not predicate()) + if (timeout_ms != 0) + { + if (not Cond::timedwait (std::chrono::milliseconds (timeout_ms))) + return false; + } else - ok = Cond::wait (); - - if (not ok and lumiera_error_expect(LUMIERA_ERROR_LOCK_TIMEOUT)) return false; - lumiera::throwOnError(); // any other error throws - + Cond::wait (); return true; } }; @@ -340,15 +264,13 @@ namespace lib { class Monitor : IMPL { - Timeout timeout_; - public: Monitor() {} ~Monitor() {} /** allow copy, without interfering with the identity of IMPL */ - Monitor (Monitor const& ref) : IMPL(), timeout_(ref.timeout_) { } - const Monitor& operator= (Monitor const& ref) { timeout_ = ref.timeout_; return *this; } + Monitor (Monitor const& ref) : IMPL() { } + const Monitor& operator= (Monitor const& ref) { /*prevent assignment to base*/ return *this; } void acquireLock() { IMPL::acquire(); } @@ -357,22 +279,19 @@ namespace lib { void signal(bool a){ IMPL::signal(a); } bool - wait (Flag flag, ulong timedwait=0) + wait (Flag flag, ulong timedwait_ms=0) { BoolFlagPredicate checkFlag(flag); - return IMPL::wait(checkFlag, timeout_.setOffset(timedwait)); + return IMPL::wait(checkFlag, timedwait_ms); } template bool - wait (X& instance, bool (X::*method)(void), ulong timedwait=0) ///////////////////////TICKET #1051 : add support for lambdas + wait (X& instance, bool (X::*method)(void), ulong timedwait_ms=0) /////////////////////TICKET #1051 : add support for lambdas { BoolMethodPredicate invokeMethod(instance, method); ///////////////////////TICKET #1057 : const correctness, allow use of const member functions - return IMPL::wait(invokeMethod, timeout_.setOffset(timedwait)); + return IMPL::wait(invokeMethod, timedwait_ms); } - - void setTimeout(ulong relative) {timeout_.setOffset(relative);} - bool isTimedWait() {return bool{timeout_};} }; typedef Mutex NonrecursiveLock_NoWait; @@ -455,8 +374,6 @@ namespace lib { void notify() { mon_.signal(false);} void notifyAll() { mon_.signal(true); } - void setTimeout(ulong time) { mon_.setTimeout(time); } - bool isTimedWait() { return mon_.isTimedWait(); } template bool @@ -467,7 +384,7 @@ namespace lib { template bool - wait (X& instance, bool (X::*predicate)(void), ulong timeout=0) //////////////////////TICKET #1051 : enable use of lambdas + wait (X& instance, bool (X::*predicate)(void), ulong timeout=0) //////////////////////TICKET #1051 : enable use of lambdas { return mon_.wait(instance,predicate,timeout); } @@ -480,8 +397,8 @@ namespace lib { */ template Lock(X* it, bool (X::*method)(void)) - : mon_(getMonitor(it)) - { + : mon_(getMonitor(it)) + { mon_.acquireLock(); mon_.wait(*it,method); } diff --git a/src/lib/test/microbenchmark.hpp b/src/lib/test/microbenchmark.hpp index 3b82d3b1d..28b904fec 100644 --- a/src/lib/test/microbenchmark.hpp +++ b/src/lib/test/microbenchmark.hpp @@ -82,7 +82,7 @@ namespace test{ inline double benchmarkTime (FUN const& invokeTestLoop, const size_t repeatCnt = DEFAULT_RUNS) { - using std::chrono::system_clock; + using std::chrono::system_clock;; /////////////////////////////////////////TICKET #886 using Dur = std::chrono::duration; auto start = system_clock::now(); diff --git a/tests/basics/diagnostic-context-test.cpp b/tests/basics/diagnostic-context-test.cpp index 4fe748cd0..f698b8105 100644 --- a/tests/basics/diagnostic-context-test.cpp +++ b/tests/basics/diagnostic-context-test.cpp @@ -29,6 +29,7 @@ #include "lib/test/test-helper.hpp" #include "lib/diagnostic-context.hpp" +#include "lib/iter-explorer.hpp" #include "lib/thread.hpp" #include @@ -171,10 +172,14 @@ namespace test{ } }; - std::array testcase; - - for (uint i=0; i < NUM_THREADS; ++i) - verifyResult (testcase[i].join()); + std::array testcases; + + auto results = lib::explore(testcases) + .transform([](TestThread& t){ return t.join(); }) + .effuse(); + + for (auto& res : results) + verifyResult (res); } diff --git a/tests/library/sync-timedwait-test.cpp b/tests/library/sync-timedwait-test.cpp index d0e15361f..de72e7b89 100644 --- a/tests/library/sync-timedwait-test.cpp +++ b/tests/library/sync-timedwait-test.cpp @@ -27,27 +27,25 @@ #include "lib/test/run.hpp" #include "lib/error.hpp" - #include "lib/sync.hpp" -#include +#include -using std::cout; using test::Test; +using std::chrono::system_clock; namespace lib { namespace test{ - namespace { // private test classes and data... + namespace { // test parameters... - const uint WAIT_mSec = 200; ///< milliseconds to wait before timeout + const uint WAIT_mSec = 20; ///< milliseconds to wait before timeout + + using CLOCK_SCALE = std::milli; // Results are in ms + using Dur = std::chrono::duration; - } // (End) test classes and data.... - - - - + }//(End) parameters @@ -55,25 +53,16 @@ namespace test{ /****************************************************************************//** - * @test timeout feature on condition wait as provided by pthread and accessible - * via the object monitor based locking/waiting mechanism. Without creating - * multiple threads, we engage into a blocking wait, which aborts due to - * setting a timeout. Our waiting facility is written such as to invoke - * the condition prior to entering wait state (and consecutively whenever - * awakened). This test switches into wait-with-timeout mode right from - * within this condition check and thus works even while there is no - * other thread and thus an unconditional wait would stall forever. - * - * @note it is discouraged to use the timed wait feature for "timing"; - * when possible you should prefer relying on the Lumiera scheduler - * + * @test timeout feature on condition wait as provided by the underlying implementation + * and accessible via the object monitor based locking/waiting mechanism. Without + * creating multiple threads, we engage into a blocking wait, which aborts due to + * setting a timeout. * @see SyncWaiting_test - * @see sync::Timeout * @see sync.hpp */ class SyncTimedwait_test : public Test, - Sync + Sync { friend class Lock; // allows inheriting privately from Sync @@ -82,60 +71,19 @@ namespace test{ virtual void run (Arg) { - checkTimeoutStruct(); + Lock lock(this); - Lock block(this, &SyncTimedwait_test::neverHappens); + auto start = system_clock::now(); + + bool salvation{false}; + bool fulfilled = lock.wait (salvation, WAIT_mSec); - cout << "back from LaLaLand, alive and thriving!\n"; - CHECK (block.isTimedWait()); + CHECK (not fulfilled); // condition not fulfilled, but timeout + + Dur duration = system_clock::now () - start; + CHECK (WAIT_mSec <= duration.count()); + CHECK (duration.count() < 2*WAIT_mSec); } - - - bool - neverHappens() ///< the "condition test" used for waiting.... - { - Lock currentLock(this); // get the Lock recursively - if (!currentLock.isTimedWait()) // right from within the condition check: - currentLock.setTimeout(WAIT_mSec); // switch waiting mode to timed wait and set timeout - - return false; - } - - - - void - checkTimeoutStruct() - { - sync::Timeout tout; - - CHECK (!tout); - CHECK (0 == tout.tv_sec); - CHECK (0 == tout.tv_nsec); - - tout.setOffset (0); - CHECK (!tout); - CHECK (0 == tout.tv_sec); - CHECK (0 == tout.tv_nsec); - - timespec ref; - clock_gettime(CLOCK_REALTIME, &ref); - tout.setOffset (1); - CHECK (tout); - CHECK (0 < tout.tv_sec); - CHECK (ref.tv_sec <= tout.tv_sec); - CHECK (ref.tv_nsec <= 1000000 + tout.tv_nsec || ref.tv_nsec > 1000000000-100000); - - clock_gettime(CLOCK_REALTIME, &ref); - tout.setOffset (1000); - CHECK (tout); - if (ref.tv_nsec!=0) // should have gotten an overflow to the seconds part - { - CHECK (ref.tv_sec <= 2 + tout.tv_sec ); - CHECK ((ref.tv_nsec + 1000000 * 999) % 1000000000 - <= tout.tv_nsec); - } - } - }; diff --git a/wiki/thinkPad.ichthyo.mm b/wiki/thinkPad.ichthyo.mm index 293ce790b..015776760 100644 --- a/wiki/thinkPad.ichthyo.mm +++ b/wiki/thinkPad.ichthyo.mm @@ -66343,9 +66343,9 @@ - + - + @@ -66360,10 +66360,334 @@ - + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + +

+ Es kann zwar ein rekursives Mutex verwenden, aber jede Ebene von Locking muß dann als eigenes unique_lock-Token repräsentiert werden. +

+ + + + + + + + + + + + + + +

+ diese setzt nur ein BasicLockable voraus +

+ + + + + + + + + + +

+ Mutex selber ist bereits BasicLockable +

+ + + + + + + + + + + + + +

+ die C++ - Wrapper sind non-copyable +

+ + + + + + + + + + + + + + + + + + +

+ ...gegebenfalls kann es dadurch passieren, daß man ein grade gesperrtes Mutex einfach „fahren läßt“ — mit unabsehbaren Folgen +

+ + + + + + + + + + + + + + + + + + + + + + +

+ Genauer gesagt, der Code versucht, die Konventions-basierte Herangehensweise aus POSIX / C in einen Token-orientierten Ansatz für C++ zu übersetzen. Das bedeutet aber, auch das Monitor-API ist noch Konventions-basiert +

+ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + +

+ Ganz am Anfang hatte ich die Makros aus NoBug, deren Gebrauch im C++-Code schwierig ist. Daher dachte ich, ich packe die jeweils in einen Wrapper. Der Monitor ist dann nur ein Zustatz-Feature +

+ + + + + + + + + +

+ ...aber nachdem ich den Monitor entwickelt hatte, war mir klar, daß ich an den Basis-Elementen gar kein Interesse mehr habe, weil der Monitor ein Design-Pattern ist und damit ordnend auf den Code wirkt. Im Lauf der Jahre hat sich dann gezeigt, daß ich überhaupt nichts anderes brauche, als nur das Lock-Guard front-End (+Atomics für alle nicht-trivialen Fälle). +

+ + + + + + + + + + + + + + + + + +

+ Verbesserung, weil sie die Implementierung einfach und klar macht, und mehr dem C++-Stil entspricht. Aber sie ist auch ein stets vorhandener zusätzlicher Storage-Ballast, der eigentlich nicht notwendig wäre +

+ + + + + + + + + + + + + + + + + + + + + + + +

+ zwar traue ich mir zu, die mittlere Stufe der »strukturellen Verbesserung« direkt zu implementieren, aber in dieser Hinsicht überschätze ich häufig die reale Komplexität und die Projekt-Risiken. Schon allein um aufwendige Regressionen zu vermeiden sollte stets auf gute Test-Coverage geachtet werden, was nurch durch schritweises Vorgehen möglich ist +

+ + + + + + + + + + + + + + + + + + + + + + + + + + + + + +

+ ...zumindest war das wohl die Motivation, wenn ich die Kommentare im Test hinzunehme. +

+ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + @@ -66536,6 +66860,96 @@ + + + + + + + + + +

+ ...hatte ich damals sehr schnell geschrieben, um zu zeigen daß eine C++ - Lösung auch »einfach« sein kann. Chistian wollte damals unbedingt die Application-main in C implementieren, „damit alles wirklich einfach und verständlich bleibt“. Ich hatte das Gefühl, da stand eine Agenda im Raum, daß alles Wichtige in C sein sollte. Ich vertrat (und vertrete bis heute) den Standpunkt, daß die Erweiterungen in C++ aus gutem Grunde geschaffen wurden, weil C in wesentlichen Aspekten mutwillig zu einfach gehalten ist. Für den Traum von der schönen einfachen Lösung zahlt man dann jeden Tag Zinsen für technische Schulden. +

+ + + + + + + + + + + + +

+ sie erfüllt alle Anforderungen, ist aber zu sehr vereinfacht +

+ + + + + + + + + + +

+ Vor allem... +

+
    +
  • + ein Subsystem ist nicht einfach gestartet oder nicht-gestartet. Vielmehr ist der Start zunächst initiiert, und nach Erfüllung lokaler Kriterien ist der Start vollständig. +
    • +
  • + entsprechend ist ein Subsystem nicht einfach gestoppt. Vielmehr ist der Shutdown-angekündigt, dann die aktive-Phase-verlassen und schließlich der shutdown-abgeschlossen. +
    • +
+ + + + + + + + + + + + + + + + + + + + + + + + +

+ In lib::Sync (genauer: in der Implementierung Monitor-wait) war eine Unterstützung für Timeout nach POSIX eingebaut worden. Dies erfordert, daß die Timeout-Spec in der Storage des Client bereitgehalten wird (weil man POSIX-Funktionen nur einen Pointer übergibt). Das habe ich hierfür ausgenützt, indem nachträglich, im Fall einer Emergency, noch ein Timeout definiert wird. Da Condition-Variablen zur Prüfung der Bedingung immer wieder aufgeweckt werden, kann man so nachträglich eine Art vorzeitigen Abbruch realisieren... +

+ + + + + + + + + + + + + + +