SERVER-73820 Improve class encapsulation, comments and tests of the ReadThroughCache

1 files changed, 29 insertions, 20 deletions

diff --git a/src/mongo/util/read_through_cache.h b/src/mongo/util/read_through_cache.h
index 0bb10dc3fef..3ed61e98e39 100644
--- a/src/mongo/util/read_through_cache.h
+++ b/src/mongo/util/read_through_cache.h
@@ -31,7 +31,6 @@
 
 #include <boost/optional.hpp>
 
-#include "mongo/bson/oid.h"
 #include "mongo/db/operation_context.h"
 #include "mongo/platform/mutex.h"
 #include "mongo/util/concurrency/thread_pool_interface.h"
@@ -49,7 +48,7 @@ class ReadThroughCacheBase {
     ReadThroughCacheBase& operator=(const ReadThroughCacheBase&) = delete;
 
 protected:
-    ReadThroughCacheBase(Mutex& mutex, ServiceContext* service, ThreadPoolInterface& threadPool);
+    ReadThroughCacheBase(ServiceContext* service, ThreadPoolInterface& threadPool);
 
     virtual ~ReadThroughCacheBase();
 
@@ -84,17 +83,18 @@ protected:
     // functionality, such as client/operation context creation)
     ServiceContext* const _serviceContext;
 
+private:
     // Thread pool to be used for invoking the blocking 'lookup' calls
     ThreadPoolInterface& _threadPool;
 
-    // Used to protect the shared state in the child ReadThroughCache template below. Has a lock
-    // level of 3, meaning that while held, it is only allowed to take '_cancelTokenMutex' below and
-    // the Client lock.
-    Mutex& _mutex;
-
-    // Used to protect calls to 'tryCancel' above. Has a lock level of 2, meaning what while held,
-    // it is only allowed to take the Client lock.
-    Mutex _cancelTokenMutex = MONGO_MAKE_LATCH("ReadThroughCacheBase::_cancelTokenMutex");
+    // Used to protect calls to 'tryCancel' above and is shared across all emitted CancelTokens.
+    // Semantically, each CancelToken's interruption is independent from all the others so they
+    // could have their own mutexes, but in the interest of not creating a mutex for each async task
+    // spawned, we share the mutex here.
+    //
+    // Has a lock level of 2, meaning what while held, any code is only allowed to take the Client
+    // lock.
+    Mutex _cancelTokensMutex = MONGO_MAKE_LATCH("ReadThroughCacheBase::_cancelTokensMutex");
 };
 
 template <typename Result, typename Key, typename Value, typename Time>
@@ -496,7 +496,8 @@ public:
                      ThreadPoolInterface& threadPool,
                      LookupFn lookupFn,
                      int cacheSize)
-        : ReadThroughCacheBase(mutex, service, threadPool),
+        : ReadThroughCacheBase(service, threadPool),
+          _mutex(mutex),
           _lookupFn(std::move(lookupFn)),
           _cache(cacheSize) {}
 
@@ -511,10 +512,10 @@ private:
                                                      LruKeyComparator<Key>>;
 
     /**
-     * This method implements an asynchronous "while (!valid)" loop over 'key', which must be on the
-     * in-progress map.
+     * This method implements an asynchronous "while (!valid)" loop over the in-progress lookup
+     * object for 'key', which must have been previously placed on the in-progress map.
      */
-    Future<LookupResult> _doLookupWhileNotValid(Key key, StatusWith<LookupResult> sw) {
+    Future<LookupResult> _doLookupWhileNotValid(Key key, StatusWith<LookupResult> sw) noexcept {
         stdx::unique_lock ul(_mutex);
         auto it = _inProgressLookups.find(key);
         invariant(it != _inProgressLookups.end());
@@ -599,6 +600,11 @@ private:
             : Future<LookupResult>::makeReady(Status(ErrorCodes::Error(461542), ""));
     }
 
+    // Used to protect the shared below. Has a lock level of 3, meaning that while held, any code is
+    // only allowed to take '_cancelTokensMutex' (which in turn is allowed to be followed by the
+    // Client lock).
+    Mutex& _mutex;
+
     // Blocking function which will be invoked to retrieve entries from the backing store
     const LookupFn _lookupFn;
 
@@ -624,8 +630,8 @@ private:
  * the invalidation logic as described in the comments of 'ReadThroughCache::invalidate'.
  *
  * It is intended to be used in conjunction with the 'ReadThroughCache', which operates on it under
- * its '_mutex' and ensures there is always at most a single active instance at a time active for
- * each 'key'.
+ * its '_mutex' and ensures there is always at most one active instance at a time active for each
+ * 'key'.
  *
  * The methods of this class are not thread-safe, unless indicated in the comments.
  *
@@ -657,11 +663,12 @@ public:
 
         stdx::lock_guard lg(_cache._mutex);
         _valid = true;
-        _cancelToken.emplace(
-            _cache._asyncWork([this, promise = std::move(promise)](
-                                  OperationContext* opCtx, const Status& status) mutable noexcept {
+        _cancelToken.emplace(_cache._asyncWork(
+            [this, promise = std::move(promise)](
+                OperationContext* opCtx, const Status& cancelStatusAtTaskBegin) mutable noexcept {
                 promise.setWith([&] {
-                    uassertStatusOK(status);
+                    uassertStatusOK(cancelStatusAtTaskBegin);
+
                     if constexpr (std::is_same_v<Time, CacheNotCausallyConsistent>) {
                         return _cache._lookupFn(opCtx, _key, _cachedValue);
                     } else {
@@ -746,6 +753,8 @@ private:
 
     const Key _key;
 
+    // The validity status must start as false so that the first round ignores the error code with
+    // which it would have completed and will loop around
     bool _valid{false};
     boost::optional<CancelToken> _cancelToken;