DataExchange, XCAF - AutoNamingScope guards XCAFDoc_ShapeTool::theAutoNaming

[gsdali]

Summary

XCAFDoc_ShapeTool::theAutoNaming is a process-global static bool (documented as such — "This
setting is global; it cannot be made a member function") that three independent call sites each
save/modify/restore around their own work with no locking:

• RWMesh_CafReader::fillDocument() — shared base of RWObj_CafReader and RWGltf_CafReader, so
  reachable from both OBJ and glTF import
• RWGltf_CafReader::fillDocument() — a separate, near-duplicate override, not a call into the
  base class's version
• XCAFDoc_Editor::Expand() — additionally recurses into itself while the save/restore is in flight

XCAFDoc_ShapeTool::AddShape/addShape reads the same flag internally to decide whether to
auto-generate a name.

ThreadSanitizer on a concurrent-OBJ-import stress (8-10 threads, each writing/reading its own
uniquely-named file — not a file-path collision) reports 9-17 races per run on theAutoNaming,
resolving to two distinct problems:

1. Two threads' save/modify/restore critical sections can interleave, so one thread's restore
   stomps another's still-in-progress override — a logical bug (wrong auto-naming mode active for
   part of a build), independent of memory safety.
2. theAutoNaming itself is a plain bool, read and written with no synchronization from any
   caller — including ordinary AddShape calls made outside all three save/restore sites above — a
   genuine data race on every access.

Fix

Two parts, both needed:

1. XCAFDoc_ShapeTool::AutoNamingScope — a new RAII helper (AutoNamingScope(bool) constructor /
   destructor) that holds a std::recursive_mutex for its entire lifetime, not just around the
   individual get/set calls, so two threads' save-modify-restore sequences serialize instead of
   interleaving. Recursive because XCAFDoc_Editor::Expand() reenters it on the same thread. All
   three call sites now use it; Expand()'s two duplicate manual-restore-before-return sites
   collapse into one destructor-driven restore that fires on every exit path.
2. theAutoNaming is now std::atomic<bool> instead of a plain bool, so every access anywhere in
   the file — including AddShape's internal read, which participates in none of the three scoped
   sections — is well-defined, closing the gap the mutex alone doesn't reach.

Testing

Minimal-module ThreadSanitizer build (FoundationClasses+ModelingData+ModelingAlgorithms+
DataExchange, RelWithDebInfo, -fsanitize=thread). A pure-C++ harness runs N threads each
building a box, meshing it, writing it to its own uniquely-named .obj file (RWObj_CafWriter),
and reading it back (RWObj_CafReader) into a fresh TDocStd_Document.

• Before: 10 threads × 200 iterations reports 9-17 theAutoNaming races per run (4 runs).
• After: 0 theAutoNaming races across the same stress, 4 separate runs — only an unrelated,
  pre-existing Message_PrinterOStream/std::cout console-write race remains (OCCT's default
  messenger has no internal locking; out of scope for this PR).
• Zero regression on unrelated concurrent-shape-creation and independent-meshing scenarios.
• RWGltf_CafReader's copy of the fix compiles cleanly (mechanically identical to the
  RWMesh_CafReader path that was exercised under TSan) but wasn't run under TSan directly in this
  session — the minimal-module build excludes TKDEGLTF (needs RapidJSON, disabled for build speed).

No behavior change for the existing (documented) semantics of SetAutoNaming/AutoNaming as a
single process-wide setting — this only serializes the internal save/modify/restore idiom and makes
the underlying storage well-defined.

Fixes #1387.

[gkv311]

@gsdali

I think that a mutex is not the right tool here and I don't quite understand how current patch fixes concurrency issue as theAutoNaming usage remains unprotected within XCAFDoc_ShapeTool.cpp itself.

What I would suggest is to add local property overriding global value as a XCAFDoc_ShapeTool class field and use it in specific places. I have prototyped the patch for an older OCCT - fill free to borrow the idea for your patch:

---------------------- src/XCAFDoc/XCAFDoc_ShapeTool.hxx
   Standard_EXPORT static Standard_Boolean AutoNaming();
-  
+
+  //! Return autonaming flag for this tool instance.
+  Standard_Boolean OwnAutoNaming() const
+  {
+    return myOwnAutonaming == -1 ? XCAFDoc_ShapeTool::AutoNaming() : myOwnAutonaming == 1;
+  }
+
+  //! Set autonaming flag for this tool instance.
+  void SetOwnAutoNaming (Standard_Boolean theOwnFlag)
+  {
+    myOwnAutonaming = theOwnFlag ? 1 : 0;
+  }
+
+  //! Reset autonaming flag for this tool instance to global state.
+  void UnsetOwnAutoNaming()
+  {
+    myOwnAutonaming = -1;
+  }
...
   //! Makes a shape on label L to be a reference to shape refL
   //! with location loc
-  Standard_EXPORT static void MakeReference (const TDF_Label& L, const TDF_Label& refL, const TopLoc_Location& loc);
+  Standard_EXPORT void MakeReference (const TDF_Label& L, const TDF_Label& refL, const TopLoc_Location& loc);
...
-  Standard_Boolean hasSimpleShapes;
-
+  Standard_Boolean hasSimpleShapes = false;
+  //! Define own autonaming property; -1 by default which means to use global flag
+  Standard_Integer myOwnAutonaming = -1;
 
 };
...
--------------------- src/XCAFDoc/XCAFDoc_ShapeTool.cxx ----------------------
index 3b36233880..b3357c9d65 100644
@@ -454,7 +454,7 @@ void XCAFDoc_ShapeTool::MakeReference (const TDF_Label &L,
   refNode->Remove(); // abv: fix against bug in TreeNode::Append()
   mainNode->Append(refNode);
 
-  if (theAutoNaming)
+  if (OwnAutoNaming())
     SetLabelNameByLink(L);
 }
 
@@ -529,7 +529,7 @@ TDF_Label XCAFDoc_ShapeTool::addShape (const TopoDS_Shape& S, const Standard_Boo
 //  }
   A->SetShape(S);
   
-  if (theAutoNaming)
+  if (OwnAutoNaming())
     SetLabelNameByShape(ShapeLabel);
 
   // if shape is Compound and flag is set, create assembly
@@ -537,7 +537,7 @@ TDF_Label XCAFDoc_ShapeTool::addShape (const TopoDS_Shape& S, const Standard_Boo
     // mark assembly by assigning UAttribute
     Handle(TDataStd_UAttribute) Uattr;
     Uattr = TDataStd_UAttribute::Set ( ShapeLabel, XCAFDoc::AssemblyGUID() );
-    if (theAutoNaming)
+    if (OwnAutoNaming())
       TDataStd_Name::Set(ShapeLabel, TCollection_ExtendedString("ASSEMBLY"));
 
     // iterate on components
@@ -1564,7 +1564,7 @@ Standard_Boolean XCAFDoc_ShapeTool::SetSHUO (const TDF_LabelSequence& labels,
   
   TDF_TagSource aTag;
   TDF_Label UpperSubL = aTag.NewChild( labels( 1 ) );
-  if (theAutoNaming) {
+  if (OwnAutoNaming()) {
     TCollection_ExtendedString Entry("SHUO");
     TDataStd_Name::Set(UpperSubL, TCollection_ExtendedString( Entry ));
   }
@@ -1575,7 +1575,7 @@ Standard_Boolean XCAFDoc_ShapeTool::SetSHUO (const TDF_LabelSequence& labels,
   // add other next_usage occurrences.
   for (i = 2; i <= labels.Length(); i++) {
     TDF_Label NextSubL = aTag.NewChild( labels( i ) );
-    if (theAutoNaming) {
+    if (OwnAutoNaming()) {

------------------------ src/XCAFDoc/XCAFDoc_Editor.cxx ------------------------
index 6897238bc3..b18adf6217 100644
@@ -68,8 +68,7 @@ Standard_Boolean XCAFDoc_Editor::Expand (const TDF_Label& theDoc,
     return Standard_False;
   }
   Handle(XCAFDoc_ShapeTool) aShapeTool = XCAFDoc_DocumentTool::ShapeTool(theDoc);
-  Standard_Boolean isAutoNaming = aShapeTool->AutoNaming();
-  aShapeTool->SetAutoNaming(Standard_False);
+  aShapeTool->SetOwnAutoNaming(false);
 
   TDF_Label aCompoundPartL = theShape;
   if (aShapeTool->IsReference(theShape))
@@ -137,10 +136,10 @@ Standard_Boolean XCAFDoc_Editor::Expand (const TDF_Label& theDoc,
         }
       }
     }
-    aShapeTool->SetAutoNaming(isAutoNaming);
+    aShapeTool->UnsetOwnAutoNaming();
     return Standard_True;
   }
-  aShapeTool->SetAutoNaming(isAutoNaming);
+  aShapeTool->UnsetOwnAutoNaming();
   return Standard_False;
 }
----------------------- src/RWMesh/RWMesh_CafReader.cxx -----------------------
index 4164e4496f..d021a51043 100644
@@ -178,11 +178,10 @@ void RWMesh_CafReader::fillDocument()
     Message::SendWarning("Warning: Length unit of document not equal to the system length unit");
   }
 
-  const Standard_Boolean wasAutoNaming = XCAFDoc_ShapeTool::AutoNaming();
-  XCAFDoc_ShapeTool::SetAutoNaming (Standard_False);
   const TCollection_AsciiString aRootName; // = generateRootName (theFile);
   CafDocumentTools aTools;
   aTools.ShapeTool = XCAFDoc_DocumentTool::ShapeTool (myXdeDoc->Main());
+  aTools.ShapeTool->SetOwnAutoNaming (false);
   aTools.ColorTool = XCAFDoc_DocumentTool::ColorTool (myXdeDoc->Main());
   aTools.VisMaterialTool = XCAFDoc_DocumentTool::VisMaterialTool (myXdeDoc->Main());
   for (TopTools_SequenceOfShape::Iterator aRootIter (myRootShapes); aRootIter.More(); aRootIter.Next())
@@ -190,7 +189,7 @@ void RWMesh_CafReader::fillDocument()
     addShapeIntoDoc (aTools, aRootIter.Value(), TDF_Label(), aRootName);
   }
   XCAFDoc_DocumentTool::ShapeTool (myXdeDoc->Main())->UpdateAssemblies();
-  XCAFDoc_ShapeTool::SetAutoNaming (wasAutoNaming);
+  aTools.ShapeTool->UnsetOwnAutoNaming();
 }

In this way, the API with the global setting will remain working, while allowing to disable auto-naming locally within specific algorithms.

[gsdali]

@gkv311 Thank you for the review — and you're right, a mutex isn't the right tool here.

We'd used a mutex-guarded save/restore successfully for a couple of similar unsynchronized-global-state issues elsewhere in this codebase, so that was the pattern we defaulted to without stopping to ask whether the state should have been global in the first place. In hindsight this is a clearly better fix: theAutoNaming was only ever being overridden by three call sites that each want to suppress naming for their own document build, not change a process-wide setting — so the override belongs on XCAFDoc_ShapeTool (already one instance per document), not on a shared flag. That's the original intent the three call sites were reaching for anyway; the global was the wrong tool even before concurrency entered the picture.

Pushed an update using your suggested design: myOwnAutonaming (tri-state, -1 inherits the process-wide default) plus OwnAutoNaming()/SetOwnAutoNaming()/UnsetOwnAutoNaming(), with AddShape/MakeReference/SetSHUO reading OwnAutoNaming() instead of the flag directly. One thing worth flagging since it wasn't obvious from the one-line sketch: XCAFDoc_Editor::Expand() recurses into itself on the same document, so a bare SetOwnAutoNaming()/UnsetOwnAutoNaming() pair at entry/exit would clobber an outer call's override mid-recursion (the inner call's unconditional "reset to inherit global" would win over the outer call's still-active override). OwnAutoNamingScope does a proper save/restore of whatever override state the instance had on entry — same shape as the RAII scope this replaces, just scoped to the instance instead of a process-wide flag, so nesting composes correctly with no lock needed either way.

Re-verified with the same ThreadSanitizer stress as before (10 threads x 200 iterations) — zero theAutoNaming races, same result as the previous fix. Also added a stress scenario for the specific gap you flagged: half the threads locally override via OwnAutoNamingScope on their own document while the other half do plain unscoped AddShape() on independent documents relying on the process-wide default, concurrently — 3000 operations, zero instances of the unscoped threads observing another thread's override.