Don't delete BgpTable till all walkers are done

Add infra to keep track of number of walkers per DBTableBase and invoke
the RetryDelete method when the last walker is done. Modify BgpTable to
use the new infra.

Change-Id: Id33c84c7fac76ab99fdf919045da280b0b5038b4
Closes-Bug: 1468052

src/bgp/bgp_table.cc

@@ -414,26 +414,32 @@ void BgpTable::Input(DBTablePartition *root, DBClient *client,
 bool BgpTable::MayDelete() const {
     CHECK_CONCURRENCY("bgp::Config");
 
-    //
-    // The route replicator may be still in the processes of cleaning up
-    // the table.
+    // Bail if the table has listeners.
     if (HasListeners()) {
         BGP_LOG_TABLE(this, SandeshLevel::SYS_DEBUG, BGP_LOG_FLAG_ALL,
                       "Paused table deletion due to pending listeners");
         return false;
     }
 
-    //
-    // This table cannot be deleted yet if any route is still present.
-    //
+    // Bail if the table has walkers.
+    if (HasWalkers()) {
+        BGP_LOG_TABLE(this, SandeshLevel::SYS_DEBUG, BGP_LOG_FLAG_ALL,
+                      "Paused table deletion due to pending walkers");
+        return false;
+    }
+
+    // Bail if the table is not empty.
     size_t size = Size();
     if (size > 0) {
         BGP_LOG_TABLE(this, SandeshLevel::SYS_DEBUG, BGP_LOG_FLAG_ALL,
                       "Paused table deletion due to " << size <<
-                      " pending entries");
+                      " pending routes");
         return false;
     }
-    return true;
+
+    // Check the base class at the end so that we add custom checks
+    // before this if needed and to get more informative log message.
+    return DBTableBase::MayDelete();
 }
 
 void BgpTable::Shutdown() {

src/bgp/bgp_table.h

@@ -97,7 +97,7 @@ class BgpTable : public RouteTable {
     void ManagedDelete();
     virtual void RetryDelete();
     void Shutdown();
-    bool MayDelete() const;
+    virtual bool MayDelete() const;
     bool IsDeleted() const { return deleter()->IsDeleted(); }
 
     RoutingInstance *routing_instance() { return rtinstance_; }

src/bgp/test/bgp_xmpp_deferq_test.cc

@@ -408,10 +408,58 @@ class BgpXmppSerializeMembershipReqTest : public BgpXmppUnitTest {
     }
 };
 
+static bool DummyWalkFunction(DBTablePartBase *root, DBEntryBase *entry) {
+    return true;
+}
+
+static void DummyWalkDoneFunction(DBTableBase *table) {
+}
+
 int BgpXmppUnitTest::validate_done_;
 
 namespace {
 
+TEST_F(BgpXmppUnitTest, TableDeleteWithPendingWalk) {
+    Configure();
+    task_util::WaitForIdle();
+
+    // create an XMPP client in server A
+    agent_a_.reset(
+        new test::NetworkAgentMock(&evm_, SUB_ADDR, xs_a_->GetPort()));
+
+    // Pause deletion for blue inet table.
+    BgpTable *blue_table = VerifyBgpTable("blue", Address::INET);
+    PauseDelete(blue_table->deleter());
+
+    // Unconfigure all instances.
+    // The blue instance and blue inet table should exist in deleted state.
+    UnconfigureRoutingInstances();
+    task_util::WaitForIdle();
+    RoutingInstance *blue = VerifyRoutingInstance("blue");
+    TASK_UTIL_EXPECT_TRUE(blue->deleted());
+    TASK_UTIL_EXPECT_TRUE(blue_table->IsDeleted());
+
+    // Stop the scheduler and resume delete of the table.
+    // This should create the bgp::Config Task to process lifetime manager
+    // work queue.
+    TaskScheduler::GetInstance()->Stop();
+    blue_table->deleter()->ResumeDelete();
+
+    // Start a bunch of table walks.
+    DBTableWalker *walker = a_->database()->GetWalker();
+    for (int idx = 0; idx < 128; ++idx) {
+        walker->WalkTable(
+            blue_table, NULL, DummyWalkFunction, DummyWalkDoneFunction);
+    }
+
+    // Start the scheduler.
+    // Table should get deleted only after all the walks are done.
+    TaskScheduler::GetInstance()->Start();
+
+    // The blue instance should have been destroyed.
+    VerifyNoRoutingInstance("blue");
+}
+
 TEST_F(BgpXmppUnitTest, Connection) {
     Configure();
     task_util::WaitForIdle();

src/db/db_table.cc

@@ -158,7 +158,8 @@ class DBTableBase::ListenerInfo {
 
 DBTableBase::DBTableBase(DB *db, const string &name)
         : db_(db), name_(name), info_(new ListenerInfo(name)),
-          enqueue_count_(0), input_count_(0), notify_count_(0) {
+          enqueue_count_(0), input_count_(0), notify_count_(0),
+          walker_count_(0) {
     walk_request_count_ = 0;
     walk_complete_count_ = 0;
     walk_cancel_count_ = 0;
@@ -202,6 +203,20 @@ void DBTableBase::AddToDBStateCount(ListenerId listener, int count) {
     info_->AddToDBStateCount(listener, count);
 }
 
+bool DBTableBase::MayDelete() const {
+    if (HasListeners()) {
+        return false;
+    }
+    if (HasWalkers()) {
+        return false;
+    }
+    if (!empty()) {
+        return false;
+    }
+
+    return true;
+}
+
 bool DBTableBase::HasListeners() const {
     return !info_->empty();
 }

src/db/db_table.h

@@ -83,7 +83,7 @@ class DBTableBase {
 
     void RunNotify(DBTablePartBase *tpart, DBEntryBase *entry);
 
-    // manage db state count for a listner
+    // Manage db state count for a listener.
     void AddToDBStateCount(ListenerId listener, int count);
 
     // Calculate the size across all partitions.
@@ -93,6 +93,7 @@ class DBTableBase {
 
     // Suspended deletion resume hook for user function
     virtual void RetryDelete() { }
+    virtual bool MayDelete() const;
 
     DB *database() { return db_; }
     const DB *database() const { return db_; }
@@ -115,6 +116,10 @@ class DBTableBase {
     void incr_notify_count() { notify_count_++; }
     void reset_notify_count() { notify_count_ = 0; }
 
+    bool HasWalkers() const { return walker_count_ != 0; }
+    void incr_walker_count() { walker_count_++; }
+    uint64_t decr_walker_count() { return --walker_count_; }
+
     uint64_t walk_request_count() const { return walk_request_count_; }
     uint64_t walk_complete_count() const { return walk_complete_count_; }
     uint64_t walk_cancel_count() const { return walk_cancel_count_; }
@@ -130,6 +135,7 @@ class DBTableBase {
     uint64_t enqueue_count_;
     uint64_t input_count_;
     uint64_t notify_count_;
+    uint64_t walker_count_;
     tbb::atomic<uint64_t> walk_request_count_;
     tbb::atomic<uint64_t> walk_complete_count_;
     tbb::atomic<uint64_t> walk_cancel_count_;

src/db/db_table_walker.cc

@@ -202,6 +202,7 @@ DBTableWalker::WalkId DBTableWalker::WalkTable(DBTable *table,
                                     walkerfn, walk_complete);
         walkers_[i] = walker;
     }
+    table->incr_walker_count();
     return i;
 }
 
@@ -214,6 +215,7 @@ void DBTableWalker::WalkCancel(WalkId id) {
 void DBTableWalker::PurgeWalker(WalkId id) {
     tbb::mutex::scoped_lock lock(walkers_mutex_);
     Walker *walker = walkers_[id];
+    DBTable *table = walker->table_;
     delete walker;
     walkers_[id] = NULL;
     if ((size_t) id == walkers_.size() - 1) {
@@ -229,4 +231,9 @@ void DBTableWalker::PurgeWalker(WalkId id) {
         }
         walker_map_.set(id);
     }
+
+    // Retry table deletion when the last walker is purged.
+    if (table->decr_walker_count() == 0) {
+        table->RetryDelete();
+    }
 }