Step thread safety improvement #307

Mutex is added to XSControl_WorkSession to prevent data races
during reading and writing.
Tests are added to check the behavior of STEP readers/writers in
multithreading environment.

src/AIS/AIS_Manipulator.cxx

@@ -331,15 +331,6 @@ void AIS_Manipulator::EnableMode(const AIS_ManipulatorMode theMode)
 
 //=================================================================================================
 
-void AIS_Manipulator::attachToPoint(const gp_Pnt& thePoint)
-{
-  gp_Ax2 aPosition = gp::XOY();
-  aPosition.SetLocation(thePoint);
-  SetPosition(aPosition);
-}
-
-//=================================================================================================
-
 void AIS_Manipulator::attachToBox(const Bnd_Box& theBox)
 {
   if (theBox.IsVoid())
@@ -401,15 +392,7 @@ void AIS_Manipulator::Attach(const Handle(AIS_ManipulatorObjectSequence)& theObj
 
   if (theOptions.AdjustPosition)
   {
-    const Handle(Graphic3d_TransformPers)& aTransPers = aCurObject->TransformPersistence();
-    if (!aTransPers.IsNull() && (aTransPers->IsZoomOrRotate() || aTransPers->IsAxial()))
-    {
-      attachToPoint(aTransPers->AnchorPoint());
-    }
-    else
-    {
-      attachToBox(aBox);
-    }
+    attachToBox(aBox);
   }
 
   if (theOptions.AdjustSize)
@@ -763,18 +746,9 @@ void AIS_Manipulator::Transform(const gp_Trsf& theTrsf)
     NCollection_Sequence<gp_Trsf>::Iterator aTrsfIter(myStartTrsfs);
     for (; anObjIter.More(); anObjIter.Next(), aTrsfIter.Next())
     {
-      const Handle(AIS_InteractiveObject)&   anObj      = anObjIter.ChangeValue();
-      const Handle(Graphic3d_TransformPers)& aTransPers = anObj->TransformPersistence();
-      if (!aTransPers.IsNull() && (aTransPers->IsZoomOrRotate() || aTransPers->IsAxial()))
-      {
-        gp_XYZ aNewAnchorPoint = aTransPers->AnchorPoint().XYZ() - myPosition.Location().XYZ();
-        aNewAnchorPoint += myStartPosition.Location().Transformed(theTrsf).XYZ();
-        aTransPers->SetAnchorPoint(aNewAnchorPoint);
-        continue;
-      }
-
-      const gp_Trsf&                anOldTrsf   = aTrsfIter.Value();
-      const Handle(TopLoc_Datum3D)& aParentTrsf = anObj->CombinedParentTransformation();
+      const Handle(AIS_InteractiveObject)& anObj       = anObjIter.ChangeValue();
+      const gp_Trsf&                       anOldTrsf   = aTrsfIter.Value();
+      const Handle(TopLoc_Datum3D)&        aParentTrsf = anObj->CombinedParentTransformation();
       if (!aParentTrsf.IsNull() && aParentTrsf->Form() != gp_Identity)
       {
         // recompute local transformation relative to parent transformation

src/AIS/AIS_Manipulator.hxx

@@ -416,8 +416,6 @@ protected:
   Standard_EXPORT Handle(Graphic3d_Group) getGroup(const Standard_Integer    theIndex,
                                                    const AIS_ManipulatorMode theMode) const;
 
-  Standard_EXPORT void attachToPoint(const gp_Pnt& thePoint);
-
   Standard_EXPORT void attachToBox(const Bnd_Box& theBox);
 
   Standard_EXPORT void adjustSize(const Bnd_Box& theBox);

src/QABugs/QABugs_20.cxx

@@ -66,6 +66,9 @@
 #include <TDataStd_Name.hxx>
 #include <AppCont_Function.hxx>
 #include <math_ComputeKronrodPointsAndWeights.hxx>
+#include <STEPCAFControl_Writer.hxx>
+#include <STEPCAFControl_Controller.hxx>
+#include <ShapeAnalysis_ShapeContents.hxx>
 
 #include <limits>
 
@@ -4923,6 +4926,155 @@ static Standard_Integer OCC33048(Draw_Interpretor&, Standard_Integer, const char
   return 0;
 }
 
+//=================================================================================================
+
+static Standard_Integer OCC33657_1(Draw_Interpretor&, Standard_Integer, const char**)
+{
+  STEPCAFControl_Controller::Init();
+  // Checking constructors working in parallel.
+  OSD_Parallel::For(0, 1000, [](int) {
+    STEPCAFControl_Reader aReader;
+    aReader.SetColorMode(true);
+    STEPCAFControl_Writer aWriter;
+    aWriter.SetDimTolMode(true);
+  });
+
+  return 0;
+}
+
+//=================================================================================================
+
+static Standard_Integer OCC33657_2(Draw_Interpretor& theDI,
+                                   Standard_Integer  theArgC,
+                                   const char**      theArgV)
+{
+  if (theArgC < 2)
+  {
+    theDI << "Use: " << theArgV[0] << " file\n";
+    return 1;
+  }
+
+  STEPCAFControl_Controller::Init();
+  // Checking readers working in parallel.
+  OSD_Parallel::For(0, 100, [&](int) {
+    STEPControl_Reader aReader;
+    aReader.ReadFile(theArgV[1], DESTEP_Parameters{});
+    aReader.TransferRoots();
+  });
+
+  return 0;
+}
+
+//=================================================================================================
+
+static Standard_Integer OCC33657_3(Draw_Interpretor&, Standard_Integer, const char**)
+{
+  STEPCAFControl_Controller::Init();
+  const TopoDS_Shape aShape = BRepPrimAPI_MakeBox(10.0, 20.0, 30.0).Shape();
+  // Checking writers working in parallel.
+  OSD_Parallel::For(0, 100, [&](int) {
+    STEPControl_Writer aWriter;
+    aWriter.Transfer(aShape, STEPControl_StepModelType::STEPControl_AsIs, DESTEP_Parameters{});
+    std::ostringstream aStream;
+    aWriter.WriteStream(aStream);
+  });
+
+  return 0;
+}
+
+//=================================================================================================
+
+static Standard_Integer OCC33657_4(Draw_Interpretor& theDI,
+                                   Standard_Integer  theArgC,
+                                   const char**      theArgV)
+{
+  if (theArgC < 2)
+  {
+    theDI << "Use: " << theArgV[0] << " file\n";
+    return 1;
+  }
+
+  STEPCAFControl_Controller::Init();
+
+  // Acquire shape to write/read.
+  STEPControl_Reader aReader;
+  aReader.ReadFile(theArgV[1], DESTEP_Parameters{});
+  aReader.TransferRoots();
+  TopoDS_Shape aSourceShape = aReader.OneShape();
+
+  // Analyzer to compare the shape with the the same shape after write-read sequence.
+  ShapeAnalysis_ShapeContents aSourceAnalyzer;
+  aSourceAnalyzer.Perform(aSourceShape);
+
+  // Flag is set to false if any error is detected.
+  // Reads and writes to the flag are performed exclusively in relaxed memory order
+  // in order to avoid inter-thread syncronization that can potentially omit some problems.
+  std::atomic_bool anErrorOccurred(false);
+
+  OSD_Parallel::For(0, 100, [&](int) {
+    if (anErrorOccurred.load(std::memory_order_relaxed))
+    {
+      return;
+    }
+
+    // Writing.
+    STEPControl_Writer aWriter;
+    aWriter.Transfer(aSourceShape,
+                     STEPControl_StepModelType::STEPControl_AsIs,
+                     DESTEP_Parameters{});
+    std::stringstream aStream;
+    aWriter.WriteStream(aStream);
+
+    // Reading.
+    STEPControl_Reader aReader;
+    aReader.ReadStream("", DESTEP_Parameters{}, aStream);
+    aReader.TransferRoots();
+    const TopoDS_Shape          aResultShape = aReader.OneShape();
+    ShapeAnalysis_ShapeContents aResultAnalyzer;
+    aResultAnalyzer.Perform(aResultShape);
+
+    // Making sure that shape is unchanged.
+    if (aSourceAnalyzer.NbSolids() != aResultAnalyzer.NbSolids())
+    {
+      theDI << "Error: Wrong number of solids in the result shape.\nExpected: "
+            << aSourceAnalyzer.NbSolids() << "\nActual" << aResultAnalyzer.NbSolids() << "\n";
+      anErrorOccurred.store(true, std::memory_order_relaxed);
+    }
+    if (aSourceAnalyzer.NbShells() != aResultAnalyzer.NbShells())
+    {
+      theDI << "Error: Wrong number of shells in the result shape.\nExpected: "
+            << aSourceAnalyzer.NbShells() << "\nActual" << aResultAnalyzer.NbShells() << "\n";
+      anErrorOccurred.store(true, std::memory_order_relaxed);
+    }
+    if (aSourceAnalyzer.NbFaces() != aResultAnalyzer.NbFaces())
+    {
+      theDI << "Error: Wrong number of faces in the result shape.\nExpected: "
+            << aSourceAnalyzer.NbFaces() << "\nActual" << aResultAnalyzer.NbFaces() << "\n";
+      anErrorOccurred.store(true, std::memory_order_relaxed);
+    }
+    if (aSourceAnalyzer.NbWires() != aResultAnalyzer.NbWires())
+    {
+      theDI << "Error: Wrong number of wires in the result shape.\nExpected: "
+            << aSourceAnalyzer.NbWires() << "\nActual" << aResultAnalyzer.NbWires() << "\n";
+      anErrorOccurred.store(true, std::memory_order_relaxed);
+    }
+    if (aSourceAnalyzer.NbEdges() != aResultAnalyzer.NbEdges())
+    {
+      theDI << "Error: Wrong number of edges in the result shape.\nExpected: "
+            << aSourceAnalyzer.NbEdges() << "\nActual" << aResultAnalyzer.NbEdges() << "\n";
+      anErrorOccurred.store(true, std::memory_order_relaxed);
+    }
+    if (aSourceAnalyzer.NbVertices() != aResultAnalyzer.NbVertices())
+    {
+      theDI << "Error: Wrong number of vertices in the result shape.\nExpected: "
+            << aSourceAnalyzer.NbVertices() << "\nActual" << aResultAnalyzer.NbVertices() << "\n";
+      anErrorOccurred.store(true, std::memory_order_relaxed);
+    }
+  });
+
+  return anErrorOccurred;
+}
+
 //=======================================================================
 // function : QACheckBends
 // purpose :
@@ -5283,5 +5435,30 @@ void QABugs::Commands_20(Draw_Interpretor& theCommands)
                   OCC26441,
                   group);
 
+  theCommands.Add(
+    "OCC33657_1",
+    "Check performance of STEPCAFControl_Reader/Writer constructors in multithreading environment.",
+    __FILE__,
+    OCC33657_1,
+    group);
+
+  theCommands.Add("OCC33657_2",
+                  "Check performance of STEPControl_Reader in multithreading environment.",
+                  __FILE__,
+                  OCC33657_2,
+                  group);
+
+  theCommands.Add("OCC33657_3",
+                  "Check performance of STEPControl_Writer in multithreading environment.",
+                  __FILE__,
+                  OCC33657_3,
+                  group);
+
+  theCommands.Add("OCC33657_4",
+                  "Check performance of STEPControl_Reader/Writer in multithreading environment.",
+                  __FILE__,
+                  OCC33657_4,
+                  group);
+
   return;
 }

src/StepFile/StepFile_Read.cxx

@@ -31,6 +31,7 @@
 
 #include <Standard_ErrorHandler.hxx>
 #include <Standard_Failure.hxx>
+#include <Standard_Mutex.hxx>
 
 #include <Message.hxx>
 #include <Message_Messenger.hxx>
@@ -46,6 +47,11 @@
   #define CHRONOMESURE
 #endif
 
+namespace
+{
+static Standard_Mutex THE_GLOBAL_READ_MUTEX;
+}
+
 void StepFile_Interrupt(Standard_CString theErrorMessage, const Standard_Boolean theIsFail)
 {
   if (theErrorMessage == NULL)
@@ -113,7 +119,8 @@ static Standard_Integer StepFile_Read(const char*                            the
 
   sout << "      ...    STEP File   Read    ...\n";
 
-  Standard_Integer nbhead, nbrec, nbpar;
+  Standard_Mutex::Sentry aLocker(THE_GLOBAL_READ_MUTEX);
+  Standard_Integer       nbhead, nbrec, nbpar;
   aFileDataModel.GetFileNbR(&nbhead, &nbrec, &nbpar); // renvoi par lex/yacc
   Handle(StepData_StepReaderData) undirec =
     // clang-format off

src/ViewerTest/ViewerTest_ViewerCommands.cxx

@@ -12083,8 +12083,7 @@ static int VManipulator(Draw_Interpretor& theDi, Standard_Integer theArgsNb, con
   NCollection_Sequence<ManipAxisModeOnOff> aParts;
   gp_XYZ aLocation(RealLast(), RealLast(), RealLast()), aVDir, anXDir;
   //
-  bool                              toDetach    = false;
-  bool                              toAddObject = false;
+  bool                              toDetach = false;
   AIS_Manipulator::OptionsForAttach anAttachOptions;
   Handle(AIS_InteractiveObject)     anAttachObject;
   Handle(V3d_View)                  aViewAffinity;
@@ -12274,10 +12273,6 @@ static int VManipulator(Draw_Interpretor& theDi, Standard_Integer theArgsNb, con
       aTrsf.SetRotation(gp_Ax1(gp_Pnt(aRotPnt), gp_Dir(aRotAxis)), aTmpReal);
     }
     //
-    else if (anArg == "-addobject")
-    {
-      toAddObject = true;
-    }
     else if (anArg == "-detach")
     {
       toDetach = true;
@@ -12435,16 +12430,7 @@ static int VManipulator(Draw_Interpretor& theDi, Standard_Integer theArgsNb, con
 
   if (!anAttachObject.IsNull())
   {
-    if (toAddObject && aManipulator->IsAttached())
-    {
-      Handle(AIS_ManipulatorObjectSequence) anAttachObjects = aManipulator->Objects();
-      anAttachObjects->Append(anAttachObject);
-      aManipulator->Attach(anAttachObjects, anAttachOptions);
-    }
-    else
-    {
-      aManipulator->Attach(anAttachObject, anAttachOptions);
-    }
+    aManipulator->Attach(anAttachObject, anAttachOptions);
   }
   if (!aViewAffinity.IsNull())
   {
@@ -14504,7 +14490,6 @@ Options:
  '-enableModes    {0|1}'             enable modes when attaching
  '-view  {active | [name of view]}'  display manipulator only in defined view,
                                      by default it is displayed in all views of the current viewer
- '-addObject                         allows attach manipulator to multiple objects (replace by default)
  '-detach'                           detach manipulator
  '-startTransform mouse_x mouse_y' - invoke start of transformation
  '-transform      mouse_x mouse_y' - invoke transformation

src/XSControl/XSControl_WorkSession.cxx

@@ -32,6 +32,9 @@
 
 IMPLEMENT_STANDARD_RTTIEXT(XSControl_WorkSession, IFSelect_WorkSession)
 
+// Initializing static mutex.
+Standard_Mutex XSControl_WorkSession::myGlobalMutex;
+
 //=================================================================================================
 
 XSControl_WorkSession::XSControl_WorkSession()
@@ -67,6 +70,7 @@ void XSControl_WorkSession::ClearData(const Standard_Integer mode)
 
 Standard_Boolean XSControl_WorkSession::SelectNorm(const Standard_CString normname)
 {
+  const Standard_Mutex::Sentry aMutexLock(myGlobalMutex);
   // Old norm and results
   myTransferReader->Clear(-1);
   //  ????  En toute rigueur, menage a faire dans XWS : virer les items
@@ -424,6 +428,7 @@ Standard_Integer XSControl_WorkSession::TransferReadRoots(const Message_Progress
 
 Handle(Interface_InterfaceModel) XSControl_WorkSession::NewModel()
 {
+  const Standard_Mutex::Sentry     aMutexLock(myGlobalMutex);
   Handle(Interface_InterfaceModel) newmod;
   if (myController.IsNull())
     return newmod;
@@ -446,7 +451,8 @@ IFSelect_ReturnStatus XSControl_WorkSession::TransferWriteShape(
   const Standard_Boolean       compgraph,
   const Message_ProgressRange& theProgress)
 {
-  IFSelect_ReturnStatus status;
+  const Standard_Mutex::Sentry aMutexLock(myGlobalMutex);
+  IFSelect_ReturnStatus        status;
   if (myController.IsNull())
     return IFSelect_RetError;
   const Handle(Interface_InterfaceModel)& model = Model();

src/XSControl/XSControl_WorkSession.hxx

@@ -196,11 +196,14 @@ private:
   //! Clears binders
   Standard_EXPORT void ClearBinders();
 
+private:
   Handle(XSControl_Controller)     myController;
   Handle(XSControl_TransferReader) myTransferReader;
   Handle(XSControl_TransferWriter) myTransferWriter;
   XSControl_WorkSessionMap         myContext;
   Handle(XSControl_Vars)           myVars;
+
+  static Standard_Mutex myGlobalMutex; //!< Mutex to prevent data races during reading and writing.
 };
 
 #endif // _XSControl_WorkSession_HeaderFile

tests/bugs/step/begin

@@ -1,4 +1,5 @@
 pload XDE
+pload QAcommands
 
 set subgroup step
 

tests/bugs/step/bug33657_1

@@ -0,0 +1,3 @@
+# Check performance of STEPCAFControl_Reader/Writer constructors in multithreading environment.
+# If no crash occures, its fine.
+OCC33657_1

tests/bugs/step/bug33657_2

@@ -0,0 +1,3 @@
+# Check performance of STEPControl_Reader in multithreading environment.
+# If no crash occures, its fine.
+OCC33657_2 [locate_data_file bug21802_as1-oc-214.stp]

tests/bugs/step/bug33657_3

@@ -0,0 +1,3 @@
+# Check performance of STEPControl_Writer in multithreading environment.
+# If no crash occures, its fine.
+OCC33657_1

tests/bugs/step/bug33657_4

@@ -0,0 +1,2 @@
+# Check performance of STEPControl_Reader/Writer in multithreading environment.
+OCC33657_4 [locate_data_file bug21802_as1-oc-214.stp]

tests/v3d/bugs/bug32116

@@ -1,41 +0,0 @@
-puts "============"
-puts "0032116: Visualization - AIS_Manipulator is unusable when attaching to objects with Graphic3d_TMF_ZoomPers"
-puts "============"
-puts ""
-
-pload MODELING VISUALIZATION
-
-box b1 10 10 10 10 20 30
-box b2 50 50 50 10 20 30
-
-vinit View1
-vdisplay b1 -dispmode 1 -trsfPers zoom -trsfPersPos 0 0 0
-vdisplay b2 -dispmode 1 -trsfPers zoom -trsfPersPos 10 10 10
-vfit
-
-vmanipulator m -attach b1 -addObject
-vmanipulator m -attach b2 -addObject
-
-set mouse_pick {90 225}
-set mouse_drag {90 150}
-
-vmoveto {*}$mouse_pick
-vselect {*}$mouse_pick
-vmanipulator m -startTransform {*}$mouse_pick
-vmanipulator m -transform {*}$mouse_drag
-vmanipulator m -stopTransform
-vmoveto {*}$mouse_drag
-
-if { [vreadpixel {*}$mouse_drag -rgb -name] != "CYAN" } { puts "Error: wrong manipulator position" }
-if { [vreadpixel 115 170 -rgb -name] != "DARKGOLDENROD" } { puts "Error: wrong b1 tranformation" }
-if { [vreadpixel 400 10 -rgb -name] != "DARKGOLDENROD" } { puts "Error: wrong b2 tranformation" }
-
-vdump ${imagedir}/${casename}_1.png
-
-vmoveto 0 0
-vzoom 0.2
-
-if { [vreadpixel 210 170 -rgb -name] != "DARKGOLDENROD" } { puts "Error: wrong b1 tranformation" }
-if { [vreadpixel 310 120 -rgb -name] != "DARKGOLDENROD" } { puts "Error: wrong b2 tranformation" }
-
-vdump ${imagedir}/${casename}_2.png