MB-23086: Always use SynchronousEPEngine::build

Ensure the various tests which use SynchronousEPEngine
use the factory build method to give better control
around the setup/teardown of this object.

Change-Id: Iedc019f3bae151746775af62ba1a9de7b645621c
Reviewed-on: http://review.couchbase.org/107946
Reviewed-by: Dave Rigby <[email protected]>
Tested-by: Build Bot <[email protected]>

Author: jimwwalker

engines/ep/benchmarks/engine_fixture.cc

@@ -33,14 +33,8 @@ void EngineFixture::SetUp(const benchmark::State& state) {
                 ExecutorPool::get());
         std::string config = "dbname=benchmarks-test;ht_locks=47;" + varConfig;
 
-        engine.reset(new SynchronousEPEngine(config));
+        engine = SynchronousEPEngine::build(config);
 
-        ObjectRegistry::onSwitchThread(engine.get());
-
-        engine->setKVBucket(
-                engine->public_makeBucket(engine->getConfiguration()));
-
-        engine->public_initializeEngineCallbacks();
         initialize_time_functions(get_mock_server_api()->core);
         cookie = create_mock_cookie();
     } else {

engines/ep/benchmarks/engine_fixture.h

@@ -20,10 +20,10 @@
 #include <benchmark/benchmark.h>
 
 #include "item.h"
+#include "tests/mock/mock_synchronous_ep_engine.h"
 
 class BenchmarkMemoryTracker;
 class SingleThreadedExecutorPool;
-class SynchronousEPEngine;
 
 /**
  * A fixture for benchmarking EpEngine and related classes.
@@ -36,7 +36,7 @@ class EngineFixture : public benchmark::Fixture {
 
     Item make_item(Vbid vbid, const std::string& key, const std::string& value);
 
-    std::unique_ptr<SynchronousEPEngine> engine;
+    SynchronousEPEngineUniquePtr engine;
     const void* cookie = nullptr;
     const Vbid vbid = Vbid(0);
 

engines/ep/tests/mock/mock_synchronous_ep_engine.cc

@@ -71,10 +71,9 @@ void SynchronousEPEngine::setDcpConnMap(
     dcpConnMap_ = std::move(dcpConnMap);
 }
 
-std::unique_ptr<SynchronousEPEngine> SynchronousEPEngine::build(
+SynchronousEPEngineUniquePtr SynchronousEPEngine::build(
         const std::string& config) {
-    std::unique_ptr<SynchronousEPEngine> engine(
-            new SynchronousEPEngine(config));
+    SynchronousEPEngineUniquePtr engine(new SynchronousEPEngine(config));
 
     // switch current thread to this new engine, so all sub-created objects
     // are accounted in it's mem_used.
@@ -90,6 +89,12 @@ std::unique_ptr<SynchronousEPEngine> SynchronousEPEngine::build(
     return engine;
 }
 
+void SynchronousEPEngineDeleter::operator()(SynchronousEPEngine* engine) {
+    ObjectRegistry::onSwitchThread(engine);
+    delete engine;
+    ObjectRegistry::onSwitchThread(nullptr);
+}
+
 void SynchronousEPEngine::initializeConnmap() {
     dcpConnMap_->initialize();
 }

engines/ep/tests/mock/mock_synchronous_ep_engine.h

@@ -24,6 +24,15 @@
 #include <ep_bucket.h>
 #include <ep_engine.h>
 
+class SynchronousEPEngine;
+
+struct SynchronousEPEngineDeleter {
+    void operator()(SynchronousEPEngine*);
+};
+
+using SynchronousEPEngineUniquePtr =
+        std::unique_ptr<SynchronousEPEngine, SynchronousEPEngineDeleter>;
+
 /* A class which subclasses the real EPEngine. Its main purpose is to allow
  * us to construct and setup an EPStore without starting all the various
  * background tasks which are normally started by EPEngine as part of creating
@@ -43,8 +52,7 @@ class SynchronousEPEngine : public EventuallyPersistentEngine {
 
     /// Constructs a SynchronousEPEngine instance, along with the necessary
     /// sub-components.
-    static std::unique_ptr<SynchronousEPEngine> build(
-            const std::string& config);
+    static SynchronousEPEngineUniquePtr build(const std::string& config);
 
     /* Allow us to call normally protected methods */
 

engines/ep/tests/module_tests/dcp_reflection_test.cc

@@ -182,7 +182,7 @@ class DCPLoopbackStreamTest : public SingleThreadedKVBucketTest {
 
     void takeoverTest(EnableExpiryOutput enableExpiryOutput);
 
-    std::unique_ptr<SynchronousEPEngine> replicaEngine;
+    SynchronousEPEngineUniquePtr replicaEngine;
     std::shared_ptr<MockDcpConsumer> consumer;
     // Non-owning ptr to consumer stream (owned by consumer).
     PassiveStream* consumerStream;

engines/ep/tests/module_tests/dcp_test.cc

@@ -1656,15 +1656,12 @@ TEST_P(ConnectionTest, test_mb21784) {
 class DcpConnMapTest : public ::testing::Test {
 protected:
     void SetUp() override {
-        /* Set up the bare minimum stuff needed by the 'SynchronousEPEngine'
-           (mock engine) */
-        ObjectRegistry::onSwitchThread(&engine);
-        engine.setKVBucket(engine.public_makeBucket(engine.getConfiguration()));
-        engine.public_initializeEngineCallbacks();
+        engine = SynchronousEPEngine::build({});
+
         initialize_time_functions(get_mock_server_api()->core);
 
         /* Set up one vbucket in the bucket */
-        engine.getKVBucket()->setVBucketState(vbid, vbucket_state_active);
+        engine->getKVBucket()->setVBucketState(vbid, vbucket_state_active);
     }
 
     void TearDown() override {
@@ -1682,7 +1679,7 @@ class DcpConnMapTest : public ::testing::Test {
         return ENGINE_SUCCESS;
     }
 
-    SynchronousEPEngine engine;
+    SynchronousEPEngineUniquePtr engine;
     const Vbid vbid = Vbid(0);
 };
 
@@ -1695,9 +1692,9 @@ class DcpConnMapTest : public ::testing::Test {
 TEST_F(DcpConnMapTest, DeleteProducerOnUncleanDCPConnMapDelete) {
     /* Create a new Dcp producer */
     const void* dummyMockCookie = create_mock_cookie();
-    DcpProducer* producer = engine.getDcpConnMap().newProducer(dummyMockCookie,
-                                                               "test_producer",
-                                                               /*flags*/ 0);
+    DcpProducer* producer = engine->getDcpConnMap().newProducer(dummyMockCookie,
+                                                                "test_producer",
+                                                                /*flags*/ 0);
     /* Open stream */
     uint64_t rollbackSeqno = 0;
     uint32_t opaque = 0;
@@ -1719,7 +1716,7 @@ TEST_F(DcpConnMapTest, DeleteProducerOnUncleanDCPConnMapDelete) {
     /* Delete the connmap, connection should be deleted as the owner of
        the connection (connmap) is deleted. Checks that there is no cyclic
        reference between conn (producer) and stream or any other object */
-    engine.setDcpConnMap(nullptr);
+    engine->setDcpConnMap(nullptr);
 }
 
 /* Tests that there is no memory loss due to cyclic reference between a
@@ -1728,7 +1725,7 @@ TEST_F(DcpConnMapTest, DeleteProducerOnUncleanDCPConnMapDelete) {
 TEST_F(DcpConnMapTest, DeleteNotifierConnOnUncleanDCPConnMapDelete) {
     /* Create a new Dcp producer */
     const void* dummyMockCookie = create_mock_cookie();
-    DcpProducer* producer = engine.getDcpConnMap().newProducer(
+    DcpProducer* producer = engine->getDcpConnMap().newProducer(
             dummyMockCookie,
             "test_producer",
             cb::mcbp::request::DcpOpenPayload::Notifier);
@@ -1753,20 +1750,20 @@ TEST_F(DcpConnMapTest, DeleteNotifierConnOnUncleanDCPConnMapDelete) {
     /* Delete the connmap, connection should be deleted as the owner of
        the connection (connmap) is deleted. Checks that there is no cyclic
        reference between conn (producer) and stream or any other object */
-    engine.setDcpConnMap(nullptr);
+    engine->setDcpConnMap(nullptr);
 }
 
 /* Tests that there is no memory loss due to cyclic reference between a
  * consumer connection and a passive stream.
  */
 TEST_F(DcpConnMapTest, DeleteConsumerConnOnUncleanDCPConnMapDelete) {
     /* Consumer stream needs a replica vbucket */
-    engine.getKVBucket()->setVBucketState(vbid, vbucket_state_replica);
+    engine->getKVBucket()->setVBucketState(vbid, vbucket_state_replica);
 
     /* Create a new Dcp consumer */
     const void* dummyMockCookie = create_mock_cookie();
-    DcpConsumer* consumer = engine.getDcpConnMap().newConsumer(dummyMockCookie,
-                                                               "test_consumer");
+    DcpConsumer* consumer = engine->getDcpConnMap().newConsumer(
+            dummyMockCookie, "test_consumer");
 
     /* Add passive stream */
     ASSERT_EQ(ENGINE_SUCCESS,
@@ -1779,7 +1776,7 @@ TEST_F(DcpConnMapTest, DeleteConsumerConnOnUncleanDCPConnMapDelete) {
     /* Delete the connmap, connection should be deleted as the owner of
        the connection (connmap) is deleted. Checks that there is no cyclic
        reference between conn (consumer) and stream or any other object */
-    engine.setDcpConnMap(nullptr);
+    engine->setDcpConnMap(nullptr);
 }
 
 class NotifyTest : public DCPTest {

engines/ep/tests/module_tests/kv_bucket_test.cc

@@ -30,6 +30,7 @@
 #include "dcp/flow-control-manager.h"
 #include "ep_engine.h"
 #include "ep_time.h"
+#include "fakes/fake_executorpool.h"
 #include "flusher.h"
 #include "globaltask.h"
 #include "lambda_task.h"
@@ -105,7 +106,6 @@ void KVBucketTest::destroy() {
     destroy_mock_cookie(cookie);
     destroy_mock_event_callbacks();
     engine->getDcpConnMap().manageConnections();
-    ObjectRegistry::onSwitchThread(nullptr);
     engine.reset();
 }
 

engines/ep/tests/module_tests/kv_bucket_test.h

@@ -27,14 +27,14 @@
 
 #include "callbacks.h"
 #include "item.h"
+#include <tests/mock/mock_synchronous_ep_engine.h>
 
 #include <folly/portability/GTest.h>
 #include <memcached/protocol_binary.h>
 
 #include <memory>
 
 class KVBucket;
-class SynchronousEPEngine;
 
 namespace Collections {
 class Manager;
@@ -239,7 +239,7 @@ class KVBucketTest : virtual public ::testing::Test {
     const Vbid vbid = Vbid(0);
 
     // The mock engine (needed to construct the store).
-    std::unique_ptr<SynchronousEPEngine> engine;
+    SynchronousEPEngineUniquePtr engine;
 
     // The store under test. Wrapped in a mock to expose some normally
     // protected members. Uses a raw pointer as this is owned by the engine.

engines/ep/tests/module_tests/objectregistry_test.cc

@@ -21,6 +21,7 @@
 
 #include "bucket_logger.h"
 #include "bucket_logger_test.h"
+#include "fakes/fake_executorpool.h"
 #include "item.h"
 #include "memory_tracker.h"
 #include "objectregistry.h"
@@ -34,39 +35,43 @@
 class ObjectRegistryTest : virtual public ::testing::Test {
 protected:
     void SetUp() override {
-        ObjectRegistry::onSwitchThread(&engine);
+        SingleThreadedExecutorPool::replaceExecutorPoolWithFake();
+        engine = SynchronousEPEngine::build({});
     }
+
     void TearDown() override {
-        ObjectRegistry::onSwitchThread(nullptr);
+        destroy_mock_event_callbacks();
+        engine.reset();
+        ExecutorPool::shutdown();
     }
 
-    SynchronousEPEngine engine;
+    SynchronousEPEngineUniquePtr engine;
 };
 
 // Check that constructing & destructing an Item is correctly tracked in
 // EpStats::numItem via ObjectRegistry::on{Create,Delete}Item.
 TEST_F(ObjectRegistryTest, NumItem) {
-    ASSERT_EQ(0, engine.getEpStats().getNumItem());
+    ASSERT_EQ(0, engine->getEpStats().getNumItem());
 
     {
         auto item = make_item(Vbid(0), makeStoredDocKey("key"), "value");
-        EXPECT_EQ(1, engine.getEpStats().getNumItem());
+        EXPECT_EQ(1, engine->getEpStats().getNumItem());
     }
-    EXPECT_EQ(0, engine.getEpStats().getNumItem());
+    EXPECT_EQ(0, engine->getEpStats().getNumItem());
 }
 
 // Check that constructing & destructing an Item is correctly tracked in
 // EpStats::memOverhead via ObjectRegistry::on{Create,Delete}Item.
 TEST_F(ObjectRegistryTest, MemOverhead) {
-    ASSERT_EQ(0, engine.getEpStats().getMemOverhead());
+    auto baseline = engine->getEpStats().getMemOverhead();
 
     {
         auto item = make_item(Vbid(0), makeStoredDocKey("key"), "value");
         // Currently just checking the overhead is non-zero; could expand
         // to calculate expected size based on the Item's size.
-        EXPECT_NE(0, engine.getEpStats().getMemOverhead());
+        EXPECT_NE(baseline, engine->getEpStats().getMemOverhead());
     }
-    EXPECT_EQ(0, engine.getEpStats().getMemOverhead());
+    EXPECT_EQ(baseline, engine->getEpStats().getMemOverhead());
 }
 
 /**
@@ -100,15 +105,16 @@ class ObjectRegistrySpdlogTest : public BucketLoggerTest,
 
         // Enable memory tracking hooks
         MemoryTracker::getInstance(*get_mock_server_api()->alloc_hooks);
-        engine.getEpStats().memoryTrackerEnabled.store(true);
+        engine->getEpStats().memoryTrackerEnabled.store(true);
     }
 
     void TearDown() override {
         MemoryTracker::destroyInstance();
 
         // Parent classes TearDown methods are sufficient here.
-        ObjectRegistryTest::TearDown();
         BucketLoggerTest::TearDown();
+        // called last so that the engine is destroyed last
+        ObjectRegistryTest::TearDown();
     }
 };
 
@@ -119,13 +125,13 @@ TEST_F(ObjectRegistrySpdlogTest, SpdlogMemoryTrackedCorrectly) {
             ::testing::UnitTest::GetInstance()->current_test_info()->name();
 
     // const char* - uses the single argument overload of warn().
-    auto baselineMemory = engine.getEpStats().getPreciseTotalMemoryUsed();
+    auto baselineMemory = engine->getEpStats().getPreciseTotalMemoryUsed();
     {
         auto logger = BucketLogger::createBucketLogger(testName);
         logger->log(spdlog::level::warn, "const char* message");
         logger->flush();
     }
-    EXPECT_EQ(baselineMemory, engine.getEpStats().getPreciseTotalMemoryUsed());
+    EXPECT_EQ(baselineMemory, engine->getEpStats().getPreciseTotalMemoryUsed());
 
     // multiple arguments using format string, with a short (< sizeof(sync_msg)
     // log string.
@@ -146,25 +152,25 @@ TEST_F(ObjectRegistrySpdlogTest, SpdlogMemoryTrackedCorrectly) {
                      std::string(asyncMsgCapacity, 's'));
         logger->flush();
     }
-    EXPECT_EQ(baselineMemory, engine.getEpStats().getPreciseTotalMemoryUsed());
+    EXPECT_EQ(baselineMemory, engine->getEpStats().getPreciseTotalMemoryUsed());
 
     // As previous, but looping with multiple warn() calls - check that we
     // correctly account even when multiple messages are created & destroyed.
     {
         auto logger = BucketLogger::createBucketLogger(testName);
         auto afterLoggerMemory =
-                engine.getEpStats().getPreciseTotalMemoryUsed();
+                engine->getEpStats().getPreciseTotalMemoryUsed();
 
         for (int i = 0; i < 100; i++) {
             logger->warn("short+variable loop ({}) {} ",
                          i,
                          std::string(asyncMsgCapacity, 's'));
             logger->flush();
             EXPECT_EQ(afterLoggerMemory,
-                      engine.getEpStats().getPreciseTotalMemoryUsed());
+                      engine->getEpStats().getPreciseTotalMemoryUsed());
         }
     }
-    EXPECT_EQ(baselineMemory, engine.getEpStats().getPreciseTotalMemoryUsed());
+    EXPECT_EQ(baselineMemory, engine->getEpStats().getPreciseTotalMemoryUsed());
 
     // Multiple arguments with a very long string (greater than
     // asyncMsgCapacity)
@@ -178,23 +184,23 @@ TEST_F(ObjectRegistrySpdlogTest, SpdlogMemoryTrackedCorrectly) {
                      std::string(asyncMsgCapacity * 2, 'x'));
         logger->flush();
     }
-    EXPECT_EQ(baselineMemory, engine.getEpStats().getPreciseTotalMemoryUsed());
+    EXPECT_EQ(baselineMemory, engine->getEpStats().getPreciseTotalMemoryUsed());
 
     // Multiple log messages; each with a log string - check that we correctly
     // account even when multiple messages are created & destroyed.
     {
         auto logger = BucketLogger::createBucketLogger(testName);
         auto afterLoggerMemory =
-                engine.getEpStats().getPreciseTotalMemoryUsed();
+                engine->getEpStats().getPreciseTotalMemoryUsed();
 
         for (int i = 0; i < 100; i++) {
             logger->warn("long+variable loop ({}) {}",
                          i,
                          std::string(asyncMsgCapacity * 2, 'x'));
             logger->flush();
             EXPECT_EQ(afterLoggerMemory,
-                      engine.getEpStats().getPreciseTotalMemoryUsed());
+                      engine->getEpStats().getPreciseTotalMemoryUsed());
         }
     }
-    EXPECT_EQ(baselineMemory, engine.getEpStats().getPreciseTotalMemoryUsed());
+    EXPECT_EQ(baselineMemory, engine->getEpStats().getPreciseTotalMemoryUsed());
 }