MB-48925 1/3: Don't extend VBucket lifetime via bg Tasks

During bucket shutdown we intermittently see an exception thrown
during task scheduling on a background NonIO thread, which crashes the
memcached process.

+Analysis+

Bug is as follows. Starting at the main thread which is deleting the Bucket (Thread 1):

    (gdb) bt
    ...
    #10 0x0000000000649bf3 in FollyExecutorPool::schedule(std::shared_ptr<GlobalTask>) () at /c++/10.2.0/new:175
    #11 0x000000000084271b in EPVBucket::scheduleDeferredDeletion(EventuallyPersistentEngine&) () at /c++/10.2.0/ext/atomicity.h:100
    #12 0x00000000006dfe7a in VBucket::DeferredDeleter::operator()(VBucket*) const () at kv_engine/engines/ep/src/vbucket.cc:3990
    #13 0x000000000086f874 in std::_Sp_counted_deleter<EPVBucket*, VBucket::DeferredDeleter, ...>::_M_dispose () at /c++/10.2.0/bits/shared_ptr_base.h:453
    ...
    #18 std::shared_ptr<VBucket>::~shared_ptr (this=0x7b44000515d0, __in_chrg=<optimized out>) at /c++/10.2.0/bits/shared_ptr.h:121
    #19 PagingVisitor::~PagingVisitor (this=0x7b4400051540, __in_chrg=<optimized out>) at kv_engine/engines/ep/src/paging_visitor.h:39
    ...
    #31 std::__shared_ptr<GlobalTask, (__gnu_cxx::_Lock_policy)2>::reset () at /c++/10.2.0/bits/shared_ptr_base.h:1301
    #32 EventuallyPersistentEngine::waitForTasks(std::vector<std::shared_ptr<GlobalTask>, std::allocator<std::shared_ptr<GlobalTask> > >&) () at kv_engine/engines/ep/src/ep_engine.cc:6752
    #33 0x000000000082396f in EventuallyPersistentEngine::destroyInner(bool) () at kv_engine/engines/ep/src/ep_engine.cc:2135

1. PagingVisitor is still in existence running after
   `EventuallyPersistentEngine::destroyInner` - see frame #19. This is
   because all tasks belonging to bucket were returned from
   unregisterTaskable() just before.

2. PagingVisitor (via VBCBAdaptor) is destroyed, it decrements the
   refcount on the shared_ptr<VBucket> it owns - see frame #18.

3. That is the last reference to the VBucket, which results in
   VBucket::DeferredDeleter being invoked which in turn schedules a
   task to delete the VBucket (disk and memory) in the background -
   see frame #11.

We see the schedule's lambda happen on the SchedulerPool0 thread (T:35):

    Thread 35 "SchedulerPool0" hit Catchpoint 1 (exception thrown), __cxxabiv1::__cxa_throw (..., tinfo=0x10c4ec8 <typeinfo for std::out_of_range@@GLIBCXX_3.4>, ...) at /tmp/deploy/objdir/../gcc-10.2.0/libstdc++-v3/libsupc++/eh_throw.cc:80
    (gdb) bt
    #1  0x00007ffff4cad7d2 in std::__throw_out_of_range (__s=__s@entry=0xcc68e6 "_Map_base::at") at /tmp/deploy/objdir/../gcc-10.2.0/libstdc++-v3/src/c++11/functexcept.cc:82
    ...
    #3  0x00000000005504ee in std::unordered_map<...>::at (__k=@0x7fffe83a8f88: 0x7b7400000848, this=0x7b1000005580) at /c++/10.2.0/bits/unordered_map.h:1000
    #4  FollyExecutorPool::State::scheduleTask (this=..., executor=..., pool=..., task=...) at kv_engine/executor/folly_executorpool.cc:415
    ...
    #8  folly::EventBase::runInEventBaseThreadAndWait(...) at folly/io/async/EventBase.cpp:671
    ...

In FollyExecutorPool::State::scheduleTask (frame #3) we attempt to
lookup the Taskable (Bucket) in the ExecutorPool's map, however given
its already been unregistered, the taskable is not found an the
std::out_of_range exception is thrown.

This is a lifetime issue. We have VBucket objects potentially being
kept alive longer than their expected lifetime by virtue of background
tasks having shared ownership of them - and those background tasks
outlive the lifetime of their parent object (KVBucket), and crucially
past when the owning Bucket is unregistered with the ExecutorPool and
can no longer schedule tasks.

When it then _does+ attempt to schedule a task against an unregistered
(and deleted) Taskable; we see the crash.

+Solution+

There's arguably two problems which should be addressed (although
technically only one of the two is required to encounter this crash):

1. Background tasks owning VBuckets when they are not executing.
2. Background tasks outliving their associated Taskable (aka Bucket).

This patch addresses the critical issue of (1) - we remove the
(shared) ownership of VBucket from the background tasks which
previoulsy had it - both PagingVisitor which is the problematic class
in this scenario, but also in the other background Tasks which
potentially have the same problem.

The 2nd patch will tighten up the API for visiting VBuckets, so
visitors are not passed a VBucketPtr, but instead VBucket& which
reduces the chance of similar problems happening in future.

The 3rd patch will adddress Background Taks outliving their Taskable.

Change-Id: I340a3e4dc3d9234c4a34866b410fb8295a1c98d1
Reviewed-on: http://review.couchbase.org/c/kv_engine/+/163783
Tested-by: Dave Rigby <[email protected]>
Reviewed-by: Richard de Mellow <[email protected]>
Reviewed-by: James H <[email protected]>

engines/ep/src/paging_visitor.cc

@@ -182,10 +182,11 @@ void PagingVisitor::visitBucket(const VBucketPtr& vb) {
     // fast path for expiry item pager
     if (owner == EXPIRY_PAGER) {
         if (vBucketFilter(vb->getId())) {
-            currentBucket = vb;
+            currentBucket = vb.get();
             // EvictionPolicy is not required when running expiry item
             // pager
             vb->ht.visit(*this);
+            currentBucket = nullptr;
         }
         return;
     }
@@ -204,8 +205,7 @@ void PagingVisitor::visitBucket(const VBucketPtr& vb) {
         return;
     }
 
-    currentBucket = vb;
-    maxCas = currentBucket->getMaxCas();
+    maxCas = vb->getMaxCas();
     itemEviction.reset();
     freqCounterThreshold = 0;
 
@@ -220,17 +220,18 @@ void PagingVisitor::visitBucket(const VBucketPtr& vb) {
                                 : ItemEviction::learningPopulation;
     itemEviction.setUpdateInterval(interval);
 
+    currentBucket = vb.get();
     vb->ht.visit(*this);
+    currentBucket = nullptr;
     /**
      * Note: We are not taking a reader lock on the vbucket state.
      * Therefore it is possible that the stats could be slightly
      * out.  However given that its just for stats we don't want
      * to incur any performance cost associated with taking the
      * lock.
      */
-    const bool isActiveOrPending =
-            ((currentBucket->getState() == vbucket_state_active) ||
-             (currentBucket->getState() == vbucket_state_pending));
+    const bool isActiveOrPending = ((vb->getState() == vbucket_state_active) ||
+                                    (vb->getState() == vbucket_state_pending));
 
     // Take a snapshot of the latest frequency histogram
     if (isActiveOrPending) {

engines/ep/src/paging_visitor.h

@@ -122,7 +122,8 @@ class PagingVisitor : public CappedDurationVBucketVisitor,
     size_t ejected;
 
     // The current vbucket that the eviction algorithm is operating on.
-    VBucketPtr currentBucket;
+    // Only valid while inside visitBucket().
+    VBucket* currentBucket{nullptr};
 
     // The frequency counter threshold that is used to determine whether we
     // should evict items from the hash table.

engines/ep/src/warmup.cc

@@ -1092,8 +1092,9 @@ void LoadStorageKVPairCallback::purge() {
 
         void visitBucket(const VBucketPtr& vb) override {
             if (vBucketFilter(vb->getId())) {
-                currentBucket = vb;
+                currentBucket = vb.get();
                 vb->ht.visit(*this);
+                currentBucket = nullptr;
             }
         }
 
@@ -1107,7 +1108,9 @@ void LoadStorageKVPairCallback::purge() {
 
     private:
         EPBucket& epstore;
-        VBucketPtr currentBucket;
+        // The current vbucket that the visitor is operating on. Only valid
+        // while inside visitBucket().
+        VBucket* currentBucket{nullptr};
     };
 
     auto vbucketIds(vbuckets.getBuckets());

engines/ep/tests/mock/mock_paging_visitor.h

@@ -60,7 +60,7 @@ class MockPagingVisitor : public PagingVisitor {
     }
 
     void setCurrentBucket(VBucketPtr _currentBucket) {
-        currentBucket = _currentBucket;
+        currentBucket = _currentBucket.get();
     }
 
     MOCK_METHOD1(visitBucket, void(const VBucketPtr&));

engines/ep/tests/module_tests/evp_engine_test.cc

@@ -22,11 +22,13 @@
 #include "programs/engine_testapp/mock_cookie.h"
 #include "programs/engine_testapp/mock_server.h"
 #include "tests/module_tests/test_helpers.h"
+#include "vb_visitors.h"
 #include <executor/cb3_taskqueue.h>
 
 #include <boost/algorithm/string/join.hpp>
 #include <boost/filesystem.hpp>
 #include <configuration_impl.h>
+#include <folly/synchronization/Baton.h>
 #include <platform/dirutils.h>
 #include <chrono>
 #include <thread>
@@ -108,10 +110,16 @@ void EventuallyPersistentEngineTest::TearDown() {
 }
 
 void EventuallyPersistentEngineTest::shutdownEngine() {
-    destroy_mock_cookie(cookie);
-    // Need to force the destroy (i.e. pass true) because
-    // NonIO threads may have been disabled (see DCPTest subclass).
-    engine->destroy(true);
+    if (cookie) {
+        destroy_mock_cookie(cookie);
+        cookie = nullptr;
+    }
+    if (engine) {
+        // Need to force the destroy (i.e. pass true) because
+        // NonIO threads may have been disabled (see DCPTest subclass).
+        engine->destroy(true);
+        engine = nullptr;
+    }
 }
 
 queued_item EventuallyPersistentEngineTest::store_item(
@@ -495,3 +503,82 @@ INSTANTIATE_TEST_SUITE_P(EphemeralOrPersistent,
                          [](const ::testing::TestParamInfo<std::string>& info) {
                              return info.param;
                          });
+
+/**
+ * Regression test for MB-48925 - if a Task is scheduled against a Taskable
+ * (Bucket) which has already been unregistered, then the ExecutorPool throws
+ * and crashes the process.
+ * Note: This test as it stands will *not* crash kv-engine if the fix for the
+ * issue (see rest of this commit) is reverted. This is because the fix is
+ * to change the currentVb Task member variable from an (owning)
+ * shared_ptr<VBucket> to a (non-owning) VBucket* - the same thing TestVisior
+ * below does. However it is included here for reference as to the original
+ * problematic scenario.
+ */
+TEST_F(EventuallyPersistentEngineTest, MB48925_ScheduleTaskAfterUnregistered) {
+    class TestVisitor : public InterruptableVBucketVisitor {
+    public:
+        TestVisitor(int& visitCount,
+                    folly::Baton<>& waitForVisit,
+                    folly::Baton<>& waitForDeinitialise)
+            : visitCount(visitCount),
+              waitForVisit(waitForVisit),
+              waitForDeinitialise(waitForDeinitialise) {
+        }
+
+        void visitBucket(const VBucketPtr& vb) override {
+            if (visitCount++ == 0) {
+                currentVb = vb.get();
+                // On first call to visitBucket() perform the necessary
+                // interleaved baton wait / sleeping.
+                // Suspend execution of this thread; and allow main thread to
+                // continue, delete Bucket and unregisterTaskable.
+                waitForVisit.post();
+
+                // Keep task running until unregisterTaskable() has been called
+                // and starts to cancel tasks - this ensures that the Task
+                // object is still alive (ExecutorPool has a reference to it)
+                // and hence is passed out from unregisterTaskable(), hence kept
+                // alive past when KVBucket is deleted.
+                waitForDeinitialise.wait();
+            }
+        }
+        InterruptableVBucketVisitor::ExecutionState shouldInterrupt() override {
+            return ExecutionState::Continue;
+        }
+
+        int& visitCount;
+        folly::Baton<>& waitForVisit;
+        folly::Baton<>& waitForDeinitialise;
+
+        // Model the behaviour of PagingVisitor prior to the bugfix. Note that
+        // _if_ this is changed to a shared_ptr<VBucket> then we crash.
+        VBucket* currentVb;
+    };
+
+    int visitCount{0};
+    folly::Baton waitForVisit;
+    folly::Baton waitForUnregister;
+    engine->getKVBucket()->visitAsync(
+            std::make_unique<TestVisitor>(
+                    visitCount, waitForVisit, waitForUnregister),
+            "MB48925_ScheduleTaskAfterUnregistered",
+            TaskId::ExpiredItemPagerVisitor,
+            std::chrono::seconds{1});
+    waitForVisit.wait();
+
+    // Setup testing hook so we allow our TestVisitor's Task above to
+    // continue once we are inside unregisterTaskable.
+    ExecutorPool::get()->unregisterTaskablePostCancelHook =
+            [&waitForUnregister]() { waitForUnregister.post(); };
+
+    // Delete the vbucket; so the file deletion will be performed by
+    // VBucket::DeferredDeleter when the last reference goes out of scope
+    // (expected to be the ExpiryPager.
+    engine->getKVBucket()->deleteVBucket(vbid);
+
+    // Destroy the engine. This does happen implicitly in TearDown, but call
+    // it earlier because we need to call destroy() before our various Baton
+    // local variables etc go out of scope.
+    shutdownEngine();
+}

executor/executorpool.h

@@ -11,6 +11,7 @@
 
 #include <memcached/engine_common.h>
 #include <memcached/thread_pool_config.h>
+#include <utilities/testing_hook.h>
 
 #include "task_type.h"
 #include <atomic>
@@ -222,6 +223,12 @@ class ExecutorPool {
      */
     static int getThreadPriority(task_type_t taskType);
 
+    /************** Testing *************************************************/
+
+    // Testing hook for MB-48925 - called inside unregisterTaskable after
+    // tasks have been cancelled.
+    TestingHook<> unregisterTaskablePostCancelHook;
+
 protected:
     ExecutorPool(size_t maxThreads);
 

executor/folly_executorpool.cc

@@ -879,6 +879,8 @@ std::vector<ExTask> FollyExecutorPool::unregisterTaskable(Taskable& taskable,
                 removedTasks = state->cancelTasksOwnedBy(taskable, force);
             });
 
+    unregisterTaskablePostCancelHook();
+
     // Step 2 - poll for taskOwners to become empty. This will only
     // occur once all outstanding, running tasks have been cancelled.
     auto isTaskOwnersEmpty = [eventBase, &state = this->state, &taskable] {