MB-20943: Set state to dead when deleting vbucket 73/67873/8 4.5.1 v4.5.1
authorDaniel Owen <owend@couchbase.com>
Wed, 21 Sep 2016 09:50:33 +0000 (10:50 +0100)
committerDave Rigby <daver@couchbase.com>
Wed, 21 Sep 2016 16:29:44 +0000 (16:29 +0000)
When executing the VBucketMemoryDeletionTask the vbucket state is
unchanged.  notifyAllPendingConnsFailed is called in the run
function of VBucketMemoryDeletionTask.  This inturn calls fireAllOps,
which ensures all pending ops are cleared if the vbucket is in an
active state.

However if the vbucket is in a pending state is does nothing and
therefore the pending operations remain.  This results in connections
not being closed down.

The solution provided is to set the vbucket state to dead in
deleteVBucket, prior to calling scheduleVBDeletion.

A corresponding test has been added, which without the fix will hang.

Change-Id: I09cd4597b26576dd4b99d26f3a60c031e1b5f41d
Reviewed-on: http://review.couchbase.org/67873
Well-Formed: buildbot <build@couchbase.com>
Tested-by: buildbot <build@couchbase.com>
Reviewed-by: Dave Rigby <daver@couchbase.com>
Reviewed-by: Jim Walker <jim@couchbase.com>
src/ep.cc
src/vbucket.cc
tests/ep_testsuite.cc

index ee11303..1f76432 100644 (file)
--- a/src/ep.cc
+++ b/src/ep.cc
@@ -1487,6 +1487,7 @@ ENGINE_ERROR_CODE EventuallyPersistentStore::deleteVBucket(uint16_t vbid,
         return ENGINE_NOT_MY_VBUCKET;
     }
 
+    vb->setState(vbucket_state_dead);
     engine.getDcpConnMap().vbucketStateChanged(vbid, vbucket_state_dead);
     vbMap.removeBucket(vbid);
     lh.unlock();
index 90811ba..8cde4cb 100644 (file)
@@ -21,6 +21,7 @@
 #include <list>
 #include <set>
 #include <string>
+#include <vector>
 
 #include "atomic.h"
 #include "bgfetcher.h"
@@ -153,6 +154,8 @@ VBucket::~VBucket() {
 
 void VBucket::fireAllOps(EventuallyPersistentEngine &engine,
                          ENGINE_ERROR_CODE code) {
+    LockHolder lh(pendingOpLock);
+
     if (pendingOpsStart > 0) {
         hrtime_t now = gethrtime();
         if (now > pendingOpsStart) {
@@ -168,8 +171,15 @@ void VBucket::fireAllOps(EventuallyPersistentEngine &engine,
     stats.pendingOps.fetch_sub(pendingOps.size());
     atomic_setIfBigger(stats.pendingOpsMax, pendingOps.size());
 
-    engine.notifyIOComplete(pendingOps, code);
-    pendingOps.clear();
+    while (!pendingOps.empty()) {
+        const void *pendingOperation = pendingOps.back();
+        pendingOps.pop_back();
+        // We don't want to hold the pendingOpLock when
+        // calling notifyIOComplete.
+        lh.unlock();
+        engine.notifyIOComplete(pendingOperation, code);
+        lh.lock();
+    }
 
     LOG(EXTENSION_LOG_INFO,
         "Fired pendings ops for vbucket %" PRIu16 " in state %s\n",
@@ -177,7 +187,6 @@ void VBucket::fireAllOps(EventuallyPersistentEngine &engine,
 }
 
 void VBucket::fireAllOps(EventuallyPersistentEngine &engine) {
-    LockHolder lh(pendingOpLock);
 
     if (state == vbucket_state_active) {
         fireAllOps(engine, ENGINE_SUCCESS);
index 429d38c..637f1d0 100644 (file)
 #include <stdlib.h>
 #include <string.h>
 
+#include <condition_variable>
 #include <cstdlib>
 #include <iostream>
 #include <iomanip>
 #include <map>
+#include <mutex>
 #include <set>
 #include <sstream>
 #include <string>
+#include <thread>
 #include <unordered_map>
 #include <vector>
 
@@ -6474,6 +6477,54 @@ static enum test_result test_mb20697(ENGINE_HANDLE *h,
     return SUCCESS;
 }
 
+static enum test_result test_mb20943_complete_pending_ops_on_vbucket_delete(
+        ENGINE_HANDLE *h, ENGINE_HANDLE_V1 *h1) {
+    const void *cookie = testHarness.create_cookie();
+    bool  ready = false;
+    std::mutex m;
+    std::condition_variable cv;
+    item *i = NULL;
+
+    check(set_vbucket_state(h, h1, 1, vbucket_state_pending),
+              "Failed to set vbucket state.");
+    testHarness.set_ewouldblock_handling(cookie, false);
+
+    checkeq(ENGINE_EWOULDBLOCK, h1->get(h, cookie, &i, "key", strlen("key"),
+                                        1), "Expected EWOULDBLOCK.");
+
+    // Create a thread that will wait for the cookie notify.
+    std::thread notify_waiter{[&cv, &ready, &m, &cookie](){
+        {
+            std::lock_guard<std::mutex> lk(m);
+            testHarness.lock_cookie(cookie);
+            ready = true;
+        }
+        // Once we have locked the cookie we can allow the main thread to
+        // continue.
+        cv.notify_one();
+        testHarness.waitfor_cookie(cookie);
+        testHarness.unlock_cookie(cookie);
+
+    }};
+
+    std::unique_lock<std::mutex> lk(m);
+    // Wait until spawned thread has locked the cookie.
+    cv.wait(lk, [&ready]{return ready;});
+    lk.unlock();
+    vbucketDelete(h, h1, 1);
+    // Wait for the thread to finish, which will occur when the thread has been
+    // notified.
+    notify_waiter.join();
+
+    // vbucket no longer exists and therefore should return not my vbucket.
+    checkeq(ENGINE_NOT_MY_VBUCKET,
+           h1->get(h, cookie, &i, "key", strlen("key"), 1),
+            "Expected NOT MY VBUCKET.");
+    h1->release(h, NULL, i);
+    testHarness.destroy_cookie(cookie);
+    return SUCCESS;
+}
+
 /* This test case checks the purge seqno validity when no items are actually
    purged in a compaction call */
 static enum test_result test_vbucket_compact_no_purge(ENGINE_HANDLE *h,
@@ -6978,5 +7029,8 @@ BaseTestCase testsuite_testcases[] = {
 
         TestCase("test_MB-20697", test_mb20697, test_setup, teardown, NULL, prepare,
                  cleanup),
+        TestCase("test_MB-test_mb20943_remove_pending_ops_on_vbucket_delete",
+                 test_mb20943_complete_pending_ops_on_vbucket_delete,
+                 test_setup, teardown, NULL, prepare, cleanup),
         TestCase(NULL, NULL, NULL, NULL, NULL, prepare, cleanup)
 };