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

@@ -405,26 +405,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

@@ -405,10 +405,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

@@ -102,7 +102,7 @@ class DBTableBase::ListenerInfo {
 };
 
 DBTableBase::DBTableBase(DB *db, const string &name)
-        : db_(db), name_(name), info_(new ListenerInfo()) {
+        : db_(db), name_(name), info_(new ListenerInfo()), walker_count_(0) {
 }
 
 DBTableBase::~DBTableBase() {
@@ -136,6 +136,20 @@ void DBTableBase::RunNotify(DBTablePartBase *tpart, DBEntryBase *entry) {
     info_->RunNotify(tpart, entry);
 }
 
+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

@@ -80,9 +80,11 @@ class DBTableBase {
 
     // Calculate the size across all partitions.
     virtual size_t Size() const { return 0; }
+    bool empty() const { return (Size() == 0); }
 
     // Suspended deletion resume hook for user function
     virtual void RetryDelete() { }
+    virtual bool MayDelete() const;
 
     DB *database() { return db_; }
     const DB *database() const { return db_; }
@@ -91,11 +93,16 @@ class DBTableBase {
 
     bool HasListeners() const;
 
+    bool HasWalkers() const { return walker_count_ != 0; }
+    void incr_walker_count() { walker_count_++; }
+    uint64_t decr_walker_count() { return --walker_count_; }
+
 private:
     class ListenerInfo;
     DB *db_;
     std::string name_;
     std::auto_ptr<ListenerInfo> info_;
+    uint64_t walker_count_;
 };
 
 // An implementation of DBTableBase that uses boost::set as data-store

src/db/db_table_walker.cc

@@ -204,6 +204,7 @@ DBTableWalker::WalkId DBTableWalker::WalkTable(DBTable *table,
                                     walkerfn, walk_complete);
         walkers_[i] = walker;
     }
+    table->incr_walker_count();
     return i;
 }
 
@@ -217,6 +218,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) {
@@ -232,4 +234,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();
+    }
 }