MB-31495: Fix bug in getRandomKey

This patch fixes a bug where getRandomKey would loop indefinitely if
start == 0.

The bug is that we check for curr == start before checking if
curr == max.  See the original code below.

start = 0;
curr = start;
VBucketPtr vb = getVBucket(Vbid(curr++));
while (!vb || vb->getState() != vbucket_state_active) {
    if (curr == start) {
        return GetValue(NULL, ENGINE_KEY_ENOENT);
    }
    if (curr == max) {
        curr = 0;
    }

    vb = getVBucket(Vbid(curr++));
}

Assume the bucket is empty. The first time we enter the while loop
curr == 1.  curr != start and curr != max and so we will call
getVBucket and increment curr to 2.  We repeat until curr == 1024.
On going round the while loop again curr != start, but curr == max
and so set curr = 0.

We then call getVbucket and increment curr to 1.  On going round the
while loop again curr != start (as it is 1), and hence we loop
indefinitely.

Change-Id: I0de040627c481fb53e0faeddd34f44f31055c241
Reviewed-on: http://review.couchbase.org/100280
Well-Formed: Build Bot <[email protected]>
Reviewed-by: Dave Rigby <[email protected]>
Reviewed-by: Trond Norbye <[email protected]>
Tested-by: Build Bot <[email protected]>

Author: owend74

engines/ep/src/kv_bucket.cc

@@ -1395,24 +1395,24 @@ GetValue KVBucket::getInternal(const DocKey& key,
 GetValue KVBucket::getRandomKey() {
     VBucketMap::id_type max = vbMap.getSize();
 
-    const long start = random() % max;
+    const long start = getRandom() % max;
     long curr = start;
     std::unique_ptr<Item> itm;
 
     while (itm == NULL) {
         VBucketPtr vb = getVBucket(curr++);
         while (!vb || vb->getState() != vbucket_state_active) {
-            if (curr == start) {
-                return GetValue(NULL, ENGINE_KEY_ENOENT);
-            }
             if (curr == max) {
                 curr = 0;
             }
+            if (curr == start) {
+                return GetValue(NULL, ENGINE_KEY_ENOENT);
+            }
 
             vb = getVBucket(curr++);
         }
 
-        if ((itm = vb->ht.getRandomKey(random()))) {
+        if ((itm = vb->ht.getRandomKey(getRandom()))) {
             GetValue rv(std::move(itm), ENGINE_SUCCESS);
             return rv;
         }

engines/ep/src/kv_bucket.h

@@ -176,6 +176,11 @@ class KVBucket : public KVBucketIface {
                            options);
     }
 
+    /**
+     * Retrieve a value randomly from the store.
+     *
+     * @return a GetValue representing the value retrieved
+     */
     GetValue getRandomKey(void);
 
     /**
@@ -1017,6 +1022,12 @@ class KVBucket : public KVBucketIface {
 
     std::atomic<size_t> maxTtl;
 
+    /**
+     * Allows us to override the random function.  This is used for testing
+     * purposes where we want a constant number as opposed to a random one.
+     */
+    std::function<long()> getRandom = random;
+
     friend class KVBucketTest;
 
     DISALLOW_COPY_AND_ASSIGN(KVBucket);

engines/ep/tests/module_tests/kv_bucket_test.cc

@@ -334,6 +334,9 @@ protocol_binary_response_status KVBucketTest::getAddResponseStatus(
 protocol_binary_response_status KVBucketTest::addResponseStatus =
         PROTOCOL_BINARY_RESPONSE_SUCCESS;
 
+void KVBucketTest::setRandomFunction(std::function<long()>& randFunction) {
+    store->getRandom = randFunction;
+}
 
 // getKeyStats tests //////////////////////////////////////////////////////////
 
@@ -1232,6 +1235,23 @@ TEST_P(KVBucketParamTest, AccessScannerInvalidLogLocation) {
                "not exist";
 }
 
+// Check that getRandomKey works correctly when given a random value of zero
+TEST_P(KVBucketParamTest, MB31495_GetRandomKey) {
+    std::function<long()> returnZero = []() { return 0; };
+    setRandomFunction(returnZero);
+
+    // Try with am empty hash table
+    auto gv = store->getRandomKey();
+    EXPECT_EQ(ENGINE_KEY_ENOENT, gv.getStatus());
+
+    Item item = store_item(
+                vbid, {"key", DocNamespace::DefaultCollection}, "value", 0);
+
+    // Try with a non-empty hash table
+    gv = store->getRandomKey();
+    EXPECT_EQ(ENGINE_SUCCESS, gv.getStatus());
+}
+
 class StoreIfTest : public KVBucketTest {
 public:
     void SetUp() override {

engines/ep/tests/module_tests/kv_bucket_test.h

@@ -209,6 +209,12 @@ class KVBucketTest : public ::testing::Test {
         completeWarmup = value;
     }
 
+    /**
+     * set the random function used by KVBucket.
+     * @param randFunction  The random function to be used by the KVBucket.
+     */
+    void setRandomFunction(std::function<long()>& randFunction);
+
 private:
     /**
      * Initialise test objects - e.g. engine/store/cookie