MB-32589: AddStatsStream: Correctly account allocated mem to NonBucket (take 2)

The previous fix for this issue (in the alice branch) failed to
correctly account for the memory freed by the std::string created from
'buf':

     auto* e = ObjectRegistry::onSwitchThread(nullptr, true);
>    auto value = buf.str(); // value is of type std::string
     callback(key.data(), key.size(), value.data(), value.size(), cookie);
     ObjectRegistry::onSwitchThread(e);

The manual onTheadSwitch() calls incorrectly assigned the deallocation
of std::string to the engine as the std::string dtor runs after
onThreadSwitch(e).

Fix (hopefully correctly this time!) by using a scoped guard
instead. Also includes a unit test.

Change-Id: Ic3eb7a1aa8a00012c99910bbaf4277a5d1548c80
Reviewed-on: http://review.couchbase.org/103843
Well-Formed: Build Bot <[email protected]>
Reviewed-by: Chris Farman <[email protected]>
Tested-by: Build Bot <[email protected]>

Author: daverigby

engines/ep/src/ep_engine.cc

@@ -3347,10 +3347,9 @@ class AddStatsStream : public std::ostream {
         // stat data) which will be de-allocated inside the server (i.e.
         // after the engine call has returned). As such we do not want to
         // account such memory against this bucket.
-        auto* e = ObjectRegistry::onSwitchThread(nullptr, true);
+        SystemAllocationGuard guard;
         auto value = buf.str();
         callback(key.data(), key.size(), value.data(), value.size(), cookie);
-        ObjectRegistry::onSwitchThread(e);
     }
 
 private:

engines/ep/tests/module_tests/stats_test.cc

@@ -24,6 +24,7 @@
 #include "dcp/producer.h"
 #include "dcp/stream.h"
 #include "evp_store_single_threaded_test.h"
+#include "memory_tracker.h"
 #include "tasks.h"
 #include "test_helpers.h"
 #include "thread_gate.h"
@@ -144,6 +145,59 @@ TEST_F(StatTest, vbucket_takeover_stats_stream_not_active) {
     producer->closeStream(/*opaque*/ 0, vbid);
 }
 
+// MB-32589: Check that _hash-dump stats correctly accounts temporary memory.
+TEST_F(StatTest, HashStatsMemUsed) {
+    // Enable memory tracking hooks
+    MemoryTracker::getInstance(*get_mock_server_api()->alloc_hooks);
+    engine->getEpStats().memoryTrackerEnabled.store(true);
+
+    ASSERT_TRUE(engine->getEpStats().memoryTrackerEnabled);
+
+    // Add some items to VBucket 0 so the stats call has some data to
+    // dump.
+    store_item(0, makeStoredDocKey("key1"), std::string(100, 'x'));
+    store_item(0, makeStoredDocKey("key2"), std::string(100, 'y'));
+
+    auto baselineMemory = engine->getEpStats().getPreciseTotalMemoryUsed();
+
+    // Perform the stats call. Should be associated with an engine at this
+    // point (as setup by EvpGetStats in the 'real' environment).
+    auto oldEngine = ObjectRegistry::getCurrentEngine();
+    ASSERT_TRUE(oldEngine);
+
+    cb::const_char_buffer key{"_hash-dump 0"};
+    struct Cookie : public cb::tracing::Traceable {
+        int addStats_calls = 0;
+    } state;
+
+    auto callback = [](const char* key,
+                       const uint16_t klen,
+                       const char* val,
+                       const uint32_t vlen,
+                       gsl::not_null<const void*> cookie) {
+        Cookie& state =
+                *reinterpret_cast<Cookie*>(const_cast<void*>(cookie.get()));
+        state.addStats_calls++;
+
+        // This callback should run in the memcache-context so no engine should
+        // be assigned to the current thread.
+        EXPECT_FALSE(ObjectRegistry::getCurrentEngine());
+    };
+
+    ASSERT_EQ(ENGINE_SUCCESS,
+              engine->getStats(&state, key.data(), key.size(), callback));
+
+    // Sanity check - should have had at least 1 call to ADD_STATS (otherwise
+    // the test isn't valid).
+    ASSERT_GT(state.addStats_calls, 0);
+
+    // Should still be associated with the same engine.
+    EXPECT_EQ(oldEngine, ObjectRegistry::getCurrentEngine());
+
+    // Any temporary memory should have been freed by now, and accounted
+    // correctly.
+    EXPECT_EQ(baselineMemory, engine->getEpStats().getPreciseTotalMemoryUsed());
+}
 
 TEST_P(DatatypeStatTest, datatypesInitiallyZero) {
     // Check that the datatype stats initialise to 0