From dd2fe7da596898c614644713467660e3430ee6f3 Mon Sep 17 00:00:00 2001 From: Ichthyostega Date: Mon, 9 Oct 2023 16:47:56 +0200 Subject: [PATCH] Library: restructure wrapper in accordance to the solution found So this finally solves the fundamental problem regarding a race on initialisation of the thread-wrapper; it does *not* solve the same problem for classes deriving from thread-wrapper, which renders this design questionable altogether -- but this is another story. In the end, this initialisation-race is rooted in the very nature of starting a thread; it seems there are the two design alternatives: - expose the thread-creation directly to user code (offloading the responsibility) - offer building blocks which are inherently dangerous --- src/lib/meta/util.hpp | 14 ++++ src/lib/thread.hpp | 168 +++++++++++++++++++++++---------------- wiki/thinkPad.ichthyo.mm | 146 +++++++++++++++++++++++++++++++--- 3 files changed, 246 insertions(+), 82 deletions(-) diff --git a/src/lib/meta/util.hpp b/src/lib/meta/util.hpp index ce327b281..72d64cc4e 100644 --- a/src/lib/meta/util.hpp +++ b/src/lib/meta/util.hpp @@ -210,6 +210,20 @@ namespace meta { }; + /** + * Metaprogramming helper to mark some arbitrary base type by subclassing. + * In most respects the _specially marked type_ behaves like the base; this + * can be used to mark some element at compile time, e.g. to direct it into + * a specialisation or let it pick some special overload. + */ + template + struct Marked + : BAS + { + using BAS::BAS; + }; + + diff --git a/src/lib/thread.hpp b/src/lib/thread.hpp index ac1f45675..ff717d7f9 100644 --- a/src/lib/thread.hpp +++ b/src/lib/thread.hpp @@ -76,7 +76,23 @@ ** in cases where a race could be critical, additional means must be implemented; a ** possible solution would be to use a [N-fold synchronisation barrier](\ref lib::SyncBarrier) ** explicitly, or otherwise to ensure there is sufficient delay in the starting thread function. - ** + ** + ** ##Caveat + ** While these thread-wrapper building blocks aim at packaging the complexity away, there is + ** the danger to miss a potential race, which is inherent with starting threads: the operation + ** in the new thread contends with any initialisation done _after_ launching the thread. Even + ** though encapsulating complex concurrent logic into an opaque component, as built on top + ** of the thread-wrappers, is highly desirable from a code sanity angle — it is dangerously + ** tempting to package self-contained data initialisation into a subclass, leading to the + ** kind of _undefined behaviour,_ which „can never happen“ under normal circumstances. + ** Even while the OS scheduler typically adds an latency of at least 100µs to the start + ** of the new thread function, initialising anything (even subclass data members) after + ** creating the thread-wrapper instance *is undefined behaviour*. As a remedy + ** - it should be considered to put the thread-warpper into a _member_ (instead of inheriting) + ** - an explicit lib::SyncBarrier can be added, to ensure the thread-function touches any + ** extended facilities only after the initialisation is complete (as a downside, note that + ** any hard synchronisation adds a possibility for deadlock). + ** ** @remarks Historical design evolution: ** - Lumiera offered simplified convenience wrappers long before a similar design ** became part of the C++14 standard. These featured the distinction in join-able or @@ -140,23 +156,10 @@ namespace lib { using std::is_same; using std::__or_; - ////////////////////////////////////////////////////////////////////////////////////////////////////OOO extract -> lib/meta /** - * Metaprogramming helper to mark some arbitrary base type by subclassing. - * In most respects the _specially marked type_ behaves like the base; this - * can be used to mark some element at compile time, e.g. to direct it into - * a specialisation or let it pick some special overload. - */ - template - struct Marked - : BAS - { - using BAS::BAS; - }; - ////////////////////////////////////////////////////////////////////////////////////////////////////OOO extract(End) - /** @internal wraps the C++ thread handle - * and provides some implementation details, - * which are then combined by the _policy template_ + * @internal wraps the C++ thread handle + * and provides some implementation details, + * which are then combined by the _policy template_ */ struct ThreadWrapper : util::MoveOnly @@ -173,10 +176,9 @@ namespace lib { , threadImpl_{} { } - template - ThreadWrapper (string const& threadID, ARGS&& ...args) + ThreadWrapper (string const& threadID) : threadID_{util::sanitise (threadID)} - , threadImpl_{forward (args)... } + , threadImpl_{} //Note: deliberately not starting the thread yet... { } /** @internal actually launch the new thread. @@ -325,49 +327,12 @@ namespace lib { Policy::handle_thread_still_running(); } - public: - /** - * Is this thread »active« and thus tied to OS resources? - * @note this implies some statefulness, which may contradict the RAII pattern. - * - especially note the possibly for derived classes to create an _empty_ Thread. - * - moreover note that ThreadJoinable may have terminated, but still awaits `join()`. - */ - explicit - operator bool() const - { - return Policy::isLive(); - } - - /** @return does this call happen from within this thread? */ - using Policy::invokedWithinThread; - - - /** Create a new thread to execute the given operation. - * The new thread starts up synchronously, can't be cancelled and it can't be joined. - * @param threadID human readable descriptor to identify the thread for diagnostics - * @param threadFunction a functor holding the code to execute within the new thread. - * Any function-like entity or callable is acceptable; arguments can be given. - * @warning The operation functor and all arguments will be copied into the new thread. - * The return from this constructor _syncs-with_ the launch of the operation. - */ - template - ThreadLifecycle (string const& threadID, FUN&& threadFunction, ARGS&& ...args) - : Policy{threadID - , &ThreadLifecycle::invokeThreadFunction...>, this - , forward (threadFunction) - , decay_t (args)... } // Note: need to decay the argument types - { } // (arguments must be copied) - - template - ThreadLifecycle (RES (SUB::*memFun) (ARGS...), ARGS ...args) - : ThreadLifecycle{util::joinDash (typeSymbol(), args...) - ,std::move (memFun) - ,static_cast (this) - ,forward (args)... } + /** derived classes may create a disabled thread */ + ThreadLifecycle() + : Policy{} { } - - + public: /** * Build a λ actually to launch the given thread operation later, * after the thread-wrapper-object is fully initialised. @@ -389,7 +354,7 @@ namespace lib { return [invocation = move(argCopy)] //Note: functor+args bound by-value into the λ (ThreadLifecycle& wrapper) { //the thread-main function - wrapper.launchThread (tuple_cat (tuple{&ThreadLifecycle::invokeThreadFunction + wrapper.launchThread (tuple_cat (tuple{&ThreadLifecycle::invokeThreadFunction...> , &wrapper} // passing the wrapper as instance-this ,move (invocation))); //...invokeThreadFunction() in turn delegates }; // to the user-provided thread-operation @@ -397,15 +362,18 @@ namespace lib { /** - * Configuration builder to define the operation to run in the thread, - * and possibly configure further details, depending on the Policy used. + * Configuration builder to define the operation running within the thread, + * and possibly configure further details, depending on the actual Policy used. * @remark the primary ThreadLifecycle-ctor accepts such a Launch-instance - * and invokes the chain of λ-functions collected in the member #launch + * and invokes a chain of λ-functions collected in the member #launch */ struct Launch : util::MoveOnly { - function launch; + using Act = function; + + Act launch; + string id; template Launch (FUN&& threadFunction, ARGS&& ...args) @@ -413,23 +381,83 @@ namespace lib { { } Launch&& - threadID (string const& id) + threadID (string const& threadID) { - launch = [=, chain=std::move(launch)] + id = threadID; + return move(*this); + } + + Launch&& + addLayer (Act action) + { + launch = [action=move(action), chain=move(launch)] (ThreadLifecycle& wrapper) { - util::unConst(wrapper.threadID_) = id; + action(wrapper); chain (wrapper); }; return move(*this); } }; + + /** + * Primary constructor: Launch the new thread with flexible configuration. + * @param launcher a #Launch builder with a λ-chain to configure and + * finally trigger start of the thread + */ ThreadLifecycle (Launch launcher) - : Policy{} + : Policy{launcher.id} { launcher.launch (*this); } + + /** + * Create a new thread to execute the given operation. + * The new thread starts up synchronously, can't be cancelled and it can't be joined. + * @param threadID human readable descriptor to identify the thread for diagnostics + * @param threadFunction a functor holding the code to execute within the new thread. + * Any function-like entity or callable is acceptable; arguments can be given. + * @warning The operation functor and all arguments will be copied into the new thread. + * The return from this constructor _syncs-with_ the launch of the operation. + */ + template + ThreadLifecycle (string const& threadID, FUN&& threadFunction, ARGS&& ...args) + : ThreadLifecycle{ + Launch{forward (threadFunction), forward (args)...} + .threadID(threadID)} + { } + + /** + * Special variant to bind a subclass member function as thread operation. + * @warning potential race between thread function and subclass initialisation + */ + template + ThreadLifecycle (RES (SUB::*memFun) (ARGS...), ARGS ...args) + : ThreadLifecycle{ + Launch{std::move (memFun) + ,static_cast (this) + ,forward (args)... + } + .threadID(util::joinDash (typeSymbol(), args...))} + { } + + + + /** + * Is this thread »active« and thus tied to OS resources? + * @note this implies some statefulness, which may contradict the RAII pattern. + * - especially note the possibly for derived classes to create an _empty_ Thread. + * - moreover note that ThreadJoinable may have terminated, but still awaits `join()`. + */ + explicit + operator bool() const + { + return Policy::isLive(); + } + + /** @return does this call happen from within this thread? */ + using Policy::invokedWithinThread; }; }//(End)base implementation. diff --git a/wiki/thinkPad.ichthyo.mm b/wiki/thinkPad.ichthyo.mm index 17ba42cc3..19a2e7946 100644 --- a/wiki/thinkPad.ichthyo.mm +++ b/wiki/thinkPad.ichthyo.mm @@ -65389,8 +65389,13 @@ + + + - + + + @@ -65467,6 +65472,10 @@ + + + + @@ -65482,7 +65491,8 @@ - + + @@ -65498,6 +65508,30 @@ + + + + + + + + + + + + + + + + + + + + + + + + @@ -65604,8 +65638,8 @@ - - + + @@ -65626,7 +65660,7 @@ - + @@ -65802,8 +65836,8 @@ - - + + @@ -65811,15 +65845,46 @@ - + + + - + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + @@ -65834,15 +65899,16 @@ - - + + - + + @@ -65884,6 +65950,9 @@ + + + @@ -95379,6 +95448,59 @@ class Something + + + + + + + + + + +

+ Einmal-Event beim Ausführen der Testsuite nach Compile. +

+

+ TEST Render job planning calculation: JobPlanning_test .. FAILED +

+

+ unexpected data on stdout +

+

+ 'total latency    : ≺11ms≻':1 +

+

+ does not match +

+

+ literal: total latency    : ≺30ms≻:1 +

+

+ Bemerkenswert: +

+
    +
  • + der Test selber scheitert nicht, weil die Berechnung wie erwartet abläuft +
    • +
  • + aber die berichtete Latency ist 11ms statt 30ms wie erwartet +
    • +
  • + das bedeutet: das JobTicket -> isEmpty() [weil dann fallback auf JOB_MINIMUM_RUNTIME in JobTicket::getExpectedRuntime ] +
    • +
  • + ohne das nun im Detail zu analysieren: ich bin beunruhigt, wie KANN das JobTicket empty sein -- es wird doch hier im Test explizit aus der MockFixture erstellt +
    • +
+

+ Kontext: bin grade am Umbauen des Thread-Frameworks... +

+ + + + +