Foundation, Modeling - Fix thread-safety data races for concurrent operations by dpasukhi · Pull Request #1180 · Open-Cascade-SAS/OCCT

dpasukhi · 2026-04-01T10:32:37Z

Eliminate non-deterministic SIGSEGV when multiple threads simultaneously create and destroy independent OCCT objects (no shared data between threads). Addresses crashes in BRepCheck_Analyzer, HLRBRep_Algo, BRepPrimAPI_Make*, and TopExp::MapShapesAndAncestors observed under high thread contention (~800 concurrent tests in Swift Testing parallel runner).

Poly_Triangulation: Replace raw Bnd_Box* cache with std::atomic<Bnd_Box*>
plus std::mutex for writes. Lock-free reads via memory_order_acquire; mutex-protected SetCachedMinMax/unsetCachedMinMax prevent use-after-free and double-delete when shared triangulations are accessed from multiple threads.

BRepCheck: Replace broken std::unique_ptrstd::mutex conditional lock pattern with always-present std::mutex. Use std::unique_lock with defer_lock and myIsParallel guard to avoid mutex overhead in single-threaded mode (zero-cost when parallel=false). Remove fragile double-checked locking in SetParallel; retain parallel flag for sub-algorithm dispatch only.

Foundation globals: Apply std::atomic for simple flags/counters (UnitsMethods_CascadeLengthUnit, OSD_Parallel_ToUseOcctThreads, OSD_WasSetSignal, OSD_SignalStackTraceLength, ADR_ACT_SIGIO_HANDLER, Standard_Failure_DefaultStackTraceLength). Protect Resource_Unicode format state with mutex plus atomic fast-path for lock-free reads after initialization. Protect Plugin::Load cache with std::shared_mutex for concurrent read-only lookups. Protect Units and UnitsAPI globals with std::recursive_mutex for thread-safe lazy initialization. Use std::call_once for OSD_Host Windows init.

TKBool: Convert 19 global mutable static variables to thread_local across 8 files (TopOpeBRepTool_2d, TopOpeBRepDS_connex, TopOpeBRepBuild_HBuilder, TopOpeBRepTool_SC, TopOpeBRepBuild_ffsfs, TopOpeBRepDS_FaceInterferenceTool, TopOpeBRep_vprdeg, TopOpeBRepDS_CurveExplorer) to eliminate cross-thread data corruption in boolean operations.

…dependent OCCT operations Eliminate non-deterministic SIGSEGV when multiple threads simultaneously create and destroy independent OCCT objects (no shared data between threads). Addresses crashes in BRepCheck_Analyzer, HLRBRep_Algo, BRepPrimAPI_Make*, and TopExp::MapShapesAndAncestors observed under high thread contention (~800 concurrent tests in Swift Testing parallel runner). Poly_Triangulation: Replace raw Bnd_Box* cache with std::atomic<Bnd_Box*> plus std::mutex for writes. Lock-free reads via memory_order_acquire; mutex-protected SetCachedMinMax/unsetCachedMinMax prevent use-after-free and double-delete when shared triangulations are accessed from multiple threads. BRepCheck: Replace broken std::unique_ptr<std::mutex> conditional lock pattern with always-present std::mutex. Use std::unique_lock with defer_lock and myIsParallel guard to avoid mutex overhead in single-threaded mode (zero-cost when parallel=false). Remove fragile double-checked locking in SetParallel; retain parallel flag for sub-algorithm dispatch only. Foundation globals: Apply std::atomic for simple flags/counters (UnitsMethods_CascadeLengthUnit, OSD_Parallel_ToUseOcctThreads, OSD_WasSetSignal, OSD_SignalStackTraceLength, ADR_ACT_SIGIO_HANDLER, Standard_Failure_DefaultStackTraceLength). Protect Resource_Unicode format state with mutex plus atomic fast-path for lock-free reads after initialization. Protect Plugin::Load cache with std::shared_mutex for concurrent read-only lookups. Protect Units and UnitsAPI globals with std::recursive_mutex for thread-safe lazy initialization. Use std::call_once for OSD_Host Windows init. TKBool: Convert 19 global mutable static variables to thread_local across 8 files (TopOpeBRepTool_2d, TopOpeBRepDS_connex, TopOpeBRepBuild_HBuilder, TopOpeBRepTool_SC, TopOpeBRepBuild_ffsfs, TopOpeBRepDS_FaceInterferenceTool, TopOpeBRep_vprdeg, TopOpeBRepDS_CurveExplorer) to eliminate cross-thread data corruption in boolean operations.

Copilot

Pull request overview

This PR aims to eliminate data races and non-deterministic crashes (e.g., SIGSEGV) when OCCT is used from many concurrent threads by adding synchronization around shared caches/globals and removing cross-thread shared mutable state.

Changes:

Refactors BRepCheck_* result classes to use an always-present mutex with a parallel-mode guard, and updates the parallel analyzer to use the new locking model.
Makes multiple Foundation-level globals thread-safe via std::atomic, adds mutex-based protection for lazy initialization, and introduces std::call_once on Windows host initialization.
Converts several TKBool global mutable statics to thread_local to prevent cross-thread state corruption.

Reviewed changes

Copilot reviewed 27 out of 27 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
src/ModelingAlgorithms/TKTopAlgo/BRepCheck/BRepCheck_Wire.cxx	Switches to `std::unique_lock` with `defer_lock` + `myIsParallel` gating for map access.
src/ModelingAlgorithms/TKTopAlgo/BRepCheck/BRepCheck_Vertex.cxx	Same locking pattern update for vertex result context map.
src/ModelingAlgorithms/TKTopAlgo/BRepCheck/BRepCheck_Shell.cxx	Same locking pattern update across shell checks/status mutations.
src/ModelingAlgorithms/TKTopAlgo/BRepCheck/BRepCheck_Result.hxx	Replaces optional mutex with always-present mutex; introduces `myIsParallel` flag and inline accessors.
src/ModelingAlgorithms/TKTopAlgo/BRepCheck/BRepCheck_Result.cxx	Updates `SetFailStatus` locking; removes old lazy-mutex `SetParallel()` implementation.
src/ModelingAlgorithms/TKTopAlgo/BRepCheck/BRepCheck_Face.cxx	Same locking pattern update for face checks/status mutations.
src/ModelingAlgorithms/TKTopAlgo/BRepCheck/BRepCheck_Edge.cxx	Same locking pattern update; parallel dispatch now keyed off `myIsParallel`.
src/ModelingAlgorithms/TKTopAlgo/BRepCheck/BRepCheck_Analyzer.cxx	Updates parallel analyzer to lock result mutex directly under parallel mode.
src/ModelingAlgorithms/TKBool/TopOpeBRepTool/TopOpeBRepTool_SC.cxx	Converts a shared static classifier pointer to `thread_local`.
src/ModelingAlgorithms/TKBool/TopOpeBRepTool/TopOpeBRepTool_2d.cxx	Converts multiple shared static caches/state to `thread_local`.
src/ModelingAlgorithms/TKBool/TopOpeBRepDS/TopOpeBRepDS_FaceInterferenceTool.cxx	Converts shared static boolean to `thread_local`.
src/ModelingAlgorithms/TKBool/TopOpeBRepDS/TopOpeBRepDS_CurveExplorer.cxx	Converts shared static pointer to `thread_local`.
src/ModelingAlgorithms/TKBool/TopOpeBRepDS/TopOpeBRepDS_connex.cxx	Converts shared static caches/state to `thread_local`.
src/ModelingAlgorithms/TKBool/TopOpeBRepBuild/TopOpeBRepBuild_HBuilder.cxx	Converts shared static list/iterator pointers to `thread_local`.
src/ModelingAlgorithms/TKBool/TopOpeBRepBuild/TopOpeBRepBuild_ffsfs.cxx	Converts shared static state to `thread_local`.
src/ModelingAlgorithms/TKBool/TopOpeBRep/TopOpeBRep_vprdeg.cxx	Converts shared static pointer to `thread_local`.
src/FoundationClasses/TKMath/Poly/Poly_Triangulation.hxx	Replaces raw cached bbox pointer with `std::atomic<Bnd_Box*>` + mutex for writes.
src/FoundationClasses/TKMath/Poly/Poly_Triangulation.cxx	Implements atomic/mutex cached bbox access and teardown logic.
src/FoundationClasses/TKernel/UnitsMethods/UnitsMethods.cxx	Makes `UnitsMethods_CascadeLengthUnit` atomic.
src/FoundationClasses/TKernel/UnitsAPI/UnitsAPI.cxx	Adds a recursive mutex around global UnitsAPI state and conversion APIs.
src/FoundationClasses/TKernel/Units/Units.cxx	Adds a recursive mutex around Units global lazy init and cached-conversion state.
src/FoundationClasses/TKernel/Standard/Standard_Failure.cxx	Makes default stack trace length atomic.
src/FoundationClasses/TKernel/Resource/Resource_Unicode.cxx	Adds atomic fast-path + mutex for resource-based format initialization.
src/FoundationClasses/TKernel/Plugin/Plugin.cxx	Protects plugin factory cache with `std::shared_mutex` and adds concurrent lookup fast path.
src/FoundationClasses/TKernel/OSD/OSD_signal.cxx	Makes several signal configuration globals atomic; uses atomic load in handler.
src/FoundationClasses/TKernel/OSD/OSD_Parallel.cxx	Makes `OSD_Parallel_ToUseOcctThreads` atomic.
src/FoundationClasses/TKernel/OSD/OSD_Host.cxx	Switches Windows host initialization to `std::call_once`.

Copilot · 2026-04-01T10:38:34Z

 {
  static const Bnd_Box anEmptyBox;
-  return (myCachedMinMax == nullptr) ? anEmptyBox : *myCachedMinMax;
+  const Bnd_Box*       aBox = myCachedMinMax.load(std::memory_order_acquire);


The current atomic-pointer cache is still not thread-safe: CachedMinMax() loads a raw Bnd_Box* and dereferences it without any lifetime guarantee. Another thread can call unsetCachedMinMax()/SetCachedMinMax(void) and delete the box concurrently, leading to use-after-free (and readers can also race with in-place writes to *aBox). Consider switching to a read/write lock for both readers and writers (e.g., std::shared_mutex), or store a reference-counted object (e.g., std::shared_ptr<Bnd_Box> with atomic_load/atomic_store) so readers never dereference freed or concurrently-mutated memory.

Suggested change

const Bnd_Box* aBox = myCachedMinMax.load(std::memory_order_acquire);

std::lock_guard<std::mutex> aLock(myCachedMinMaxMutex);

const Bnd_Box* aBox = myCachedMinMax.load(std::memory_order_relaxed);

Copilot · 2026-04-01T10:38:35Z

+  std::lock_guard<std::mutex> aLock(myCachedMinMaxMutex);
+  Bnd_Box*                    aBox = myCachedMinMax.load(std::memory_order_relaxed);
+  if (aBox == nullptr)
  {
-    myCachedMinMax = new Bnd_Box();
+    aBox  = new Bnd_Box();
+    *aBox = theBox;
+    myCachedMinMax.store(aBox, std::memory_order_release);
+  }
+  else
+  {
+    *aBox = theBox;
  }


SetCachedMinMax() updates *aBox in place under myCachedMinMaxMutex, but readers (CachedMinMax()/MinMax()) access the same Bnd_Box object without taking that mutex. This creates a data race even if the pointer is never deleted. A safer pattern is to treat cached boxes as immutable and atomically publish a new instance (with safe reclamation), or guard both reads and writes with the same (shared) mutex.

Copilot · 2026-04-01T10:38:35Z

+  if (AlreadyRead.load(std::memory_order_acquire))
+  {
+    return TheFormat;
+  }


GetFormat() has a lock-free fast-path returning TheFormat without holding TheFormatMutex. Since SetFormat()/ReadFormat() can update TheFormat concurrently (even though they take the mutex), this is a data race because TheFormat itself is non-atomic. To keep a lock-free read, make TheFormat a std::atomic<Resource_FormatType> (or store it into an atomic integral) and use appropriate memory ordering; otherwise, always read TheFormat under the mutex.

Suggested change

if (AlreadyRead.load(std::memory_order_acquire))

{

return TheFormat;

}

Copilot · 2026-04-01T10:38:35Z

+  // Fast path: read-only cache lookup under shared lock.
  {
+    std::shared_lock<std::shared_mutex> aReadLock(aMapMutex);
+    if (theMapOfFunctions.Find(pid, f))
+    {
+      // Cast through void* to avoid -Wcast-function-type-mismatch warning.
+      Standard_Transient* (*fp)(const Standard_GUID&) =
+        reinterpret_cast<Standard_Transient* (*)(const Standard_GUID&)>(reinterpret_cast<void*>(f));
+      return (*fp)(aGUID);
+    }
+  }


Plugin::Load() calls the plugin factory while holding aMapMutex (shared lock on the fast path and unique lock on the slow path). If the factory (directly or indirectly) calls Plugin::Load again, this can deadlock because std::shared_mutex is not recursive; it also unnecessarily blocks other threads during potentially heavy plugin initialization. Consider copying the cached OSD_Function under the lock, releasing the lock, and only then calling the factory; similarly unlock the write lock before invoking the newly loaded factory.

Copilot · 2026-04-01T10:38:36Z


-  Standard_EXPORT void SetParallel(bool theIsParallel);
+  //! Sets the parallel execution flag for sub-algorithms.
+  void SetParallel(const bool theIsParallel) { myIsParallel = theIsParallel; }


BRepCheck_Result::SetParallel() was changed from a Standard_EXPORT out-of-line method to an inline, non-exported setter. If OCCT aims to preserve binary compatibility, removing the exported symbol can break downstream binaries that link against SetParallel(). Consider keeping a Standard_EXPORT out-of-line definition (even if it only assigns myIsParallel) to avoid an ABI break.

Suggested change

void SetParallel(const bool theIsParallel) { myIsParallel = theIsParallel; }

Standard_EXPORT void SetParallel(const bool theIsParallel);

…nicode classes by replacing mutex with shared_mutex and optimizing atomic operations

…d-safety improvements

AtheneNoctuaPt · 2026-04-02T09:24:06Z

+static std::mutex                       TheFormatMutex;

-static Resource_FormatType& Resource_Current_Format()
+static void readFormatFromConfig()


Previous version of this function checked and updated AlreadyRead flag, current doesn't. Is this intentional?
Also, unlike in SetFormat() we do not acquire mutex here. Again, not sure if it is intentional or not.

…eBRep_vprdeg

BRepFilletAPI_MakeFillet reconstructs its result solid through the legacy TopOpeBRepBuild engine (ChFi3d_Builder::Compute -> TopOpeBRepBuild_HBuilder:: MergeSolid -> TopOpeBRepBuild_Builder::SplitSolid), which passes state between methods through file-scope static variables. That makes it non-reentrant: two BRepFilletAPI_MakeFillet builds on independent shapes on separate threads corrupt each other and yield a wrong-but-plausible solid that fails BRepCheck. ThreadSanitizer on an 8-thread fuse+fillet stress pinpoints the functional cause as STATIC_SOLIDINDEX (TopOpeBRepBuild_Builder.cxx): SplitSolid sets it to 1/2 to tell FillSolid which operand it is splitting, FillSolid reads it back. Concurrent reconstructions interleave the writes, so FillSolid mis-classifies faces and drops material. Converting that one variable to thread_local makes a 1600-build concurrent stress return correct geometry every time (was ~15-20% corrupt). Converts the fillet-path shared statics to thread_local (each thread keeps its own copy of the cross-call state; single-thread behaviour is unchanged): - Functional: TopOpeBRepBuild_Builder STATIC_SOLIDINDEX; TopOpeBRep_kpart STATIC_lastVPind (same cross-call-cache pattern). - Benign data races on the same path (no corruption, but flagged by TSan): BlendFunc_ConstRad / BlendFunc_EvolRad ComputeValues scratch; ChFi3d_Builder_6 checkcurve. Same class of fix, same engine, as the earlier TKBool thread_local conversion (Open-Cascade-SAS#1180); these were not covered by it. Reported downstream as SecondMouseAU/ OCCTSwift#298.

…1374) BRepFilletAPI_MakeFillet reconstructs its result solid through the legacy TopOpeBRepBuild engine (ChFi3d_Builder::Compute -> TopOpeBRepBuild_HBuilder:: MergeSolid -> TopOpeBRepBuild_Builder::SplitSolid), which passes state between methods through file-scope static variables. That makes it non-reentrant: two BRepFilletAPI_MakeFillet builds on independent shapes on separate threads corrupt each other and yield a wrong-but-plausible solid that fails BRepCheck. ThreadSanitizer on an 8-thread fuse+fillet stress pinpoints the functional cause as STATIC_SOLIDINDEX (TopOpeBRepBuild_Builder.cxx): SplitSolid sets it to 1/2 to tell FillSolid which operand it is splitting, FillSolid reads it back. Concurrent reconstructions interleave the writes, so FillSolid mis-classifies faces and drops material. Converting that one variable to thread_local makes a 1600-build concurrent stress return correct geometry every time (was ~15-20% corrupt). Converts the fillet-path shared statics to thread_local (each thread keeps its own copy of the cross-call state; single-thread behaviour is unchanged): - Functional: TopOpeBRepBuild_Builder STATIC_SOLIDINDEX; TopOpeBRep_kpart STATIC_lastVPind (same cross-call-cache pattern). - Benign data races on the same path (no corruption, but flagged by TSan): BlendFunc_ConstRad / BlendFunc_EvolRad ComputeValues scratch; ChFi3d_Builder_6 checkcurve. Same class of fix, same engine, as the earlier TKBool thread_local conversion (#1180); these were not covered by it. Reported downstream as SecondMouseAU/ OCCTSwift#298.

dpasukhi requested a review from Copilot April 1, 2026 10:32

dpasukhi self-assigned this Apr 1, 2026

github-project-automation Bot added this to Maintenance Apr 1, 2026

github-project-automation Bot moved this to Todo in Maintenance Apr 1, 2026

Copilot started reviewing on behalf of dpasukhi April 1, 2026 10:33 View session

// formatting

ba968f3

Copilot AI reviewed Apr 1, 2026

View reviewed changes

Modeling - Improve thread-safety in Poly_Triangulation and Resource_U…

c0bc0f1

…nicode classes by replacing mutex with shared_mutex and optimizing atomic operations

dpasukhi linked an issue Apr 1, 2026 that may be closed by this pull request

Thread safety research under concurrent OCCT on arm64 macOS (8.0.0-rc4) #1179

Closed

dpasukhi added 2 commits April 1, 2026 18:12

Modeling - Include <mutex> header for consistency in Plugin.cxx

c4e3538

Modeling - Include <mutex> header in Poly_Triangulation.cxx for threa…

7bfd555

…d-safety improvements

AtheneNoctuaPt reviewed Apr 2, 2026

View reviewed changes

dpasukhi added 2 commits April 2, 2026 11:45

Modeling - Enhance thread-local storage for shape processing in TopOp…

fd36697

…eBRep_vprdeg

// forattubg

c093f4a

dpasukhi merged commit 8d2d865 into Open-Cascade-SAS:IR Apr 2, 2026
22 checks passed

dpasukhi deleted the analysis_crashes branch April 2, 2026 16:21

github-project-automation Bot moved this from Todo to Done in Maintenance Apr 2, 2026

gsdali mentioned this pull request Jul 18, 2026

Modeling - Make fillet solid reconstruction reentrant across threads #1374

Merged

This was referenced Aug 27, 2026

Follow-up: Adopt upstream OCCT #1259 for Interface_Static thread-safety (STEP/IGES) SecondMouseAU/OCCTSwift#1191

Closed

Interface_Static process globals in STEP/IGES: Bridge serializes via igesMutex() but kernel fix needed SecondMouseAU/OCCTSwift#1157

Closed

Navigation Menu

Uh oh!

Foundation, Modeling - Fix thread-safety data races for concurrent operations - #1180

Foundation, Modeling - Fix thread-safety data races for concurrent operations#1180
dpasukhi merged 7 commits into
Open-Cascade-SAS:IRfrom
dpasukhi:analysis_crashes

dpasukhi commented Apr 1, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Apr 1, 2026

Uh oh!

Copilot AI Apr 1, 2026

Uh oh!

Copilot AI Apr 1, 2026

Uh oh!

Copilot AI Apr 1, 2026

Uh oh!

Copilot AI Apr 1, 2026

Uh oh!

AtheneNoctuaPt Apr 2, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	const Bnd_Box* aBox = myCachedMinMax.load(std::memory_order_acquire);
	std::lock_guard<std::mutex> aLock(myCachedMinMaxMutex);
	const Bnd_Box* aBox = myCachedMinMax.load(std::memory_order_relaxed);

	void SetParallel(const bool theIsParallel) { myIsParallel = theIsParallel; }
	Standard_EXPORT void SetParallel(const bool theIsParallel);

Uh oh!

Conversation

dpasukhi commented Apr 1, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Apr 1, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 1, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 1, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 1, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 1, 2026

Choose a reason for hiding this comment

Uh oh!

AtheneNoctuaPt Apr 2, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants