Revert "MB-36578: [BP] Dereference cookie when creating DCP conn with same name"

This reverts commit 5b0a350.

After further investigation it was determined that the issue (MB-36451)
of not dereferencing the cookie of DCP connections can be a symptom of
MB-31495 and the code appears to be correct. As such, this change is
unnecessary.

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

Author: BenHuddleston

engines/ep/src/dcp/dcpconnmap.cc

@@ -61,60 +61,42 @@ DcpConnMap::~DcpConnMap() {
     LOG(EXTENSION_LOG_NOTICE, "Deleted dcpConnMap_");
 }
 
-std::shared_ptr<ConnHandler> DcpConnMap::checkForAndRemoveExistingConn(
-        LockHolder& lh,
-        const void* cookie,
-        const std::string& name,
-        const std::string& connType) {
-    /*
-     * If we request a connection of the same name then mark the existing
-     * connection as "want to disconnect" and erase it from the map. The caller
-     * will put it in deadConnections for us after it cancels associated
-     * background tasks as we do that outside of the connsLock.
-     */
-    auto oldConnection = findByName_UNLOCKED(lh, name);
-    if (oldConnection) {
-      LOG(EXTENSION_LOG_NOTICE,
-          "%s Disconnecting existing Dcp %s %s as it has the same "
-          "name as a new connection %p",
-          oldConnection->logHeader(), connType.c_str(), name.c_str(), cookie);
-        oldConnection->setDisconnect();
-        map_.erase(oldConnection->getCookie());
-    }
-
-    return oldConnection;
-}
-
-DcpConsumer* DcpConnMap::newConsumer(const void* cookie,
-                                     const std::string& name) {
-    DcpConsumer* result = nullptr;
-    std::shared_ptr<ConnHandler> oldConnection;
+DcpConsumer *DcpConnMap::newConsumer(const void* cookie,
+                                     const std::string &name)
+{
+    LockHolder lh(connsLock);
 
     std::string conn_name("eq_dcpq:");
     conn_name.append(name);
 
-    {
-        LockHolder lh(connsLock);
-        const auto& iter = map_.find(cookie);
-        if (iter != map_.end()) {
-            iter->second->setDisconnect();
-          LOG(EXTENSION_LOG_NOTICE,
-                  "Failed to create Dcp Consumer because connection "
-                  "(%p) already exists.", cookie);
-            return nullptr;
-        }
-
-        oldConnection = checkForAndRemoveExistingConn(
-                lh, cookie, conn_name, "Consumer");
-        auto consumer = std::make_shared<DcpConsumer>(engine, cookie, conn_name);
-
-        LOG(EXTENSION_LOG_INFO, "%s Connection created", consumer->logHeader());
+    const auto& iter = map_.find(cookie);
+    if (iter != map_.end()) {
+        iter->second->setDisconnect();
+        LOG(EXTENSION_LOG_NOTICE,
+            "Failed to create Dcp Consumer because connection "
+            "(%p) already exists.", cookie);
+        return nullptr;
+    }
 
-        result = consumer.get();
-        map_[cookie] = std::move(consumer);
+    /*
+     *  If we request a connection of the same name then
+     *  mark the existing connection as "want to disconnect".
+     */
+    for (const auto& cookieToConn : map_) {
+        if (cookieToConn.second->getName() == conn_name) {
+            LOG(EXTENSION_LOG_NOTICE,
+                "%s Disconnecting existing Dcp Consumer %p as it has the same "
+                "name as a new connection %p",
+                cookieToConn.second->logHeader(), cookieToConn.first, cookie);
+            cookieToConn.second->setDisconnect();
+        }
     }
 
-    disconnectConn(std::move(oldConnection));
+    std::shared_ptr<DcpConsumer> dc =
+            std::make_shared<DcpConsumer>(engine, cookie, conn_name);
+    auto* result = dc.get();
+    LOG(EXTENSION_LOG_INFO, "%s Connection created", dc->logHeader());
+    map_[cookie] = std::move(dc);
     return result;
 }
 
@@ -153,35 +135,43 @@ DcpProducer* DcpConnMap::newProducer(const void* cookie,
                                      const std::string& name,
                                      uint32_t flags,
                                      Collections::Filter filter) {
-    DcpProducer* result = nullptr;
-    std::shared_ptr<ConnHandler> oldConnection;
+    LockHolder lh(connsLock);
 
     std::string conn_name("eq_dcpq:");
     conn_name.append(name);
-    {
-        LockHolder lh(connsLock);
 
-        const auto& iter = map_.find(cookie);
-        if (iter != map_.end()) {
-          iter->second->setDisconnect();
-          LOG(EXTENSION_LOG_NOTICE,
-              "Failed to create Dcp Producer because connection "
-              "(%p) already exists.", cookie);
-          return nullptr;
-        }
-
-        oldConnection = checkForAndRemoveExistingConn(
-            lh, cookie, conn_name, "Producer");
-        auto producer = std::make_shared<DcpProducer>(
-            engine, cookie, conn_name, flags, std::move(filter), true /*startTask*/);
-
-        LOG(EXTENSION_LOG_INFO, "%s Connection created", producer->logHeader());
+    const auto& iter = map_.find(cookie);
+    if (iter != map_.end()) {
+        iter->second->setDisconnect();
+        LOG(EXTENSION_LOG_NOTICE,
+            "Failed to create Dcp Producer because connection "
+            "(%p) already exists.", cookie);
+        return nullptr;
+    }
 
-        result = producer.get();
-        map_[cookie] = std::move(producer);
+    /*
+     *  If we request a connection of the same name then
+     *  mark the existing connection as "want to disconnect".
+     */
+    for (const auto& cookieToConn : map_) {
+        if (cookieToConn.second->getName() == conn_name) {
+            LOG(EXTENSION_LOG_NOTICE,
+                "%s Disconnecting existing Dcp Producer %p as it has the same "
+                "name as a new connection %p",
+                cookieToConn.second->logHeader(), cookieToConn.first, cookie);
+            cookieToConn.second->setDisconnect();
+        }
     }
 
-    disconnectConn(std::move(oldConnection));
+    auto producer = std::make_shared<DcpProducer>(engine,
+                                                  cookie,
+                                                  conn_name,
+                                                  flags,
+                                                  std::move(filter),
+                                                  true /*startTask*/);
+    LOG(EXTENSION_LOG_INFO, "%s Connection created", producer->logHeader());
+    auto* result = producer.get();
+    map_[cookie] = std::move(producer);
 
     return result;
 }
@@ -313,10 +303,6 @@ void DcpConnMap::disconnect(const void *cookie) {
     // acquire PassiveStream::buffer.bufMutex; and that could deadlock
     // in EPBucket::setVBucketState, via
     // PassiveStream::processBufferedMessages.
-    disconnectConn(std::move(conn));
-}
-
-void DcpConnMap::disconnectConn(std::shared_ptr<ConnHandler>&& conn) {
     if (conn) {
         auto producer = std::dynamic_pointer_cast<DcpProducer>(conn);
         if (producer) {
@@ -519,11 +505,6 @@ void DcpConnMap::consumerBatchSizeConfigChanged(size_t newValue) {
 
 std::shared_ptr<ConnHandler> DcpConnMap::findByName(const std::string& name) {
     LockHolder lh(connsLock);
-    return findByName_UNLOCKED(lh, name);
-}
-
-std::shared_ptr<ConnHandler> DcpConnMap::findByName_UNLOCKED(
-        LockHolder& lh, const std::string& name) {
     for (const auto& cookieToConn : map_) {
         // If the connection is NOT about to be disconnected
         // and the names match

engines/ep/src/dcp/dcpconnmap.h

@@ -40,21 +40,6 @@ class DcpConnMap : public ConnMap {
 
     ~DcpConnMap();
 
-    /**
-     * Checks for a connection with the same name and sets it to disconnected
-     * whilst removing it from the map
-     *
-     * @param cookie Cookie representing the client
-     * @param name Full name of the connection
-     * @param connType Logging string (Producer/Consumer)
-     * @return shared_ptr to the connection
-     */
-    std::shared_ptr<ConnHandler> checkForAndRemoveExistingConn(
-            LockHolder& lh,
-            const void* cookie,
-            const std::string& name,
-            const std::string& connType);
-
     /**
      * Find or build a dcp connection for the given cookie and with
      * the given name.
@@ -116,18 +101,8 @@ class DcpConnMap : public ConnMap {
      */
     bool handleSlowStream(uint16_t vbid, const std::string &name);
 
-    /**
-     * Disconnect the connection for the given cookie
-     */
     void disconnect(const void *cookie);
 
-    /**
-     * ConnHandler specific disconnection.
-     *
-     * @param conn RValue conn to prevent any later use by caller
-     */
-    void disconnectConn(std::shared_ptr<ConnHandler>&& conn);
-
     void manageConnections();
 
     bool canAddBackfillToActiveQ();
@@ -185,9 +160,6 @@ class DcpConnMap : public ConnMap {
      */
     std::list<std::shared_ptr<ConnHandler>> deadConnections;
 
-    std::shared_ptr<ConnHandler> findByName_UNLOCKED(LockHolder& lh,
-                                                     const std::string& name);
-
     /*
      * Change the value at which a DcpConsumer::Processor task will yield
      */

engines/ep/tests/module_tests/dcp_test.cc

@@ -1879,16 +1879,6 @@ class ConnectionTest : public DCPTest,
      */
     void processConsumerMutationsNearThreshold(bool beyondThreshold);
 
-    /**
-     * Tests that when we open a DCP connection with the same name as an
-     * existing one the old one is correctly put into
-     * DcpConnMap::deadConnections and de-referenced by manageConnections
-     *
-     * @param DCP Open flags so we can test any type of connection
-     */
-    void testDCPConnectionWIthSameNameCorrectlyDereferencesOldOne(
-            uint32_t flags);
-
     /* vbucket associated with this connection */
     uint16_t vbid;
 };
@@ -2107,82 +2097,6 @@ TEST_P(ConnectionTest, test_deadConnections) {
         << "Dead connections still remain";
 }
 
-void ConnectionTest::testDCPConnectionWIthSameNameCorrectlyDereferencesOldOne(
-        uint32_t flags) {
-    auto& connMap = engine->getDcpConnMap();
-
-    // 1) Create a new DCP connection
-    const void* cookie1 = create_mock_cookie();
-
-    // Cookie ref count is initialized to 1 (like it would be for an actual
-    // server connection)
-    ASSERT_EQ(1, get_number_of_mock_cookie_references(cookie1));
-
-    // Need to hit the engine level function because it will do the cookie ref
-    // counting only if the DCP Open is successful
-    ASSERT_EQ(ENGINE_SUCCESS,
-              engine->dcpOpen(cookie1,
-                              0 /*opaque*/,
-                              0 /*seqno*/,
-                              flags /*flags*/,
-                              "test_conn",
-                              {} /*value*/));
-
-    // Should be able to find the connection with the given cookie
-    ASSERT_NE(nullptr, connMap.findByName("eq_dcpq:test_conn"));
-    EXPECT_EQ(2, get_number_of_mock_cookie_references(cookie1));
-
-    // 2) Create a new DCP connection with the same name
-    const void* cookie2 = create_mock_cookie();
-    ASSERT_EQ(1, get_number_of_mock_cookie_references(cookie2));
-    ASSERT_EQ(ENGINE_SUCCESS,
-              engine->dcpOpen(cookie2,
-                              0 /*opaque*/,
-                              0 /*seqno*/,
-                              flags,
-                              "test_conn",
-                              {} /*value*/));
-
-    // Should be able to find the connection with the given cookie. We will
-    // always return the new connections here as we skip disconnecting
-    // connections when we search by name (and we should have marked the
-    // original connection as disconnecting.
-    auto conn = connMap.findByName("eq_dcpq:test_conn");
-    ASSERT_TRUE(conn);
-    EXPECT_EQ(cookie2, conn->getCookie());
-
-    EXPECT_FALSE(connMap.isDeadConnectionsEmpty());
-    EXPECT_EQ(2, get_number_of_mock_cookie_references(cookie1));
-    EXPECT_EQ(2, get_number_of_mock_cookie_references(cookie2));
-
-    // Manage connections is called whenever we have some connection
-    // notification work to do. This is where we deal with deadConnections and
-    // de-reference the cookie.
-    connMap.manageConnections();
-    EXPECT_TRUE(connMap.isDeadConnectionsEmpty());
-    EXPECT_EQ(1, get_number_of_mock_cookie_references(cookie1));
-    EXPECT_EQ(2, get_number_of_mock_cookie_references(cookie2));
-
-    connMap.disconnect(cookie2);
-
-    destroy_mock_cookie(cookie1);
-    destroy_mock_cookie(cookie2);
-}
-
-TEST_P(ConnectionTest, NewDcpConsumerWithSameNameCorrectlyShutsdownOldOne) {
-    testDCPConnectionWIthSameNameCorrectlyDereferencesOldOne(0 /*no flags*/);
-}
-
-TEST_P(ConnectionTest, NewDcpProducerWithSameNameCorrectlyShutsdownOldOne) {
-    testDCPConnectionWIthSameNameCorrectlyDereferencesOldOne(
-        DCP_OPEN_PRODUCER);
-}
-
-TEST_P(ConnectionTest, NewDcpNotifierWithSameNameCorrectlyShutsdownOldOne) {
-    testDCPConnectionWIthSameNameCorrectlyDereferencesOldOne(
-        DCP_OPEN_NOTIFIER);
-}
-
 TEST_P(ConnectionTest, test_mb23637_findByNameWithConnectionDoDisconnect) {
     MockDcpConnMap connMap(*engine);
     connMap.initialize();