From 67b010ba7ebaa60cb8a7d67b287dbd0c7c27e10a Mon Sep 17 00:00:00 2001 From: Ichthyostega Date: Mon, 25 Sep 2023 23:59:34 +0200 Subject: [PATCH] Library: (re)introduce the distinction join / detach While it would be straight forward from an implementation POV to just expose both variants on the API (as the C++ standard does), it seems prudent to enforce the distinction, and to highlight the auto-detaching behaviour as the preferred standard case. Creating worker threads just for one computation and joining the results seemed like a good idea 30 years ago; today we prefer Futures or asynchronous messaging to achieve similar results in a robust and performant way. ThreadJoinable can come in handy however for writing unit tests, were the controlling master thread has to wait prior to perform verification. So the old design seems well advised in this respect and will be retained --- src/lib/thread.hpp | 103 ++++++++---- tests/basics/diagnostic-context-test.cpp | 3 +- .../control/session-command-function-test.cpp | 2 +- tests/library/thread-wrapper-join-test.cpp | 20 --- wiki/thinkPad.ichthyo.mm | 154 ++++++++++++++++++ 5 files changed, 228 insertions(+), 54 deletions(-) diff --git a/src/lib/thread.hpp b/src/lib/thread.hpp index 970bd66e7..69deb4b94 100644 --- a/src/lib/thread.hpp +++ b/src/lib/thread.hpp @@ -52,6 +52,11 @@ ** As a convenience, the destructor blocks for a short timespan of 20ms; a thread running ** beyond that grace period will kill the whole application by `std::terminate`. ** + ** For the exceptional case when a supervising thread need to await the termination of + ** launched threads, a different front-end \ref lib::ThreadJoinable is provided, exposing + ** the `join()` operation. Such threads *must* be joined however, and thus the destructor + ** immediately terminates the application in case the thread is still running. + ** ** ## Synchronisation ** The C++ standard provides that the end of the `std::thread` constructor _syncs-with_ the ** start of the new thread function, and likewise the end of the thread activity _syncs-with_ @@ -106,37 +111,36 @@ #include #include #include +#include namespace lib { using std::string; + namespace thread {// Thread-wrapper base implementation... + template class ThreadWrapper : util::MoveOnly { - template void main (string threadID, FUN&& threadFunction, ARGS&& ...args) { + markThreadStart (threadID); try { - markThreadStart (threadID); - // execute the actual operation in this new thread + // execute the actual operation in this new thread std::invoke (std::forward (threadFunction), std::forward (args)...); } ERROR_LOG_AND_IGNORE (thread, "Thread function") + // + markThreadEnd (threadID); + if (autoTerm) + threadImpl_.detach(); } - void - markThreadStart (string const& threadID) - { - string logMsg = util::_Fmt{"Thread '%s' start..."} % threadID; - TRACE (thread, "%s", logMsg.c_str()); - //////////////////////////////////////////////////////////////////////OOO maybe set the the Thread-ID via POSIX ?? - } protected: std::thread threadImpl_; @@ -144,6 +148,11 @@ namespace lib { /** @internal derived classes may create an inactive thread */ ThreadWrapper() : threadImpl_{} { } + ~ThreadWrapper() + { + if (autoTerm and threadImpl_.joinable()) + waitGracePeriod(); + } public: /** Create a new thread to execute the given operation. @@ -187,6 +196,42 @@ namespace lib { { return threadImpl_.get_id() == std::this_thread::get_id(); } // Note: implies get_id() != std::thread::id{} ==> it is running + + private: + void + markThreadStart (string const& threadID) + { + string logMsg = util::_Fmt{"Thread '%s' start..."} % threadID; + TRACE (thread, "%s", logMsg.c_str()); + //////////////////////////////////////////////////////////////////////OOO maybe set the the Thread-ID via POSIX ?? + } + + void + markThreadEnd (string const& threadID) + { + string logMsg = util::_Fmt{"Thread '%s' finished..."} % threadID; + TRACE (thread, "%s", logMsg.c_str()); + } + + void + waitGracePeriod() noexcept + { + using std::chrono::steady_clock; + using std::chrono_literals::operator ""ms; + + try { + auto start = steady_clock::now(); + while (threadImpl_.joinable() + and steady_clock::now () - start < 20ms + ) + std::this_thread::yield(); + } + ERROR_LOG_AND_IGNORE (thread, "Thread shutdown wait") + + if (threadImpl_.joinable()) + ALERT (thread, "Thread failed to terminate after grace period. Abort."); + // invocation of std::thread dtor will presumably call std::terminate... + } }; }//(End)base implementation. @@ -204,7 +249,7 @@ namespace lib { * `std::terminate` afterwards, should the thread still be active then. */ class Thread - : public thread::ThreadWrapper + : public thread::ThreadWrapper { public: @@ -216,35 +261,29 @@ namespace lib { - /** - * Variant of the standard case, allowing additionally - * to join on the termination of this thread. + /************************************************************************//** + * Variant of the [standard case](\ref Thread), requiring to wait and `join()` + * on the termination of this thread. Useful to collect results calculated + * by multiple threads. Note however that the system resources of the thread + * are kept around until the `join()` call, and thus also the `bool` conversion + * yields `true`, even while the actual operation has already terminated. + * @warning Thread must be joined prior to destructor invocation, otherwise + * the application is shut down immediately via `std::terminate`. */ class ThreadJoinable - : public thread::ThreadWrapper + : public thread::ThreadWrapper { public: using ThreadWrapper::ThreadWrapper; - /** put the caller into a blocking wait until this thread has terminated. - * @return token signalling either success or failure. - * The caller can find out by invoking `isValid()` - * or `maybeThrow()` on this result token - */ - lib::Result + /** put the caller into a blocking wait until this thread has terminated */ + void join () { -// if (!isValid()) -// throw error::Logic ("joining on an already terminated thread"); -// -// lumiera_err errorInOtherThread = -// "TODO TOD-oh";//lumiera_thread_join (threadHandle_); //////////////////////////////////OOO -// threadHandle_ = 0; -// -// if (errorInOtherThread) -// return error::State ("Thread terminated with error", errorInOtherThread); -// else -// return true; + if (not threadImpl_.joinable()) + throw error::Logic ("joining on an already terminated thread"); + + threadImpl_.join(); } }; diff --git a/tests/basics/diagnostic-context-test.cpp b/tests/basics/diagnostic-context-test.cpp index 75756c4a1..f08949471 100644 --- a/tests/basics/diagnostic-context-test.cpp +++ b/tests/basics/diagnostic-context-test.cpp @@ -165,7 +165,8 @@ namespace test{ TestThread testcase[NUM_THREADS] SIDEEFFECT; for (uint i=0; i < NUM_THREADS; ++i) - CHECK (testcase[i].join().isValid() ); + testcase[i].join(); +// CHECK (testcase[i].join().isValid() ); ////////////////////////////////////////////OOO need a way to pass the verification-Result. Maybe a Future? } diff --git a/tests/core/steam/control/session-command-function-test.cpp b/tests/core/steam/control/session-command-function-test.cpp index f09493684..f2633eda2 100644 --- a/tests/core/steam/control/session-command-function-test.cpp +++ b/tests/core/steam/control/session-command-function-test.cpp @@ -345,7 +345,7 @@ namespace test { ~InvocationProducer() { - this->join().maybeThrow(); + this->join(); // .maybeThrow(); /////////////////////////////////////////OOO should detect exceptions in thread explicitly for (auto& id : cmdIDs_) Command::remove (cStr(id)); } diff --git a/tests/library/thread-wrapper-join-test.cpp b/tests/library/thread-wrapper-join-test.cpp index b451ed813..1ba9acd68 100644 --- a/tests/library/thread-wrapper-join-test.cpp +++ b/tests/library/thread-wrapper-join-test.cpp @@ -65,7 +65,6 @@ namespace test { { simpleUse (); wrongUse (); - getError (); } @@ -111,25 +110,6 @@ namespace test { VERIFY_ERROR(LOGIC, newThread.join() ); VERIFY_ERROR(LOGIC, newThread.join() ); } - - - void - getError() - { - ThreadJoinable thread1("test Thread joining-3" - , bind (&ThreadWrapperJoin_test::theAction, this, DESTRUCTION_CODE) - ); - - VERIFY_ERROR(SPECIAL, thread1.join().maybeThrow() ); - - - - ThreadJoinable thread2("test Thread joining-4" - , bind (&ThreadWrapperJoin_test::theAction, this, DESTRUCTION_CODE) - ); - - CHECK (not thread2.join().isValid()); // can check success without throwing - } }; diff --git a/wiki/thinkPad.ichthyo.mm b/wiki/thinkPad.ichthyo.mm index f5011509b..a990804fa 100644 --- a/wiki/thinkPad.ichthyo.mm +++ b/wiki/thinkPad.ichthyo.mm @@ -79390,6 +79390,138 @@ Date:   Thu Apr 20 18:53:17 2023 +0200
+ + + + + + + + +

+ ...da die Implementierung mit einem aktiven Threadpool verbunden war, gab es dort auch eine Managment-Datenstruktur; die Threadpool-Logik hat eine eventuell im Thread noch erkannte Fehlerflag dorthin gespeichert — und join() konnte diese Fehlerflag dann einfach abholen. +

+ + + + + + + + + + +

+ ...C kennt ja keine Exceptions — und der Author des Threadpool-Frameworks hielt Exceptions für ein bestenfalls überflüssiges Feature, das es ganz bestimmt nicht rechtfertigt, zusätzlichen Aufwand zu treiben +

+ + + + + + + + + + +

+ gemeint ist lib::Result +

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

+ ...und zwar im Besonderen, wenn man in einer »reaping-loop« alle Kind-Threads ernten möchte; dann würde nur ein Fehler ausgeworfen, und die weiteren Kinder bleiben ungeerntet (und terminieren jetzt, mit der C++14-Lösung sogar das Programm) +

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

+ Das ist ein klassischer Fall für ein Feature, das zu Beginn so offensichtlich und irre nützlich aussieht — dann aber in der Realität nicht wirklich „fliegt“ +

+ + + + + + + + + + +

+ heute bevorzugt man für ähnliche Anforderungen das Future / Promise - Konstrukt +

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

+ Denn tatsächlich sind aufgetretene Fehler dann ehr schon eine Form von Zustand, den man mit einem speziellen Protokoll im Thread-Objekt erfassen und nach dem join() abfragen sollte; so kann man auch die Ergebnisse mehrerer Threads korrekt kombinieren +

+ + + + + + + + + + @@ -79819,6 +79951,28 @@ Date:   Thu Apr 20 18:53:17 2023 +0200
+ + + + + + + + + + + + + + + + + + + + + +