MB-21109: Fix intermittent failure in 'temp item deletion'

Test suffers from intermittent failures due to it not accounting for
background fetch of deleted items. The issue is that the read of
curr_temp_items races with the background fetcher - if the background
fetcher has not completed then it will have a value of 1 (expected),
however if the BG fetcher completes before the stat is read then 0
will be returned (as the temp item has been cleaned up).

To address this, disable the reader threads before triggering the
background fetch. This requires that we manually handle the
EWOULDBLOCK that get_meta returns (otherwise we'll hang forever
waiting for the BG fetch task to run).

Change-Id: If1bca36a6517909259b4e3767fd58a7ff99944c3
Reviewed-on: http://review.couchbase.org/69237
Reviewed-by: Trond Norbye <trond.norbye@gmail.com>
Tested-by: buildbot <build@couchbase.com>

diff --git a/tests/ep_test_apis.cc b/tests/ep_test_apis.cc
index 41eab2d..07a02ee 100644 (file)
--- a/tests/ep_test_apis.cc
+++ b/tests/ep_test_apis.cc
@@ -599,7 +599,7 @@ void getl(ENGINE_HANDLE *h, ENGINE_HANDLE_V1 *h1, const char* key,
 }
 
 bool get_meta(ENGINE_HANDLE *h, ENGINE_HANDLE_V1 *h1, const char* key,
-              bool reqExtMeta) {
+              bool reqExtMeta, const void* cookie) {
 
     protocol_binary_request_header *req;
     if (reqExtMeta) {
@@ -611,9 +611,14 @@ bool get_meta(ENGINE_HANDLE *h, ENGINE_HANDLE_V1 *h1, const char* key,
                            NULL, 0, key, strlen(key));
     }
 
-    ENGINE_ERROR_CODE ret = h1->unknown_command(h, NULL, req,
+    ENGINE_ERROR_CODE ret = h1->unknown_command(h, cookie, req,
                                                 add_response_get_meta);
-    check(ret == ENGINE_SUCCESS, "Expected get_meta call to be successful");
+    if (ret == ENGINE_EWOULDBLOCK) {
+        last_status = static_cast<protocol_binary_response_status>(ENGINE_EWOULDBLOCK);
+    } else {
+        check(ret == ENGINE_SUCCESS,
+              "Expected get_meta call to be successful");
+    }
     cb_free(req);
     if (last_status == PROTOCOL_BINARY_RESPONSE_SUCCESS) {
         return true;

diff --git a/tests/ep_test_apis.h b/tests/ep_test_apis.h
index 983e083..7e64af3 100644 (file)
--- a/tests/ep_test_apis.h
+++ b/tests/ep_test_apis.h
@@ -365,7 +365,7 @@ void add_with_meta(ENGINE_HANDLE *h, ENGINE_HANDLE_V1 *h1, const char *key,
                    bool skipConflictResolution = false,
                    uint8_t datatype = 0x00, bool includeExtMeta = false);
 bool get_meta(ENGINE_HANDLE *h, ENGINE_HANDLE_V1 *h1, const char* key,
-              bool reqExtMeta = false);
+              bool reqExtMeta = false, const void* cookie = nullptr);
 void del_with_meta(ENGINE_HANDLE *h, ENGINE_HANDLE_V1 *h1, const char *key,
                    const size_t keylen, const uint32_t vb,
                    ItemMetaData *itemMeta, uint64_t cas_for_delete = 0,

diff --git a/tests/ep_testsuite_xdcr.cc b/tests/ep_testsuite_xdcr.cc
index f1dc39a..ef64673 100644 (file)
--- a/tests/ep_testsuite_xdcr.cc
+++ b/tests/ep_testsuite_xdcr.cc
@@ -1223,18 +1223,41 @@ static enum test_result test_temp_item_deletion(ENGINE_HANDLE *h, ENGINE_HANDLE_
     wait_for_flusher_to_settle(h, h1);
     wait_for_stat_to_be(h, h1, "curr_items", 0);
 
-    check(get_meta(h, h1, k1), "Expected to get meta");
-    checkeq(PROTOCOL_BINARY_RESPONSE_SUCCESS, last_status.load(), "Expected success");
-    check(last_deleted_flag, "Expected deleted flag to be set");
+    // Issue a get_meta for a deleted key. This will need to bring in a temp
+    // item into the hashtable as a placeholder for the (deleted) metadata
+    // which needs to be loaded from disk via BG fetch
+    // We need to temporarily disable the reader threads as to prevent the
+    // BGfetch from immediately running and removing our temp_item before
+    // we've had chance to validate its existence.
+    set_param(h, h1, protocol_binary_engine_param_flush,
+              "max_num_readers", "0");
+
+    // Tell the harness not to handle EWOULDBLOCK for us - we want it to
+    // be outstanding while we check the below stats.
+    const void *cookie = testHarness.create_cookie();
+    testHarness.set_ewouldblock_handling(cookie, false);
+
+    checkeq(false, get_meta(h, h1, k1, /*reqExtMeta*/false, cookie),
+            "Expected get_meta to fail (EWOULDBLOCK)");
+    checkeq(static_cast<protocol_binary_response_status>(ENGINE_EWOULDBLOCK),
+            last_status.load(), "Expected EWOULDBLOCK");
+
     checkeq(0, get_int_stat(h, h1, "curr_items"), "Expected zero curr_items");
     checkeq(1, get_int_stat(h, h1, "curr_temp_items"), "Expected single temp_items");
 
-    // Do get_meta for a non-existing key
+    // Re-enable EWOULDBLOCK handling (and reader threads), and re-issue.
+    testHarness.set_ewouldblock_handling(cookie, true);
+    set_param(h, h1, protocol_binary_engine_param_flush,
+              "max_num_readers", "1");
+
+    check(get_meta(h, h1, k1, /*reqExtMeta*/false, cookie),
+          "Expected get_meta to succeed");
+    check(last_deleted_flag, "Expected deleted flag to be set");
+
+    // Do get_meta for a non-existing key.
     char const *k2 = "k2";
     check(!get_meta(h, h1, k2), "Expected get meta to return false");
     checkeq(PROTOCOL_BINARY_RESPONSE_KEY_ENOENT, last_status.load(), "Expected enoent");
-    checkeq(1, get_int_stat(h, h1, "curr_temp_items"),
-            "No additional bg fetches, thanks to bloom filters");
 
     // Trigger the expiry pager and verify that two temp items are deleted
     wait_for_stat_to_be(h, h1, "ep_expired_pager", 1);
@@ -1242,6 +1265,8 @@ static enum test_result test_temp_item_deletion(ENGINE_HANDLE *h, ENGINE_HANDLE_
     checkeq(0, get_int_stat(h, h1, "curr_temp_items"), "Expected zero temp_items");
 
     h1->release(h, NULL, i);
+    testHarness.destroy_cookie(cookie);
+
     return SUCCESS;
 }