MB-42688: Merge branch 'couchbase/alice' into mad-hatter

* commit '3b0344840':
  MB-42688: Do not check vbucket state on each comparator call

Change-Id: Ia7a9fbda15849d0d3af5d9a51af503da5e017c21

Author: jameseh96

engines/ep/src/paging_visitor.cc

@@ -334,27 +334,27 @@ void PagingVisitor::complete() {
 
 std::function<bool(const Vbid&, const Vbid&)>
 PagingVisitor::getVBucketComparator() const {
-    // Get the pageable mem used of each vb _once_ and cache it.
-    // Fetching these values repeatedly in the comparator could cause issues as
-    // the values can change _during_ a given sort call.
+    // Get the pageable mem used and state of each vb _once_ and cache it.
+    // Fetching these values repeatedly in the comparator could cause issues
+    // as the values can change _during_ a given sort call.
 
-    std::map<Vbid, size_t> pageableMemUsed;
+    std::map<Vbid, std::pair<bool, size_t>> stateAndPageableMemUsed;
 
     for (const auto& vbid : store.getVBuckets().getBuckets()) {
         auto vb = store.getVBucket(vbid);
-        pageableMemUsed[vbid] = vb ? vb->getPageableMemUsage() : 0;
+        if (vb) {
+            stateAndPageableMemUsed[vbid] = {
+                    vb->getState() == vbucket_state_replica,
+                    vb->getPageableMemUsage()};
+        }
     }
 
-    return [pageableMemUsed = std::move(pageableMemUsed), &store = store](
-                   const Vbid& a, const Vbid& b) mutable {
-        auto vbA = store.getVBucket(a);
-        auto vbB = store.getVBucket(b);
-        bool aReplica = vbA && vbA->getState() == vbucket_state_replica;
-        bool bReplica = vbB && vbB->getState() == vbucket_state_replica;
+    return [stateAndPageableMemUsed = std::move(stateAndPageableMemUsed)](
+                   const Vbid& a,
+                   const Vbid& b) mutable {
         // sort replicas before all other vbucket states, then sort by
         // pageableMemUsed
-        return std::make_pair(aReplica, pageableMemUsed[a]) >
-               std::make_pair(bReplica, pageableMemUsed[b]);
+        return stateAndPageableMemUsed[a] > stateAndPageableMemUsed[b];
     };
 }
 

engines/ep/tests/module_tests/item_pager_test.cc

@@ -1094,6 +1094,113 @@ TEST_P(STItemPagerTest, ItemPagerEvictionOrder) {
         ;
 }
 
+TEST_P(STItemPagerTest, ItemPagerEvictionOrderIsSafe) {
+    // MB-42688: Test that the ordering of the paging visitor comparator is
+    // fixed even if the amount of pageable memory or the vbucket state changes.
+    // This is required to meet the strict weak ordering requirement of
+    // std::sort
+
+    // 17 is the minimum number of vbs found to demonstrate a segfault
+    // with an unsuitable comparator on linux. This is likely impl dependent.
+    // 18 gives 6 vbs per active/replica/pending
+    resetEngineAndWarmup("max_vbuckets=18");
+    std::vector<Vbid> allVBs(18);
+
+    for (int i = 0; i < 18; i++) {
+        allVBs[i] = Vbid(i);
+    }
+
+
+    for (int i = 0; i < 6; i++) {
+        setVBucketStateAndRunPersistTask(Vbid(i), vbucket_state_active);
+        setVBucketStateAndRunPersistTask(Vbid(i + 6), vbucket_state_pending);
+        setVBucketStateAndRunPersistTask(Vbid(i + 12), vbucket_state_replica);
+    }
+
+    if (engine->getConfiguration().getBucketType() == "persistent") {
+        // flush all vbs
+        for (const auto& vbid : allVBs) {
+            dynamic_cast<EPBucket&>(*store).flushVBucket(vbid);
+        }
+    }
+
+    auto& stats = engine->getEpStats();
+    auto available = std::make_shared<std::atomic<bool>>();
+    auto& config = engine->getConfiguration();
+
+    auto pv = std::make_unique<MockPagingVisitor>(
+            *store,
+            stats,
+            EvictionRatios{1.0 /*active*/, 1.0 /*replica*/},
+            available,
+            ITEM_PAGER,
+            false,
+            VBucketFilter(allVBs),
+            config.getItemEvictionAgePercentage(),
+            config.getItemEvictionFreqCounterAgeThreshold());
+
+    // now test that even with state changes, the comparator sorts the vbuckets
+    // acceptably
+    auto innerComparator = pv->getVBucketComparator();
+
+    // wrap the comparator to allow insertion of state changes mid-sort
+    auto comparator = [&innerComparator, this](const Vbid& a, const Vbid& b) {
+      // run the actual comparator
+      auto res = innerComparator(a, b);
+      // now, to _intentionally_ try to break the strict weak ordering, change
+      // the state of one of the vbuckets. If the vbucket comparator is
+      // checking the state each time, this can cause a crash in std::sort
+      for (const auto& vbid : {a, b}) {
+          auto state = store->getVBucket(vbid)->getState();
+          EXPECT_EQ(ENGINE_SUCCESS,
+                    store->setVBucketState(vbid,
+                                           state == vbucket_state_replica
+                                           ? vbucket_state_active
+                                           : vbucket_state_replica));
+      }
+
+      return res;
+    };
+
+    // if the vbucket comparator is "safe" and checks the state once, this
+    // sort will work as expected - otherwise it _may_ segfault.
+    // Note: this is not a robust test, variations in the impl of sort
+    // may mean this does _not_ crash even with an unacceptable comparator
+    std::sort(allVBs.begin(), allVBs.end(), comparator);
+
+    // as a secondary check, directly test the requirements of a strict
+    // weak ordering on the comparator while actively changing
+    // the state of vbuckets. This will fail if the comparator checks
+    // the bucket state on every call.
+
+    // irreflexivity
+    for (const auto& x : allVBs) {
+        EXPECT_FALSE(comparator(x, x));
+    }
+
+    // asymmetry
+    for (const auto& x : allVBs) {
+        for (const auto& y : allVBs) {
+            EXPECT_FALSE(comparator(x, y) && comparator(y, x));
+        }
+    }
+
+    // transitivity
+    for (const auto& x : allVBs) {
+        for (const auto& y : allVBs) {
+            for (const auto& z : allVBs) {
+                // x < y && y < z => x < z
+                // equivalent to
+                // !(x < y && y < z) || x < z
+                // if x < y and y < z, it must be true that x < z
+                EXPECT_TRUE(!(comparator(x, y) && comparator(y, z)) ||
+                            comparator(x, z));
+            }
+        }
+    }
+}
+
+
 /**
  * Test fixture for Ephemeral-only item pager tests.
  */