From 73737f2aeee5afbbdcfec80766b0a6e5be0a514b Mon Sep 17 00:00:00 2001 From: Ichthyostega Date: Sat, 14 Oct 2023 23:40:57 +0200 Subject: [PATCH] Library/Application: consolidate Monitor implementation After the fundamental switch from POSIX to the C++14 wrappers the existing implementation of the Monitor can now be drastically condensed, removing several layers of indirection. Moreover, all signatures shall be changed to blend in with the names and patterns established by the C++ standard. This is Step-1 : consolidate the Implementation. (to ensure correctness, the existing API towards application code was retained) --- src/lib/sync-classlock.hpp | 8 +- src/lib/sync.hpp | 134 ++++--- src/steam/control/steam-dispatcher.cpp | 2 +- tests/library/sync-timedwait-test.cpp | 2 +- tests/library/sync-waiting-test.cpp | 15 +- tests/vault/gear/work-force-test.cpp | 11 +- wiki/thinkPad.ichthyo.mm | 516 +++++++++++++++++++++---- 7 files changed, 536 insertions(+), 152 deletions(-) diff --git a/src/lib/sync-classlock.hpp b/src/lib/sync-classlock.hpp index abc581ce4..19e8def30 100644 --- a/src/lib/sync-classlock.hpp +++ b/src/lib/sync-classlock.hpp @@ -64,8 +64,8 @@ namespace lib { class ClassLock : public Sync::Lock { - typedef typename Sync::Lock Lock; - typedef typename sync::Monitor Monitor; + using Lock = typename Sync::Lock; + using Monitor = typename sync::Monitor; struct PerClassMonitor : Monitor {}; @@ -80,7 +80,9 @@ namespace lib { } public: - ClassLock() : Lock (getPerClassMonitor()) { } + ClassLock() + : Lock{getPerClassMonitor()} + { } }; diff --git a/src/lib/sync.hpp b/src/lib/sync.hpp index 1871e1149..949fa8a4c 100644 --- a/src/lib/sync.hpp +++ b/src/lib/sync.hpp @@ -192,36 +192,37 @@ namespace lib { template - class Condition - : public Mutex> + struct Condition + : MTX + , std::condition_variable_any { - typedef Wrapped_Condition Cond; - public: + template void - signal (bool wakeAll=false) + wait (PRED&& predicate) { - if (wakeAll) - Cond::broadcast (); - else - Cond::signal (); + condVar().wait (mutex(), std::forward (predicate)); } - - /** @return `false` in case of timeout */ - template + /** @return `false` in case of timeout, + * `true` if predicate is fulfilled at return + */ + template bool - wait (BF& predicate, uint timeout_ms =0) + wait_for (std::chrono::duration const& timeout, PRED&& predicate) { - while (not predicate()) - if (timeout_ms != 0) - { - if (not Cond::timedwait (std::chrono::milliseconds (timeout_ms))) - return false; - } - else - Cond::wait (); - return true; + return condVar().wait_for (mutex(), timeout, std::forward (predicate)); + } + + MTX& + mutex() + { + return static_cast (*this); + } + std::condition_variable_any& + condVar() + { + return static_cast (*this); } }; @@ -263,41 +264,35 @@ namespace lib { template class Monitor : IMPL + , util::NonCopyable { public: - Monitor() {} - ~Monitor() {} - /** allow copy, without interfering with the identity of IMPL */ - Monitor (Monitor const& ref) : IMPL() { } - const Monitor& operator= (Monitor const& ref) { /*prevent assignment to base*/ return *this; } + void lock() { IMPL::lock(); } + void unlock() noexcept { IMPL::unlock(); } + void notify_one() noexcept { IMPL::notify_one(); } + void notify_all() noexcept { IMPL::notify_all(); } - void acquireLock() { IMPL::acquire(); } - void releaseLock() { IMPL::release(); } - - void signal(bool a){ IMPL::signal(a); } - - bool - wait (Flag flag, ulong timedwait_ms=0) + template + void + wait (PRED&& predicate) { - BoolFlagPredicate checkFlag(flag); - return IMPL::wait(checkFlag, timedwait_ms); + IMPL::wait (std::forward(predicate)); } - template + template bool - wait (X& instance, bool (X::*method)(void), ulong timedwait_ms=0) /////////////////////TICKET #1051 : add support for lambdas + wait_for (DUR const& timeout, PRED&& predicate) { - BoolMethodPredicate invokeMethod(instance, method); ///////////////////////TICKET #1057 : const correctness, allow use of const member functions - return IMPL::wait(invokeMethod, timedwait_ms); + return IMPL::wait_for (timeout, std::forward (predicate)); } }; - typedef Mutex NonrecursiveLock_NoWait; - typedef Mutex RecursiveLock_NoWait; - typedef Condition NonrecursiveLock_Waitable; - typedef Condition RecursiveLock_Waitable; + using NonrecursiveLock_NoWait = std::mutex; + using RecursiveLock_NoWait = std::recursive_mutex; + using NonrecursiveLock_Waitable = Condition; + using RecursiveLock_Waitable = Condition; } // namespace sync (helpers and building blocks) @@ -347,11 +342,11 @@ namespace lib { template class Sync { - typedef sync::Monitor Monitor; + using Monitor = sync::Monitor; mutable Monitor objectMonitor_; static Monitor& - getMonitor(const Sync* forThis) + getMonitor(Sync const* forThis) { REQUIRE (forThis); return forThis->objectMonitor_; @@ -369,49 +364,62 @@ namespace lib { public: template - Lock(X* it) : mon_(getMonitor(it)){ mon_.acquireLock(); } - ~Lock() { mon_.releaseLock(); } + Lock(X* it) : mon_(getMonitor(it)){ mon_.lock(); } + ~Lock() { mon_.unlock(); } - void notify() { mon_.signal(false);} - void notifyAll() { mon_.signal(true); } + void notify() { mon_.notify_one(); } + void notifyAll() { mon_.notify_all(); } template bool - wait (C& cond, ulong timeout=0) //////////////////////////////////////TICKET #1055 : accept std::chrono values here + wait (C&& cond, ulong timeout_ms=0) //////////////////////////////////////TICKET #1055 : accept std::chrono values here { - return mon_.wait(cond,timeout); + if (timeout_ms) + return mon_.wait_for (std::chrono::milliseconds(timeout_ms) + ,std::forward (cond)); + else + { + mon_.wait (std::forward (cond)); + return true; + } } 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_ms=0) //////////////////////TICKET #1051 : enable use of lambdas { - return mon_.wait(instance,predicate,timeout); + return wait([&]{ return (instance.*predicate)(); }, timeout_ms); } /** convenience shortcut: * Locks and immediately enters wait state, * observing a condition defined as member function. * @deprecated WARNING this function is not correct! ////////////////////////////TICKET #1051 - * Lock is not released on error from within wait() + * Lock is not released on error from within wait() /////TODO is actually fixed now; retain this API?? */ template Lock(X* it, bool (X::*method)(void)) : mon_(getMonitor(it)) { - mon_.acquireLock(); - mon_.wait(*it,method); + mon_.lock(); + try { wait(*it,method); } + catch(...) + { + mon_.unlock(); + throw; + } } protected: /** for creating a ClassLock */ - Lock(Monitor& m) : mon_(m) - { mon_.acquireLock(); } + Lock(Monitor& m) + : mon_(m) + { + mon_.lock(); + } - /** for controlled access to the - * underlying sync primitives */ - Monitor& - accessMonitor() { return mon_; } + /** subclass access to underlying sync primitives */ + Monitor& accessMonitor() { return mon_; } }; }; diff --git a/src/steam/control/steam-dispatcher.cpp b/src/steam/control/steam-dispatcher.cpp index d6cee7eaa..bc6e30dc7 100644 --- a/src/steam/control/steam-dispatcher.cpp +++ b/src/steam/control/steam-dispatcher.cpp @@ -163,7 +163,7 @@ namespace control { init_.sync(); // done with setup; loop may run now.... INFO (session, "Steam-Dispatcher running..."); { - Lock(this); // open public session interface: + Lock sync(this); // open public session interface: commandService_.createInstance(*this); } } diff --git a/tests/library/sync-timedwait-test.cpp b/tests/library/sync-timedwait-test.cpp index de72e7b89..32e912a7a 100644 --- a/tests/library/sync-timedwait-test.cpp +++ b/tests/library/sync-timedwait-test.cpp @@ -75,7 +75,7 @@ namespace test{ auto start = system_clock::now(); - bool salvation{false}; + auto salvation = []{ return false; }; bool fulfilled = lock.wait (salvation, WAIT_mSec); CHECK (not fulfilled); // condition not fulfilled, but timeout diff --git a/tests/library/sync-waiting-test.cpp b/tests/library/sync-waiting-test.cpp index 0e82914a3..ba5276e2b 100644 --- a/tests/library/sync-waiting-test.cpp +++ b/tests/library/sync-waiting-test.cpp @@ -32,9 +32,10 @@ #include "lib/sync.hpp" #include +#include -using std::bind; using test::Test; +using std::atomic_bool; namespace lib { @@ -73,14 +74,12 @@ namespace test{ public Sync { protected: - volatile bool got_new_data_; + atomic_bool got_new_data_{false}; public: - SyncOnBool() : got_new_data_ (false) {} - void getIt() { - Lock(this).wait (got_new_data_); + Lock(this).wait ([this]{ return bool(got_new_data_); }); sum_ += input_; } @@ -105,7 +104,7 @@ namespace test{ public: void getIt() { - Lock await(this, &SyncOnMemberPredicate::checkTheFlag); + Lock await(this, &SyncOnMemberPredicate::checkTheFlag); /////////////////////TODO becomes obsolete with the API change sum_ += input_; } @@ -158,8 +157,8 @@ namespace test{ { typedef ThreadJoinable<> Thread; //////////////////////////////////////WIP - Thread ping ("SyncWaiting ping", bind (&Token::getIt, &tok)); - Thread pong ("SyncWaiting pong", bind (&Token::getIt, &tok)); + Thread ping ("SyncWaiting ping", [&]{ return tok.getIt(); }); + Thread pong ("SyncWaiting pong", [&]{ return tok.getIt(); }); CHECK (ping); CHECK (pong); diff --git a/tests/vault/gear/work-force-test.cpp b/tests/vault/gear/work-force-test.cpp index 2dc397797..b4bc89db6 100644 --- a/tests/vault/gear/work-force-test.cpp +++ b/tests/vault/gear/work-force-test.cpp @@ -391,12 +391,12 @@ namespace test { auto fullCnt = work::Config::COMPUTATION_CAPACITY; wof.activate (1.0); - sleep_for(2ms); + sleep_for(5ms); CHECK (fullCnt == uniqueCnt); CHECK (fullCnt == wof.size()); wof.activate (2.0); - sleep_for(2ms); + sleep_for(5ms); CHECK (2*fullCnt == uniqueCnt); CHECK (2*fullCnt == wof.size()); @@ -404,11 +404,11 @@ namespace test { CHECK (0 == wof.size()); uniqueCnt.clear(); - sleep_for(2ms); + sleep_for(5ms); CHECK (0 == uniqueCnt); wof.activate (0.5); - sleep_for(2ms); + sleep_for(5ms); CHECK (fullCnt/2 == uniqueCnt); CHECK (fullCnt/2 == wof.size()); } @@ -438,7 +438,8 @@ namespace test { CHECK (3 == wof.size()); - sleep_for(500ms); // ...sufficiently long to count way beyond 10'000 + while (check < 6'000) + sleep_for(10ms); // .....sufficiently long to count way beyond 10'000 CHECK (check > 6'000); CHECK (1 == wof.size()); } diff --git a/wiki/thinkPad.ichthyo.mm b/wiki/thinkPad.ichthyo.mm index 015776760..4f2d78bb8 100644 --- a/wiki/thinkPad.ichthyo.mm +++ b/wiki/thinkPad.ichthyo.mm @@ -65890,7 +65890,7 @@ - + @@ -65923,8 +65923,9 @@ - + + @@ -65937,6 +65938,17 @@ + + + + +

+ Da bin ich einem Kurzschluß aufgesessen: wenn die Konstruktion scheitert, und der Destruktor (so wie hier) sich einfach auskoppelt, dann wird nichts im Wrapper selber gemacht, sondern dieser existiert nicht mehr. Der Thread greift damit wild auf freien Speicher zu, der zudem in keinster Weise synchronisiert ist (d.h. selbst wenn wir das Handle vorher auf 0 setzen, wird der schon startende Thread das wahrscheinlich nicht mehr mitbekommen). Dieses Synchronisations-Problem ist es aber auch, das zu Fehlfunktionen in anderen Konstellationen führt: es kommt sporadisch vor, daß isLive() zu Beginn des Threads noch nicht wahr liefert; die Gründe ließen sich nicht aufklären. In einem solchen Fall würde der Thread spurlos terminieren — und dadurch an anderer Stelle möglichrweise ein Protokoll brechen oder einen Deadlock verursachen. +

+ + + + @@ -66353,8 +66365,8 @@ - - + + @@ -66421,16 +66433,13 @@ - - - +

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

- - + @@ -66438,74 +66447,68 @@ - - - +

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

- - + + + + + + + - + @@ -66513,9 +66516,7 @@ - - - +

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 @@ -66555,9 +66556,7 @@ - - - +

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 @@ -66567,9 +66566,7 @@ - - - +

...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). @@ -66587,9 +66584,7 @@ - - - +

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 @@ -66613,9 +66608,7 @@ - - - +

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 @@ -66629,8 +66622,8 @@ - - + + @@ -66648,16 +66641,13 @@ - - - +

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

- - + @@ -66668,16 +66658,373 @@ - - + + - + + + + + +

+ rein konzeptionell sollte ein synchronisierbares Objekt nicht kopierbar sein, schon allein weil sich dadurch der Status der gemeinsamen Nutzbarkeit verwischt +

+ + + + + - + + + + + + + + + + + + + + + + + + + + + + + + + + +

+ daher erscheint es sinnvoll +

+
    +
  • + diese seltenen Fälle als separate Methode herauszuführen +
    • +
  • + die Signatur komplett an C++ anzupassen +
    • +
+ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + +

+ Hier gibt es zwar eine Lücke, nämlich die const& auf einen rvalue — aber das hier ist keine general-purpose-Library! +

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

+ also entweder ganz oben oder ganz unten, und ansonsten nur noch einen Template-Parameter durchgeben +

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

+ man muß nicht speziell auf den Typ des Trägers abstellen; vielmehr kann man dann einen static-cast nachschalten, und der scheitert dann halt gegebenfalls beim Bauen. Das habe ich grade eben genauso beim neuen Thread-Wrapper gemacht, und es hat sich bisher gut bewährt. Spezielle static-asserts sind dann gar nicht notwendig +

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

+ man hätte dann also zwei verschiedene APIs für essentiell das Gleiche +

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

+ λ-Notation ist inzwischen allgemein bekannt, und hat den Vorteil, daß das Binding nahe am verwendeten Code liegt. Das gilt ganz besonders auch für bool-Flags, und zudem müssen wir uns dann nicht mehr mit Konvertierungen, volatile und Atomics herumschlagen +

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

+ Im Besonderen #994 kann nun wirklich langsam zugemacht werden! Das schiebe ich nun schon so lange ¾ fertig vor mir her ... und es ist klar daß ich den letzten Schritt (TYPES<TY....>) noch länger vor mir her schiebe, wiewohl das eigentliche Problem effetiv bereits seit 2017 gelöst ist +

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

+ hier hatte ich einen »convenience-shortcut« — und der ist broken +

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

+ hab dummerweise den this-Ptr nicht mehr an dem Punkt; und das will ich auch nicht ändern — sonst hab ich in jeder dieser delegierenden Funktionen das getMonitor(this), und es ist auch inhaltlich nicht besonders präzise, schließlich arbeitet der Guard auf dem Monitor (siehe ClassLock) +

+ + + + + + + + + + + + + + +<html> + <head> + + + </head> + <body> + <p> + ...und zwar wegen der Typedef &quot;Monitor&quot;, die nur im Scope von Sync&lt;CONF&gt; + definiert ist &#8212; das geht gut, SOLANGE niemand das Layout der Klasse Sync + &#228;ndert, niemand &#8222;aus Versehen&#8220; eine VTable einf&#252;hrt, und solange niemand + den Typ Monitor abgreift, woanders instantiiert und dann diese Referenz + &#252;ber eine von Lock abgeleitete Klasse in die Referenz einspielt. <b><font color="#ea1b82">TJA</font></b>&#160; + &#9785; diesen &#8222;Jemand&#8220; gibt es bereits: SyncClasslock <font color="#ad0000">&#55357;&#56817;</font> + </p> + </body> +</html> + + + + + + + + + + + + + + + + + +

+ und zwar, weil eine solche anonyme Instanz den umschließenden Scope nicht schützt; sie sieht aber syntaktisch genauso aus wie ein wirksamer Scope-Guard +

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

+ ...rein konzeptionell ist es nämlich nicht notwendig, das Lock zu erlangen; aber unser bisheriges API hat dazu gezwungen. Nicht schlimm, aber auch nicht schön +

+ + + + + + + + + + @@ -66865,9 +67212,7 @@ - - - +

...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. @@ -66880,9 +67225,7 @@ - - - +

sie erfüllt alle Anforderungen, ist aber zu sehr vereinfacht @@ -66893,9 +67236,7 @@ - - - +

Vor allem... @@ -66928,9 +67269,7 @@ - - - +

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... @@ -83322,6 +83661,7 @@ Date:   Thu Apr 20 18:53:17 2023 +0200
+ @@ -96446,7 +96786,7 @@ class Something - + @@ -96493,6 +96833,40 @@ class Something + + + + +

+ Zufällig aber durchaus regelmäßig wieder auftretendes Fehlverhalten +

+
    +
  • + der Test erzeugt 75 ThreadJoinable, welche jeweils eine rekursive Berechnung mit thread_lokal storage und sleep machen +
    • +
  • + danach explizit alle Threads join() +
    • +
  • + ⟹ aus den aufgesammelten lib::Result wird manchmal die Platzhalter-Exception ausgeworfen, mit der in der Policy zu Beginn der std::exception_ptr vorbelegt wurde ("No result yet, thread still running; need to join() first."). +
    • +
+

+ Theoretische Analyse: in der Doku (Cppreference) heißt es, join() synchornizes-with dem Ende des Thread. Eigentlich würde ich daraus schlußfolgern, daß wir auch eine release-acquire-Barriere haben. Das könnte aber eine zu starke Schlußfolgerung sein. Wir erzeugen ja im Thread ein neues lib::Result und weisen das per move-assign zu; wenn der Effekt der Zuweisung auf den eingebetteten std::exception_ptr im Haupt-Thread noch nicht sichtbar ist, würde das beobachtete Verhalten resultieren. +

+

+ Aber ich sehe auch noch eine zweite Theorie: gestern hatte ich an den Anfang der ThreadLifecycle::invokeThreadFunction() einen Guard eingefügt, zum Schutz gegen eine Exception aus dem Konstruktor einer abgeleiteten Klasse; d.h. wenn es den Wrapper gar nicht(mehr) gibt, soll die Thread-Funktion ebenfalls ganz geräuschlos verschwinden. Könnte es sein, daß die Bedingung Policy::isLive() zu Beginn des Thread nicht zuverlässig anspricht? Falls das passiert, würde dann auch das eingebettete lib::Result nicht zugewiesen, der Thread wäre aber im Mater-Thread später als Joinable markiert, und das beobachtete Verhalten wäre die Folge. +

+ + + + + + + + + +