Fix stack overflows in face traversal and STEP reference cycles, set the version to 1.7.0 (#486, #487)

CADability/BRepIntersection.cs

@@ -9633,18 +9633,26 @@ internal static Dictionary<int, List<int>> SortCycles(List<List<Edge>> cycles, F
         /// <returns>the faces connected to <paramref name="startWith"/>, including it; they are removed from <paramref name="allFaces"/></returns>
         internal static HashSet<Face> extractConnectedFaces(HashSet<Face> allFaces, Face startWith)
         {
+            // depth first search with an explicit stack: the recursion was as deep as the number of connected faces
+            // and overflowed the stack for shells of some ten thousand faces (#486). The faces are visited in the
+            // same order as by the recursion.
             HashSet<Face> result = new HashSet<Face>();
             result.Add(startWith);
             allFaces.Remove(startWith);
-            foreach (Edge edge in startWith.Edges)
+            Stack<(Edge[] edges, int next)> stack = new Stack<(Edge[] edges, int next)>();
+            stack.Push((startWith.Edges.ToArray(), 0));
+            while (stack.Count > 0)
             {
-                if (allFaces.Contains(edge.SecondaryFace) && edge.IsOrientedConnection)
+                (Edge[] edges, int next) = stack.Pop();
+                if (next >= 2 * edges.Length) continue;
+                stack.Push((edges, next + 1));
+                Edge edge = edges[next / 2];
+                Face face = (next % 2 == 0) ? edge.SecondaryFace : edge.PrimaryFace;
+                if (allFaces.Contains(face) && edge.IsOrientedConnection)
                 {
-                    result.UnionWith(extractConnectedFaces(allFaces, edge.SecondaryFace));
-                }
-                if (allFaces.Contains(edge.PrimaryFace) && edge.IsOrientedConnection)
-                {
-                    result.UnionWith(extractConnectedFaces(allFaces, edge.PrimaryFace));
+                    result.Add(face);
+                    allFaces.Remove(face);
+                    stack.Push((face.Edges.ToArray(), 0));
                 }
             }
             return result;
@@ -11325,12 +11333,24 @@ public Shell[] Result(bool allowOpenEdges)
 
         private void collectFaces(Face startWith, HashSet<Face> unusedfaces, HashSet<Face> connected)
         {
+            // iterative like extractConnectedFaces, to not overflow the stack with many faces (#486)
             unusedfaces.Remove(startWith);
             connected.Add(startWith);
-            foreach (Edge edg in startWith.Edges)
+            Stack<(Edge[] edges, int next)> stack = new Stack<(Edge[] edges, int next)>();
+            stack.Push((startWith.Edges.ToArray(), 0));
+            while (stack.Count > 0)
             {
-                if (edg.PrimaryFace != null && unusedfaces.Contains(edg.PrimaryFace)) collectFaces(edg.PrimaryFace, unusedfaces, connected);
-                if (edg.SecondaryFace != null && unusedfaces.Contains(edg.SecondaryFace)) collectFaces(edg.SecondaryFace, unusedfaces, connected);
+                (Edge[] edges, int next) = stack.Pop();
+                if (next >= 2 * edges.Length) continue;
+                stack.Push((edges, next + 1));
+                Edge edg = edges[next / 2];
+                Face face = (next % 2 == 0) ? edg.PrimaryFace : edg.SecondaryFace;
+                if (face != null && unusedfaces.Contains(face))
+                {
+                    unusedfaces.Remove(face);
+                    connected.Add(face);
+                    stack.Push((face.Edges.ToArray(), 0));
+                }
             }
         }
 

CADability/CADability.csproj

@@ -3,7 +3,7 @@
   <PropertyGroup>
     <TargetFramework>netstandard2.0</TargetFramework>
     <PackageId>CADability</PackageId>
-    <Version>1.6.0</Version>
+    <Version>1.7.0</Version>
     <!-- Update this for each publish -->
     <Authors>FriendsOfCADability</Authors>
     <Description>CAD functionality for .NET</Description>

CADability/ImportStep.cs

@@ -414,6 +414,7 @@ public class ImportStep
 		HashSet<Item> products = new HashSet<Item>();
 		HashSet<Item> mappedItems = new HashSet<Item>();
 		private Dictionary<int, string> importProblems;
+		private System.Collections.Concurrent.ConcurrentDictionary<int, Item> waitingFor = new System.Collections.Concurrent.ConcurrentDictionary<int, Item>(); // thread id -> item the thread waits for in CreateEntity
 		private List<Item> notImportedFaces;
 		private Dictionary<string, ColorDef> definedColors;
 		private System.Collections.Concurrent.ConcurrentDictionary<string, IGeoObject> namedGeoObjects = new System.Collections.Concurrent.ConcurrentDictionary<string, IGeoObject>();
@@ -694,6 +695,10 @@ public enum ItemType
 			public object val;
 			public Dictionary<string, Item> parameter;
 			public int definingIndex;
+			/// <summary>
+			/// Managed thread id of the thread that is creating this item in <see cref="CreateEntity"/>, 0 when none
+			/// </summary>
+			public volatile int creatingThread;
 #if DEBUG
 			public List<Item> usedBy;
 			public void SetUsedBy(Item item)
@@ -2322,12 +2327,22 @@ private object CreateEntity(Item item)
 #endif
 			if (!(item.val is List<Item>)) return item.val; // already created, maybe null
 			if (defind >= 0) item.definingIndex = defind;
-			lock (item) // no problem is not parallel
+			if (!EnterCreation(item)) return null; // cyclic reference, the threads would wait for each other
+			bool creating = false;
+			try
 			{
+				if (!(item.val is List<Item>)) return item.val; // already created, maybe null
+				if (item.creatingThread == Environment.CurrentManagedThreadId)
+				{   // cyclic reference: this item is already being created further up on this thread, creating it again
+					// would recurse until the stack overflows (#487)
+					ReportCyclicReference(item);
+					return null;
+				}
+				item.creatingThread = Environment.CurrentManagedThreadId;
+				creating = true;
 #if DEBUG
 				lock (definitionStack) definitionStack.Push(item.definingIndex);
 #endif
-				if (!(item.val is List<Item>)) return item.val; // already created, maybe null
 				if (item.type == Item.ItemType.advancedFace || item.type == Item.ItemType.faceSurface || item.type == Item.ItemType.curveBoundedSurface) CreatingFace();
 				switch (item.type)
 				{
@@ -4631,11 +4646,60 @@ private object CreateEntity(Item item)
 							break;
 						}
 				}
+				return item.val; // has now been created
+			}
+			finally
+			{
+				if (creating)
+				{
+					item.creatingThread = 0;
 #if DEBUG
-				lock (definitionStack) definitionStack.Pop();
+					lock (definitionStack) definitionStack.Pop();
 #endif
-				return item.val; // has now been created
-			} // end lock
+				}
+				Monitor.Exit(item);
+			}
+		}
+
+		/// <summary>
+		/// Acquires the lock of <paramref name="item"/> for <see cref="CreateEntity"/>. Returns false, without the lock, when
+		/// the thread that holds it waits, directly or through other threads, for an item this thread is creating: in a
+		/// reference cycle that was entered from different ends by the parallel import, the threads would wait for each
+		/// other forever (#487).
+		/// </summary>
+		private bool EnterCreation(Item item)
+		{
+			if (Monitor.TryEnter(item)) return true;
+			int me = Environment.CurrentManagedThreadId;
+			waitingFor[me] = item;
+			try
+			{
+				while (!Monitor.TryEnter(item, 10))
+				{
+					Item waitedFor = item;
+					for (int i = 0; i <= waitingFor.Count; i++)
+					{
+						int owner = waitedFor.creatingThread;
+						if (owner == 0) break; // not known yet or already done, try again
+						if (owner == me)
+						{
+							ReportCyclicReference(item);
+							return false;
+						}
+						if (!waitingFor.TryGetValue(owner, out waitedFor)) break; // the owner is working, not waiting
+					}
+				}
+				return true;
+			}
+			finally
+			{
+				waitingFor.TryRemove(me, out _);
+			}
+		}
+
+		private void ReportCyclicReference(Item item)
+		{
+			lock (importProblems) importProblems[item.definingIndex] = "cyclic reference";
 		}
 
 		private void AvoidDuplicateColorNames(ColorDef cd)

CADability/Make3D.cs

@@ -2128,17 +2128,25 @@ public static Shell[] SewFaces(Face[] faces, bool edgesArUnambiguous = false)
         }
         private static void extractConnectedFaces(HashSet<Face> allFaces, Face startWith, HashSet<Face> result)
         {
+            // depth first search with an explicit stack: the recursion was as deep as the number of connected faces
+            // and overflowed the stack for meshes of a few thousand triangles (#486). The faces are visited in the
+            // same order as by the recursion.
             result.Add(startWith);
             allFaces.Remove(startWith);
-            foreach (Edge edge in startWith.Edges)
-            {
-                if (allFaces.Contains(edge.PrimaryFace))
-                {
-                    extractConnectedFaces(allFaces, edge.PrimaryFace, result);
-                }
-                if (allFaces.Contains(edge.SecondaryFace))
-                {
-                    extractConnectedFaces(allFaces, edge.SecondaryFace, result);
+            Stack<(Edge[] edges, int next)> stack = new Stack<(Edge[] edges, int next)>();
+            stack.Push((startWith.Edges.ToArray(), 0));
+            while (stack.Count > 0)
+            {
+                (Edge[] edges, int next) = stack.Pop();
+                if (next >= 2 * edges.Length) continue;
+                stack.Push((edges, next + 1));
+                Edge edge = edges[next / 2];
+                Face face = (next % 2 == 0) ? edge.PrimaryFace : edge.SecondaryFace;
+                if (allFaces.Contains(face))
+                {
+                    result.Add(face);
+                    allFaces.Remove(face);
+                    stack.Push((face.Edges.ToArray(), 0));
                 }
             }
         }

tests/CADability.ImportTests/StepCyclicReferenceTest.cs

@@ -0,0 +1,113 @@
+using System;
+using System.IO;
+using System.Linq;
+using System.Threading.Tasks;
+using Microsoft.VisualStudio.TestTools.UnitTesting;
+using CADability;
+using CADability.GeoObject;
+
+namespace CADability.ImportTests
+{
+    /// <summary>
+    /// Regression for issue #487: ImportStep.CreateEntity resolves references recursively. A reference cycle, here a
+    /// MAPPED_ITEM whose REPRESENTATION_MAP maps the representation that contains the mapped item, made it create the same
+    /// entities again and again until the stack overflowed, which terminates the process. With the parallel import two
+    /// threads that enter a cycle from different ends would instead wait for each other forever. The cyclic reference is
+    /// now dropped and the rest of the file is imported.
+    /// </summary>
+    [TestClass]
+    public class StepCyclicReferenceTest
+    {
+        public TestContext TestContext { get; set; }
+
+        private const string SourceFile = "issue101.stp";
+        // issue101.stp contains a single solid with 25 faces, #12 is the representation that holds it
+        private const string BrepRepresentation = "#12=ADVANCED_BREP_SHAPE_REPRESENTATION('',(#14),#871);";
+
+        /// <summary>the representation of the solid also contains a mapped item that maps this representation</summary>
+        private const string SelfMappingRepresentation =
+            "#12=ADVANCED_BREP_SHAPE_REPRESENTATION('',(#14,#9001),#871);\n" +
+            "#9001=MAPPED_ITEM('',#9002,#523);\n" +
+            "#9002=REPRESENTATION_MAP(#523,#12);";
+
+        /// <summary>
+        /// two shape definition representations, whose representations map each other: the import of their roots enters
+        /// the cycle from both ends
+        /// </summary>
+        private const string MutuallyMappingRepresentations =
+            BrepRepresentation + "\n" +
+            "#9010=SHAPE_DEFINITION_REPRESENTATION(#878,#9011);\n" +
+            "#9011=SHAPE_REPRESENTATION('a',(#9012),#871);\n" +
+            "#9012=MAPPED_ITEM('',#9013,#523);\n" +
+            "#9013=REPRESENTATION_MAP(#523,#9021);\n" +
+            "#9020=SHAPE_DEFINITION_REPRESENTATION(#878,#9021);\n" +
+            "#9021=SHAPE_REPRESENTATION('b',(#9022),#871);\n" +
+            "#9022=MAPPED_ITEM('',#9023,#523);\n" +
+            "#9023=REPRESENTATION_MAP(#523,#9011);";
+
+        private string WriteVariant(string replacement)
+        {
+            string source = System.IO.Path.Combine(TestContext.DeploymentDirectory, TestContext.TestName, SourceFile);
+            Assert.IsTrue(File.Exists(source));
+            string text = File.ReadAllText(source).Replace("\r\n", "\n");
+            Assert.IsTrue(text.Contains(BrepRepresentation), "the fixture should contain the representation of the solid");
+            string file = System.IO.Path.Combine(TestContext.DeploymentDirectory, TestContext.TestName, "cyclic.stp");
+            File.WriteAllText(file, text.Replace(BrepRepresentation, replacement));
+            return file;
+        }
+
+        private static int ImportFaceCount(string file, bool parallel)
+        {
+            object old = Settings.GlobalSettings.GetValue("StepImport.Parallel");
+            Settings.GlobalSettings.SetValue("StepImport.Parallel", parallel);
+            try
+            {
+                // a cycle must neither overflow the stack nor make the threads of the parallel import wait for each other
+                Task<int> import = Task.Factory.StartNew(() =>
+                {
+                    Project project = Project.ReadFromFile(file, "stp");
+                    return project.GetActiveModel().AllObjects.Sum(CountFaces);
+                }, TaskCreationOptions.LongRunning);
+                Assert.IsTrue(import.Wait(TimeSpan.FromMinutes(2)), "the import did not finish");
+                return import.Result;
+            }
+            finally
+            {
+                if (old != null) Settings.GlobalSettings.SetValue("StepImport.Parallel", old);
+            }
+        }
+
+        [TestMethod]
+        [DeploymentItem(@"Files/Step/" + SourceFile, nameof(import_step_with_a_representation_that_maps_itself))]
+        public void import_step_with_a_representation_that_maps_itself()
+        {
+            string file = WriteVariant(SelfMappingRepresentation);
+            // the solid, and once more as the mapped item, where the cycle is cut
+            Assert.AreEqual(50, ImportFaceCount(file, false), "sequential import");
+            Assert.AreEqual(50, ImportFaceCount(file, true), "parallel import");
+        }
+
+        [TestMethod]
+        [DeploymentItem(@"Files/Step/" + SourceFile, nameof(import_step_with_representations_that_map_each_other))]
+        public void import_step_with_representations_that_map_each_other()
+        {
+            string file = WriteVariant(MutuallyMappingRepresentations);
+            // the solid, once for each of the three shape definition representations of its product
+            Assert.AreEqual(75, ImportFaceCount(file, false), "sequential import");
+            // whether the threads really enter the cycle at the same time depends on the timing, so try several times
+            for (int i = 0; i < 20; i++) Assert.AreEqual(75, ImportFaceCount(file, true), "parallel import");
+        }
+
+        private static int CountFaces(IGeoObject go)
+        {
+            switch (go)
+            {
+                case Solid solid: return solid.Shells.Sum(shell => shell.Faces.Length);
+                case Shell shell: return shell.Faces.Length;
+                case Face _: return 1;
+                case Block block: return Enumerable.Range(0, block.NumChildren).Sum(i => CountFaces(block.Child(i)));
+                default: return 0;
+            }
+        }
+    }
+}