SERVER-82367 Enqueued refreshes under the same inProgressLookup entry are always called with the same cached value in the ReadThroughCache - BACKPORT-24020 (#31615)

GitOrigin-RevId: 8193b1195989c5e90001b21f638d6441c843fefc

Author: Meryama03

src/mongo/util/read_through_cache.h

@@ -574,8 +574,14 @@ class ReadThroughCache : public ReadThroughCacheBase {
                                    !inProgressLookup.empty(ul));
         }();
 
-        if (!mustDoAnotherLoop)
+        if (!mustDoAnotherLoop) {
             _inProgressLookups.erase(it);
+        } else if (result.isOK()) {
+            // The fetched value can not satisfy all the enqueued refresh requests over the nss, but
+            // it can still be leveraged as a base version to perform the lookups that are still
+            // pending, optimizing the delta between the cached value and the remote one.
+            inProgressLookup.updateCachedValue(ul, ValueHandle(result.getValue()));
+        }
         ul.unlock();
 
         // The only reason this loop pops the values as it goes and std::moves into the last value
@@ -746,6 +752,10 @@ class ReadThroughCache<Key, Value, Time>::InProgressLookup {
             _cancelToken->tryCancel();
     }
 
+    void updateCachedValue(WithLock, ValueHandle cachedValue) {
+        _cachedValue = std::move(cachedValue);
+    }
+
 private:
     // The owning cache, from which mutex, lookupFn, async task scheduling, etc. will be used. It is
     // the responsibility of the owning cache to join all outstanding lookups at destruction time.

src/mongo/util/read_through_cache_test.cpp

@@ -27,8 +27,18 @@
  *    it in the license file.
  */
 
+#include <absl/container/node_hash_map.h>
+#include <boost/move/utility_core.hpp>
+#include <boost/none.hpp>
+#include <boost/optional/optional.hpp>
+#include <boost/smart_ptr.hpp>
+#include <cstddef>
+#include <fmt/format.h>
 #include <string>
 
+#include "mongo/base/string_data.h"
+#include "mongo/bson/timestamp.h"
+#include "mongo/db/client.h"
 #include "mongo/db/concurrency/locker_noop_service_context_test_fixture.h"
 #include "mongo/db/operation_context.h"
 #include "mongo/unittest/barrier.h"
@@ -803,5 +813,54 @@ TEST_F(ReadThroughCacheAsyncTest, CacheSizeZero) {
                                    }));
 }
 
+TEST_F(ReadThroughCacheAsyncTest, EnqueuedRequestLeveragesTheResultOfAnEarlierLookup) {
+    MockThreadPool threadPool;
+    boost::optional<CausallyConsistentCache::LookupResult> nextToReturn;
+    boost::optional<int> valueFetchedByPreviousRequest;
+
+    CausallyConsistentCache cache(getServiceContext(),
+                                  threadPool,
+                                  1,
+                                  [&](OperationContext*,
+                                      const std::string&,
+                                      const CausallyConsistentCache::ValueHandle& cachedValue,
+                                      const Timestamp&) {
+                                      if (valueFetchedByPreviousRequest.has_value()) {
+                                          ASSERT_EQ(cachedValue->counter,
+                                                    *valueFetchedByPreviousRequest);
+                                      } else {
+                                          ASSERT(!cachedValue);
+                                      }
+                                      return std::move(*nextToReturn);
+                                  });
+    // Issue two requests, advancing the time in store so that each of them will require a "remote
+    // lookup".
+    ASSERT(cache.advanceTimeInStore("TestKey", Timestamp(100)));
+    auto futureAtTS100 = cache.acquireAsync("TestKey", CacheCausalConsistency::kLatestKnown);
+    ASSERT(!futureAtTS100.isReady());
+
+    ASSERT(cache.advanceTimeInStore("TestKey", Timestamp(200)));
+    auto futureAtTS200 = cache.acquireAsync("TestKey", CacheCausalConsistency::kLatestKnown);
+    ASSERT(!futureAtTS100.isReady());
+    ASSERT(!futureAtTS200.isReady());
+
+    // Serve the first request - and verify that the received result only unblocks one future (with
+    // an already stale value).
+    nextToReturn.emplace(CachedValue(100), Timestamp(100));
+    valueFetchedByPreviousRequest = boost::none;
+    threadPool.runMostRecentTask();
+    ASSERT_EQ(100, futureAtTS100.get()->counter);
+    ASSERT(!futureAtTS100.get().isValid());
+    ASSERT(!futureAtTS200.isReady());
+
+    // Serve the second request - and verify that the cache's lookupFn receives the value retrieved
+    // by the first one to compute an "incremental remote lookup".
+    nextToReturn.emplace(CachedValue(200), Timestamp(200));
+    valueFetchedByPreviousRequest.emplace(100);
+    threadPool.runMostRecentTask();
+    ASSERT_EQ(200, futureAtTS200.get()->counter);
+    ASSERT(futureAtTS200.get().isValid());
+}
+
 }  // namespace
 }  // namespace mongo