From 47148e811e86ddd21881d7422c750d52219fd71d Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 17 Oct 2026 08:33:18 +0000 Subject: [PATCH 1/3] Collect connected faces without recursion (#486) Make3D.extractConnectedFaces, BRepOperation.extractConnectedFaces and BRepSelfIntersection.collectFaces walked the faces of a shell with a recursive depth first search that nested once per connected face. A DXF mesh of about 7000 triangles (sewing) or a boolean operation on a shell of some ten thousand faces overflowed the stack, which terminates the process. The three traversals use an explicit stack now and visit the faces in the same order as the recursion, so the shells come out unchanged. The new tests sew a grid of 3200 triangles and subtract a box from a prism with 3000 sides on a thread with a 256 KB stack; both crash the test host with a stack overflow without the change. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_012cNtmJQqQvHArBc17v66Xu --- CADability/BRepIntersection.cs | 40 ++++++-- CADability/Make3D.cs | 26 +++-- .../DeepFaceTraversalTests.cs | 94 +++++++++++++++++++ 3 files changed, 141 insertions(+), 19 deletions(-) create mode 100644 tests/CADability.Tests/DeepFaceTraversalTests.cs diff --git a/CADability/BRepIntersection.cs b/CADability/BRepIntersection.cs index 52853b5a..5b3585ee 100644 --- a/CADability/BRepIntersection.cs +++ b/CADability/BRepIntersection.cs @@ -9633,18 +9633,26 @@ internal static Dictionary> SortCycles(List> cycles, F /// the faces connected to , including it; they are removed from internal static HashSet extractConnectedFaces(HashSet 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 result = new HashSet(); 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 unusedfaces, HashSet 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)); + } } } diff --git a/CADability/Make3D.cs b/CADability/Make3D.cs index af8701d3..7cf67247 100644 --- a/CADability/Make3D.cs +++ b/CADability/Make3D.cs @@ -2128,17 +2128,25 @@ public static Shell[] SewFaces(Face[] faces, bool edgesArUnambiguous = false) } private static void extractConnectedFaces(HashSet allFaces, Face startWith, HashSet 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)); } } } diff --git a/tests/CADability.Tests/DeepFaceTraversalTests.cs b/tests/CADability.Tests/DeepFaceTraversalTests.cs new file mode 100644 index 00000000..881de916 --- /dev/null +++ b/tests/CADability.Tests/DeepFaceTraversalTests.cs @@ -0,0 +1,94 @@ +using System; +using System.Collections.Generic; +using System.Linq; +using System.Threading; +using CADability.GeoObject; + +namespace CADability.Tests +{ + /// + /// Regression for issue #486: the faces of a shell were collected by a recursive depth first search + /// (Make3D.extractConnectedFaces, BRepOperation.extractConnectedFaces, BRepSelfIntersection.collectFaces), which nests + /// once per connected face. A few thousand faces overflowed the stack, and a StackOverflowException terminates the + /// process. The tests run on a thread with a small stack, so that the old recursion fails with far fewer faces than it + /// does on a 1 MB stack. + /// + [TestClass] + public class DeepFaceTraversalTests + { + private const int SmallStack = 256 * 1024; + + private static void RunOnSmallStack(Action action) + { + Exception failure = null; + Thread thread = new Thread(() => + { + try { action(); } + catch (Exception e) { failure = e; } + }, SmallStack); + thread.Start(); + Assert.IsTrue(thread.Join(TimeSpan.FromMinutes(5)), "the operation did not finish"); + if (failure != null) throw new AssertFailedException(failure.ToString()); + } + + /// a flat grid of n x n squares, each split into two triangles + private static Face[] TriangleGrid(int n) + { + List faces = new List(); + for (int i = 0; i < n; i++) + { + for (int j = 0; j < n; j++) + { + GeoPoint p00 = new GeoPoint(i, j, 0), p10 = new GeoPoint(i + 1, j, 0); + GeoPoint p01 = new GeoPoint(i, j + 1, 0), p11 = new GeoPoint(i + 1, j + 1, 0); + faces.Add(Face.MakeFace(p00, p10, p11)); + faces.Add(Face.MakeFace(p00, p11, p01)); + } + } + return faces.ToArray(); + } + + [TestMethod] + public void sewing_thousands_of_triangles_does_not_overflow_the_stack() + { + Face[] triangles = TriangleGrid(40); + Shell[] shells = null; + RunOnSmallStack(() => shells = Make3D.SewFaces(triangles)); + Assert.AreEqual(1, shells.Length); + Assert.AreEqual(triangles.Length, shells[0].Faces.Length); + } + + [TestMethod] + public void sewing_keeps_separate_parts_apart() + { + Face[] first = TriangleGrid(3); + Face[] second = TriangleGrid(2); + ModOp move = ModOp.Translate(0, 0, 10); + foreach (Face face in second) face.Modify(move); + Shell[] shells = Make3D.SewFaces(first.Concat(second).ToArray()); + CollectionAssert.AreEquivalent(new[] { first.Length, second.Length }, shells.Select(s => s.Faces.Length).ToArray()); + } + + [TestMethod] + public void subtracting_from_a_prism_with_thousands_of_sides_does_not_overflow_the_stack() + { + const int sides = 3000; + GeoPoint[] points = new GeoPoint[sides]; + for (int i = 0; i < sides; i++) + { + double a = 2.0 * Math.PI * i / sides; + points[i] = new GeoPoint(100 * Math.Cos(a), 100 * Math.Sin(a), 0); + } + Solid prism = Make3D.MakePrism(Polyline.FromPoints(points, true), 10 * GeoVector.ZAxis, null, false) as Solid; + Assert.IsNotNull(prism); + Assert.AreEqual(sides + 2, prism.Shells[0].Faces.Length); + // a box that cuts a notch into the top face, away from the sides + Solid box = Make3D.MakeBox(new GeoPoint(-10, -10, 5), 20 * GeoVector.XAxis, 20 * GeoVector.YAxis, 10 * GeoVector.ZAxis); + Solid[] result = null; + RunOnSmallStack(() => result = Solid.Subtract(prism, box)); + Assert.AreEqual(1, result.Length); + // all sides, the bottom, the top with a hole and the five faces of the notch + Assert.AreEqual(sides + 2 + 5, result[0].Shells[0].Faces.Length); + } + } +} From a15be14421407b64bc788478756b6d1e4618213c Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 17 Oct 2026 08:33:18 +0000 Subject: [PATCH 2/3] Cut reference cycles in the STEP import instead of recursing forever (#487) ImportStep.CreateEntity creates referenced entities recursively and only knew an entity as created once it was finished. A reference cycle, e.g. a MAPPED_ITEM whose REPRESENTATION_MAP maps the representation that contains the mapped item, therefore re-entered CreateEntity for the same entity (the monitor is re-entrant) until the stack overflowed. - Each item records the thread that is creating it. When that thread reaches the item again, the reference closes a cycle: it is created as null and reported as "cyclic reference" in the import problems. - With StepImport.Parallel two threads can enter a cycle from different ends and would then wait for each other's lock forever. A thread that has to wait for an item now follows the chain of threads waiting for each other; when it leads back to an item it is creating itself, it cuts the cycle in the same way instead of waiting. - The DEBUG-only definition stack is no longer left unbalanced by the early return of an entity that another thread has created meanwhile. The new tests import issue101.stp with a representation that maps itself and with two representations that map each other, sequentially and in parallel. Without the change the first crashes the test host with a stack overflow; with only the same-thread check the parallel import of the second does not finish. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_012cNtmJQqQvHArBc17v66Xu --- CADability/ImportStep.cs | 74 +++++++++++- .../StepCyclicReferenceTest.cs | 113 ++++++++++++++++++ 2 files changed, 182 insertions(+), 5 deletions(-) create mode 100644 tests/CADability.ImportTests/StepCyclicReferenceTest.cs diff --git a/CADability/ImportStep.cs b/CADability/ImportStep.cs index 87e4fb4d..955480b2 100644 --- a/CADability/ImportStep.cs +++ b/CADability/ImportStep.cs @@ -414,6 +414,7 @@ public class ImportStep HashSet products = new HashSet(); HashSet mappedItems = new HashSet(); private Dictionary importProblems; + private System.Collections.Concurrent.ConcurrentDictionary waitingFor = new System.Collections.Concurrent.ConcurrentDictionary(); // thread id -> item the thread waits for in CreateEntity private List notImportedFaces; private Dictionary definedColors; private System.Collections.Concurrent.ConcurrentDictionary namedGeoObjects = new System.Collections.Concurrent.ConcurrentDictionary(); @@ -694,6 +695,10 @@ public enum ItemType public object val; public Dictionary parameter; public int definingIndex; + /// + /// Managed thread id of the thread that is creating this item in , 0 when none + /// + public volatile int creatingThread; #if DEBUG public List usedBy; public void SetUsedBy(Item item) @@ -2322,12 +2327,22 @@ private object CreateEntity(Item item) #endif if (!(item.val is List)) 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)) 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)) 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); + } + } + + /// + /// Acquires the lock of for . 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). + /// + 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) diff --git a/tests/CADability.ImportTests/StepCyclicReferenceTest.cs b/tests/CADability.ImportTests/StepCyclicReferenceTest.cs new file mode 100644 index 00000000..3dc3e872 --- /dev/null +++ b/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 +{ + /// + /// 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. + /// + [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);"; + + /// the representation of the solid also contains a mapped item that maps this representation + private const string SelfMappingRepresentation = + "#12=ADVANCED_BREP_SHAPE_REPRESENTATION('',(#14,#9001),#871);\n" + + "#9001=MAPPED_ITEM('',#9002,#523);\n" + + "#9002=REPRESENTATION_MAP(#523,#12);"; + + /// + /// two shape definition representations, whose representations map each other: the import of their roots enters + /// the cycle from both ends + /// + 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 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; + } + } + } +} From 04c7f74eda6576a096b4a93b7d850c34c887e093 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 17 Oct 2026 08:36:43 +0000 Subject: [PATCH 3/3] Set the version to 1.7.0 A minor release: 1.7.0 adds public API (IJsonProgress, DXF.IDxfProgress and the progress overloads, Text.ClearFontCache, ProjectedModel.InvalidateDisplayLists, the PaintTo3DOutOfMemory constructors). Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_012cNtmJQqQvHArBc17v66Xu --- CADability/CADability.csproj | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CADability/CADability.csproj b/CADability/CADability.csproj index bfdf456a..c36b555c 100644 --- a/CADability/CADability.csproj +++ b/CADability/CADability.csproj @@ -3,7 +3,7 @@ netstandard2.0 CADability - 1.6.0 + 1.7.0 FriendsOfCADability CAD functionality for .NET