From 7b50e9882d49aaab1f6c6fd252ed84ffcbad18bc Mon Sep 17 00:00:00 2001 From: Kevin Kofler Date: Wed, 5 Nov 2025 20:08:37 +0100 Subject: [PATCH] make bundled CppAD atomic_base threadsafe This fixes solveConcurrent crashing with a race condition when custom expression handlers are used. src/cppad/core/atomic_base.hpp: Include and . Do not include because CPPAD_ASSERT_FIRST_CALL_NOT_PARALLEL is no longer used. (CppAD::atomic_base::index_): Make this member non-const, because const members must be initialized in the member initializer, but the initialization is now in the constructor body because it needs to be within the mutex. (Logically, this member is still a constant.) (CppAD::atomic_base::vector_mutex): New private static variable of type std::shared_mutex. A global read-write mutex that protects accesses to the class_object() and class_name() vectors. (CppAD::atomic_base::class_object(), CppAD::atomic_base::class_name()): Remove no longer necessary CPPAD_ASSERT_FIRST_CALL_NOT_PARALLEL assertions. Instead, assume that these private static methods are called from within proper mutexes. Note that the first call can only reasonably be from the constructor, which holds an exclusive unique_lock. All the reading functions with shared locks require a valid index within the arrays, and such a valid index can only exist after at least one object has been constructed through the constructor. (Failing that, the assertion in the reading functions will trigger.) So the lazy initialization will always happen in an exclusive unique_lock, which is safe. (CppAD::atomic_base::afun_name): Hold a std::shared_lock (read lock) on vector_mutex for this whole function. (CppAD::atomic_base constructor): Remove the non-parallel restriction from the documentation and the non-parallel assertion from the code. Do not initialize index_ in the member initializer. Instead, wrap the initialization of index_ and the accesses to class_object() and class_name() in a std::unique_lock (write lock) on vector_mutex. (CppAD::atomic_base destructor): Hold a std::unique_lock (write lock) on vector_mutex for this whole function. (CppAD::atomic_base::class_object(std::size_t), CppAD::atomic_base::class_name(std::size_t)): Hold a std::shared_lock (read lock) on vector_mutex for this whole function. (CppAD::atomic_base::clear): Remove the non-parallel restriction from the documentation and the non-parallel assertion from the code. Hold a std::unique_lock (write lock) on vector_mutex for this whole function. --- src/cppad/core/atomic_base.hpp | 66 ++++++++++++++++++---------------- 1 file changed, 35 insertions(+), 31 deletions(-) diff --git a/src/cppad/core/atomic_base.hpp b/src/cppad/core/atomic_base.hpp index 71a52ec682..ab7fe58a5c 100644 --- a/src/cppad/core/atomic_base.hpp +++ b/src/cppad/core/atomic_base.hpp @@ -12,11 +12,11 @@ A copy of this license is included in the COPYING file of this distribution. Please visit http://www.coin-or.org/CppAD/ for information on other licenses. -------------------------------------------------------------------------- */ +# include +# include # include # include # include -// needed before one can use CPPAD_ASSERT_FIRST_CALL_NOT_PARALLEL -# include namespace CppAD { // BEGIN_CPPAD_NAMESPACE /*! @@ -38,7 +38,7 @@ class atomic_base { // constants // /// index of this object in class_object - const size_t index_; + size_t index_; // ----------------------------------------------------- // variables @@ -79,15 +79,16 @@ class atomic_base { // ----------------------------------------------------- // static member functions // + static std::shared_mutex vector_mutex; /// List of all the object in this class static std::vector& class_object(void) - { CPPAD_ASSERT_FIRST_CALL_NOT_PARALLEL; + { static std::vector list_; return list_; } /// List of names for each object in this class static std::vector& class_name(void) - { CPPAD_ASSERT_FIRST_CALL_NOT_PARALLEL; + { static std::vector list_; return list_; } @@ -102,7 +103,10 @@ class atomic_base { /// Name corresponding to a base_atomic object const std::string& afun_name(void) const - { return class_name()[index_]; } + { + std::shared_lock vector_lock(vector_mutex); + return class_name()[index_]; + } /* $begin atomic_ctor$$ $spell @@ -159,10 +163,6 @@ all the virtual functions that have their $head atomic_base$$ -$subhead Restrictions$$ -The $code atomic_base$$ constructor cannot be called in -$cref/parallel/ta_in_parallel/$$ mode. - $subhead Base$$ The template parameter determines the $icode Base$$ type for this $codei%AD<%Base%>%$$ atomic operation. @@ -237,15 +237,15 @@ atomic_base( const std::string& name, option_enum sparsity = bool_sparsity_enum ) : -index_ ( class_object().size() ) , sparsity_( sparsity ) -{ CPPAD_ASSERT_KNOWN( - ! thread_alloc::in_parallel() , - "atomic_base: constructor cannot be called in parallel mode." - ); - class_object().push_back(this); - class_name().push_back(name); - CPPAD_ASSERT_UNKNOWN( class_object().size() == class_name().size() ); +{ + { + std::unique_lock vector_lock(vector_mutex); + index_ = class_object().size(); + class_object().push_back(this); + class_name().push_back(name); + CPPAD_ASSERT_UNKNOWN( class_object().size() == class_name().size() ); + } // // initialize work pointers as null; for(size_t thread = 0; thread < CPPAD_MAX_NUM_THREADS; thread++) @@ -254,9 +254,13 @@ sparsity_( sparsity ) /// destructor informs CppAD that this atomic function with this index /// has dropped out of scope by setting its pointer to null virtual ~atomic_base(void) -{ CPPAD_ASSERT_UNKNOWN( class_object().size() > index_ ); - // change object pointer to null, but leave name for error reporting - class_object()[index_] = CPPAD_NULL; +{ + { + std::unique_lock vector_lock(vector_mutex); + CPPAD_ASSERT_UNKNOWN( class_object().size() > index_ ); + // change object pointer to null, but leave name for error reporting + class_object()[index_] = CPPAD_NULL; + } // // free temporary work memory for(size_t thread = 0; thread < CPPAD_MAX_NUM_THREADS; thread++) @@ -290,12 +294,16 @@ void free_work(size_t thread) } /// atomic_base function object corresponding to a certain index static atomic_base* class_object(size_t index) -{ CPPAD_ASSERT_UNKNOWN( class_object().size() > index ); +{ + std::shared_lock vector_lock(vector_mutex); + CPPAD_ASSERT_UNKNOWN( class_object().size() > index ); return class_object()[index]; } /// atomic_base function name corresponding to a certain index static const std::string& class_name(size_t index) -{ CPPAD_ASSERT_UNKNOWN( class_name().size() > index ); +{ + std::shared_lock vector_lock(vector_mutex); + CPPAD_ASSERT_UNKNOWN( class_name().size() > index ); return class_name()[index]; } /* @@ -2381,10 +2389,6 @@ If there is future use of an $code atomic_base$$ object, after a call to $code clear$$, the work space will be reallocated and held onto. -$head Restriction$$ -This routine cannot be called -while in $cref/parallel/ta_in_parallel/$$ execution mode. - $end ------------------------------------------------------------------------------ */ @@ -2394,10 +2398,8 @@ Free all thread_alloc static memory held by atomic_base (avoids reallocations). */ /// Free vector memory used by this class (work space) static void clear(void) -{ CPPAD_ASSERT_KNOWN( - ! thread_alloc::in_parallel() , - "cannot use atomic_base clear during parallel execution" - ); +{ + std::unique_lock vector_lock(vector_mutex); size_t i = class_object().size(); while(i--) { atomic_base* op = class_object()[i]; @@ -2419,5 +2421,7 @@ virtual void set_old(size_t id) { } // --------------------------------------------------------------------------- }; + +template std::shared_mutex atomic_base::vector_mutex; } // END_CPPAD_NAMESPACE # endif