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/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 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/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.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; + } + } + } +} 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); + } + } +}