Merge pull request #2 from wanmeihuali/hotfix/stack_overflow_better_solution

a better solution for ISUEE-1433

gtsam/inference/BayesTree.h

@@ -28,7 +28,6 @@
 
 #include <string>
 #include <queue>
-#include <deque>
 
 namespace gtsam {
 
@@ -114,21 +113,24 @@ namespace gtsam {
     BayesTree(const This& other);
 
     ~BayesTree() {
+      nodes_.clear();  // clear the map manually, as we cannot control the order of destruction in the map
       for (auto&& root: roots_) {
-        std::queue<Clique*> bfs_queue;
-        std::deque<Clique*> topological_order;
-        bfs_queue.push(root.get());
+        std::queue<sharedClique> bfs_queue;
+        
+        // first, move the root to the queue
+        bfs_queue.push(root);
+        root = nullptr;
+
+        // do a BFS on the tree, for each node, add its children to the queue, and then delete it from the queue
+        // So if the reference count of the node is 1, it will be deleted, and because its children are in the queue,
+        // the deletion of the node will not trigger a recursive deletion of the children.
         while (!bfs_queue.empty()) {
-          Clique* current = bfs_queue.front();
+          auto current = bfs_queue.front();
           bfs_queue.pop();
-          topological_order.push_front(current);
-          for (auto&& child: current->children) {
-            bfs_queue.push(child.get());
+          for (auto child: current->children) {
+            bfs_queue.push(child);
           }
         }
-        for (auto&& clique: topological_order) {
-          clique->children.clear();
-        }
       }
     }
 

gtsam/inference/BayesTreeCliqueBase.h

@@ -26,7 +26,6 @@
 #include <mutex>
 #include <optional>
 
-
 namespace gtsam {
 
   // Forward declarations

gtsam/inference/ClusterTree.h

@@ -9,7 +9,6 @@
 
 #pragma once
 
-#include <deque>
 #include <queue>
 #include <gtsam/base/Testable.h>
 #include <gtsam/base/FastVector.h>
@@ -48,8 +47,7 @@ class ClusterTree {
 
     Cluster() : problemSize_(0) {}
 
-    virtual ~Cluster() {
-    }
+    virtual ~Cluster() {}
 
     const Cluster& operator[](size_t i) const {
       return *(children[i]);
@@ -169,22 +167,20 @@ class ClusterTree {
 
   virtual ~ClusterTree() {
     // use default destructor which recursively deletes all nodes with shared_ptr causes stack overflow.
-    // so for each tree, we do a BFS to get sequence of nodes to delete, and clear their children first.
+    // so for each tree, we first move the root into a queue; then we do a BFS on the tree with the queue;
+    // for each node iterated, if its reference count is 1, it will be deleted while its children are still in the queue.
+    // so that the recursive deletion will not happen.
     for (auto&& root : roots_) {
-      std::queue<Cluster*> bfs_queue;
-      std::deque<Cluster*> topological_order;
-      bfs_queue.push(root.get());
+      std::queue<sharedNode> bfs_queue;
+      bfs_queue.push(root);
+      root = nullptr;
       while (!bfs_queue.empty()) {
         auto node = bfs_queue.front();
-        topological_order.push_front(node);
         bfs_queue.pop();
-        for (auto&& child : node->children) {
-          bfs_queue.push(child.get());
+        for (auto child : node->children) {
+          bfs_queue.push(child);
         }
       }
-      for (auto&& node : topological_order) {
-        node->children.clear();
-      }
     }
   }
 

gtsam/inference/EliminationTree.h

@@ -121,24 +121,21 @@ namespace gtsam {
 
   public:
     ~EliminationTree() {
-      // default destructor will cause stack overflow for deletion of large trees
-      // so we delete the tree explicitly by first BFSing the tree to determine the topological order
-      // and then clearing the children of each node in topological order
+      // use default destructor which recursively deletes all nodes with shared_ptr causes stack overflow.
+      // so for each tree, we first move the root into a queue; then we do a BFS on the tree with the queue;
+      // for each node iterated, if its reference count is 1, it will be deleted while its children are still in the queue.
+      // so that the recursive deletion will not happen.
       for (auto&& root : roots_) {
-        std::queue<Node*> bfs_queue;
-        std::deque<Node*> topological_order;
-        bfs_queue.push(root.get());
+        std::queue<sharedNode> bfs_queue;
+        bfs_queue.push(root);
+        root = nullptr;
         while (!bfs_queue.empty()) {
-          Node* node = bfs_queue.front();
+          auto node = bfs_queue.front();
           bfs_queue.pop();
-          topological_order.push_front(node);
           for (auto&& child : node->children) {
-            bfs_queue.push(child.get());
+            bfs_queue.push(child);
           }
         }
-        for (auto&& node : topological_order) {
-          node->children.clear();
-        }
       }
     }
     /// @name Standard Interface
@@ -179,7 +176,6 @@ namespace gtsam {
     /** Swap the data of this tree with another one, this operation is very fast. */
     void swap(This& other);
 
-
   protected:
     /// Protected default constructor
     EliminationTree() {}