SERVER-62477 Cleanup the interface and comments of ReadThroughCache

7 files changed, 131 insertions, 111 deletions

diff --git a/src/mongo/db/auth/authorization_manager_impl.cpp b/src/mongo/db/auth/authorization_manager_impl.cpp
index 66a84083482..a733e5bf058 100644
--- a/src/mongo/db/auth/authorization_manager_impl.cpp
+++ b/src/mongo/db/auth/authorization_manager_impl.cpp
@@ -679,7 +679,7 @@ void AuthorizationManagerImpl::invalidateUserByName(OperationContext* opCtx,
     _authSchemaVersionCache.invalidateAll();
     // Invalidate the named User, assuming no externally provided roles. When roles are defined
     // externally, there exists no user document which may become invalid.
-    _userCache.invalidate(UserRequest(userName, boost::none));
+    _userCache.invalidateKey(UserRequest(userName, boost::none));
 }
 
 void AuthorizationManagerImpl::invalidateUsersFromDB(OperationContext* opCtx, StringData dbname) {
@@ -700,8 +700,10 @@ void AuthorizationManagerImpl::invalidateUserCache(OperationContext* opCtx) {
 Status AuthorizationManagerImpl::refreshExternalUsers(OperationContext* opCtx) {
     LOGV2_DEBUG(5914801, 2, "Refreshing all users from the $external database");
     // First, get a snapshot of the UserHandles in the cache.
-    std::vector<UserHandle> cachedUsers = _userCache.getValueHandlesIfKey(
-        [&](const UserRequest& userRequest) { return userRequest.name.getDB() == "$external"_sd; });
+    auto cachedUsers =
+        _userCache.peekLatestCachedIf([&](const UserRequest& userRequest, const User&) {
+            return userRequest.name.getDB() == "$external"_sd;
+        });
 
     // Then, retrieve the corresponding Users from the backing store for users in the $external
     // database. Compare each of these user objects with the cached user object and call
@@ -713,7 +715,7 @@ Status AuthorizationManagerImpl::refreshExternalUsers(OperationContext* opCtx) {
         if (!storedUserStatus.isOK()) {
             // If the user simply is not found, then just invalidate the cached user and continue.
             if (storedUserStatus.getStatus().code() == ErrorCodes::UserNotFound) {
-                _userCache.invalidate(request);
+                _userCache.invalidateKey(request);
                 continue;
             } else {
                 return storedUserStatus.getStatus();
diff --git a/src/mongo/db/read_write_concern_defaults.cpp b/src/mongo/db/read_write_concern_defaults.cpp
index 7f91d0607e5..50e6ada1832 100644
--- a/src/mongo/db/read_write_concern_defaults.cpp
+++ b/src/mongo/db/read_write_concern_defaults.cpp
@@ -174,7 +174,7 @@ void ReadWriteConcernDefaults::observeDirectWriteToConfigSettings(OperationConte
 }
 
 void ReadWriteConcernDefaults::invalidate() {
-    _defaults.invalidate(Type::kReadWriteConcernEntry);
+    _defaults.invalidateKey(Type::kReadWriteConcernEntry);
 }
 
 void ReadWriteConcernDefaults::setDefault(OperationContext* opCtx, RWConcernDefault&& rwc) {
diff --git a/src/mongo/s/catalog_cache.cpp b/src/mongo/s/catalog_cache.cpp
index 0ac8a70e023..ea0a5fdb444 100644
--- a/src/mongo/s/catalog_cache.cpp
+++ b/src/mongo/s/catalog_cache.cpp
@@ -362,7 +362,7 @@ void CatalogCache::onStaleDatabaseVersion(const StringData dbName,
                                   "version"_attr = version.toBSONForLogging());
         _databaseCache.advanceTimeInStore(dbName, version);
     } else {
-        _databaseCache.invalidate(dbName);
+        _databaseCache.invalidateKey(dbName);
     }
 }
 
@@ -403,27 +403,28 @@ void CatalogCache::invalidateEntriesThatReferenceShard(const ShardId& shardId) {
                 "Invalidating databases and collections referencing a specific shard",
                 "shardId"_attr = shardId);
 
-    _databaseCache.invalidateCachedValueIf(
-        [&](const DatabaseType& dbt) { return dbt.getPrimary() == shardId; });
+    _databaseCache.invalidateLatestCachedValueIf_IgnoreInProgress(
+        [&](const std::string&, const DatabaseType& dbt) { return dbt.getPrimary() == shardId; });
 
     // Invalidate collections which contain data on this shard.
-    _collectionCache.invalidateCachedValueIf([&](const OptionalRoutingTableHistory& ort) {
-        if (!ort.optRt)
-            return false;
-        const auto& rt = *ort.optRt;
-
-        std::set<ShardId> shardIds;
-        rt.getAllShardIds(&shardIds);
-
-        LOGV2_DEBUG(22647,
-                    3,
-                    "Invalidating cached collection {namespace} that has data "
-                    "on shard {shardId}",
-                    "Invalidating cached collection",
-                    "namespace"_attr = rt.nss(),
-                    "shardId"_attr = shardId);
-        return shardIds.find(shardId) != shardIds.end();
-    });
+    _collectionCache.invalidateLatestCachedValueIf_IgnoreInProgress(
+        [&](const NamespaceString&, const OptionalRoutingTableHistory& ort) {
+            if (!ort.optRt)
+                return false;
+            const auto& rt = *ort.optRt;
+
+            std::set<ShardId> shardIds;
+            rt.getAllShardIds(&shardIds);
+
+            LOGV2_DEBUG(22647,
+                        3,
+                        "Invalidating cached collection {namespace} that has data "
+                        "on shard {shardId}",
+                        "Invalidating cached collection",
+                        "namespace"_attr = rt.nss(),
+                        "shardId"_attr = shardId);
+            return shardIds.find(shardId) != shardIds.end();
+        });
 
     LOGV2(22648,
           "Finished invalidating databases and collections with data on shard: {shardId}",
@@ -432,7 +433,7 @@ void CatalogCache::invalidateEntriesThatReferenceShard(const ShardId& shardId) {
 }
 
 void CatalogCache::purgeDatabase(StringData dbName) {
-    _databaseCache.invalidate(dbName);
+    _databaseCache.invalidateKey(dbName);
     _collectionCache.invalidateKeyIf(
         [&](const NamespaceString& nss) { return nss.db() == dbName; });
 }
@@ -487,11 +488,11 @@ void CatalogCache::checkAndRecordOperationBlockedByRefresh(OperationContext* opC
 }
 
 void CatalogCache::invalidateDatabaseEntry_LINEARIZABLE(const StringData& dbName) {
-    _databaseCache.invalidate(dbName);
+    _databaseCache.invalidateKey(dbName);
 }
 
 void CatalogCache::invalidateCollectionEntry_LINEARIZABLE(const NamespaceString& nss) {
-    _collectionCache.invalidate(nss);
+    _collectionCache.invalidateKey(nss);
 }
 
 void CatalogCache::Stats::report(BSONObjBuilder* builder) const {
diff --git a/src/mongo/util/invalidating_lru_cache.h b/src/mongo/util/invalidating_lru_cache.h
index d07eb12e0ec..1ceb55aec51 100644
--- a/src/mongo/util/invalidating_lru_cache.h
+++ b/src/mongo/util/invalidating_lru_cache.h
@@ -431,6 +431,33 @@ public:
     }
 
     /**
+     * Returns a vector of the latest values from the cache which satisfy the predicate. Uses the
+     * 'kLatestCached' causal consistency model.
+     */
+    template <typename Pred>
+    std::vector<ValueHandle> getLatestCachedIf(Pred predicate) {
+        stdx::lock_guard lg(_mutex);
+        std::vector<ValueHandle> entries;
+        entries.reserve(_cache.size() + _evictedCheckedOutValues.size());
+
+        for (const auto& kv : _cache) {
+            if (predicate(kv.first, &kv.second->value)) {
+                entries.push_back(ValueHandle(kv.second));
+            }
+        }
+
+        for (const auto& kv : _evictedCheckedOutValues) {
+            if (auto storedValue = kv.second.lock()) {
+                if (predicate(kv.first, &storedValue->value)) {
+                    entries.push_back(ValueHandle(std::move(storedValue)));
+                }
+            }
+        }
+
+        return entries;
+    }
+
+    /**
      * Indicates to the cache that the backing store contains a new value for the specified key,
      * with a timestamp of 'newTimeInStore'.
      *
@@ -535,32 +562,6 @@ public:
         }
     }
 
-    /**
-     * Returns a vector of ValueHandles for all of the entries that satisfy matchPredicate.
-     */
-    template <typename Pred>
-    std::vector<ValueHandle> getEntriesIf(Pred matchPredicate) {
-        std::vector<ValueHandle> entries;
-        entries.reserve(_cache.size() + _evictedCheckedOutValues.size());
-        {
-            stdx::lock_guard lg(_mutex);
-            for (const auto& entry : _cache) {
-                if (matchPredicate(entry.first, &entry.second->value)) {
-                    entries.push_back(ValueHandle(entry.second));
-                }
-            }
-
-            for (const auto& entry : _evictedCheckedOutValues) {
-                if (auto storedValue = entry.second.lock()) {
-                    if (matchPredicate(entry.first, &storedValue->value)) {
-                        entries.push_back(ValueHandle(std::move(storedValue)));
-                    }
-                }
-            }
-        }
-        return entries;
-    }
-
     struct CachedItemInfo {
         Key key;            // The key of the item in the cache
         long int useCount;  // The number of callers of 'get', which still have the item checked-out
diff --git a/src/mongo/util/net/ssl_manager_openssl.cpp b/src/mongo/util/net/ssl_manager_openssl.cpp
index 211b0ab5e50..f2ddee148c7 100644
--- a/src/mongo/util/net/ssl_manager_openssl.cpp
+++ b/src/mongo/util/net/ssl_manager_openssl.cpp
@@ -2108,7 +2108,7 @@ Future<void> SSLManagerOpenSSL::ocspClientVerification(SSL* ssl, const ExecutorP
         auto timeNow = Date_t::now();
 
         if (validatedResponse.second.get() < timeNow) {
-            cache.invalidate(cacheKey);
+            cache.invalidateKey(cacheKey);
             auto semifuture = cache.acquireAsync(cacheKey);
             return convert(std::move(semifuture))
                 .onCompletion(validate)
diff --git a/src/mongo/util/read_through_cache.h b/src/mongo/util/read_through_cache.h
index 65c5a39d96c..bf9366db3fc 100644
--- a/src/mongo/util/read_through_cache.h
+++ b/src/mongo/util/read_through_cache.h
@@ -307,7 +307,8 @@ public:
      * Acquires the latest value from the cache, or an empty ValueHandle if the key is not present
      * in the cache.
      *
-     * Doesn't attempt to lookup, and so doesn't block.
+     * Doesn't attempt to lookup, and so doesn't block, but this means it will ignore any
+     * in-progress keys or keys whose time in store is newer than what is currently cached.
      */
     TEMPLATE(typename KeyType)
     REQUIRES(IsComparable<KeyType>)
@@ -316,26 +317,52 @@ public:
     }
 
     /**
+     * Returns a vector of the latest values from the cache which satisfy the predicate.
+     *
+     * Doesn't attempt to lookup, and so doesn't block, but this means it will ignore any
+     * in-progress keys or keys whose time in store is newer than what is currently cached.
+     */
+    template <typename Pred>
+    std::vector<ValueHandle> peekLatestCachedIf(const Pred& pred) {
+        auto invalidatingCacheValues = [&] {
+            stdx::lock_guard lg(_mutex);
+            return _cache.getLatestCachedIf(
+                [&](const Key& key, const StoredValue* value) { return pred(key, value->value); });
+        }();
+
+        std::vector<ValueHandle> valueHandles;
+        valueHandles.reserve(invalidatingCacheValues.size());
+        std::transform(invalidatingCacheValues.begin(),
+                       invalidatingCacheValues.end(),
+                       std::back_inserter(valueHandles),
+                       [](auto& invalidatingCacheValue) {
+                           return ValueHandle(std::move(invalidatingCacheValue));
+                       });
+
+        return valueHandles;
+    }
+
+    /**
      * Invalidates the given 'key' and immediately replaces it with a new value.
      *
      * The 'time' parameter is mandatory for causally-consistent caches, but not needed otherwise
      * (since the time never changes).
      */
-    void insertOrAssign(const Key& key, Value&& newValue, Date_t updateWallClockTime) {
-        stdx::lock_guard lg(_mutex);
-        if (auto it = _inProgressLookups.find(key); it != _inProgressLookups.end())
-            it->second->invalidateAndCancelCurrentLookupRound(lg);
-        _cache.insertOrAssign(key, {std::move(newValue), updateWallClockTime});
+    void insertOrAssign(const Key& key, Value&& value, Date_t updateWallClockTime) {
+        MONGO_STATIC_ASSERT_MSG(
+            !isCausallyConsistent<Time>,
+            "Time must be passed to insertOrAssign on causally consistent caches");
+        insertOrAssign(key, std::move(value), updateWallClockTime, Time());
     }
 
     void insertOrAssign(const Key& key,
-                        Value&& newValue,
+                        Value&& value,
                         Date_t updateWallClockTime,
                         const Time& time) {
         stdx::lock_guard lg(_mutex);
         if (auto it = _inProgressLookups.find(key); it != _inProgressLookups.end())
             it->second->invalidateAndCancelCurrentLookupRound(lg);
-        _cache.insertOrAssign(key, {std::move(newValue), updateWallClockTime}, time);
+        _cache.insertOrAssign(key, {std::move(value), updateWallClockTime}, time);
     }
 
     /**
@@ -345,21 +372,21 @@ public:
      * The 'time' parameter is mandatory for causally-consistent caches, but not needed otherwise
      * (since the time never changes).
      */
-    ValueHandle insertOrAssignAndGet(const Key& key, Value&& newValue, Date_t updateWallClockTime) {
-        stdx::lock_guard lg(_mutex);
-        if (auto it = _inProgressLookups.find(key); it != _inProgressLookups.end())
-            it->second->invalidateAndCancelCurrentLookupRound(lg);
-        return _cache.insertOrAssignAndGet(key, {std::move(newValue), updateWallClockTime});
+    ValueHandle insertOrAssignAndGet(const Key& key, Value&& value, Date_t updateWallClockTime) {
+        MONGO_STATIC_ASSERT_MSG(
+            !isCausallyConsistent<Time>,
+            "Time must be passed to insertOrAssign on causally consistent caches");
+        return insertOrAssignAndGet(key, std::move(value), updateWallClockTime, Time());
     }
 
     ValueHandle insertOrAssignAndGet(const Key& key,
-                                     Value&& newValue,
+                                     Value&& value,
                                      Date_t updateWallClockTime,
                                      const Time& time) {
         stdx::lock_guard lg(_mutex);
         if (auto it = _inProgressLookups.find(key); it != _inProgressLookups.end())
             it->second->invalidateAndCancelCurrentLookupRound(lg);
-        return _cache.insertOrAssignAndGet(key, {std::move(newValue), updateWallClockTime}, time);
+        return _cache.insertOrAssignAndGet(key, {std::move(value), updateWallClockTime}, time);
     }
 
     /**
@@ -371,7 +398,7 @@ public:
      * guarantee that if 'advanceTimeInStore' is called with a 'newTime', a subsequent call to
      * 'LookupFn' for 'key' must return at least 'newTime' or later.
      *
-     * Returns true if the passed 'newTimeInStore' is grater than the time of the currently cached
+     * Returns true if the passed 'newTimeInStore' is greater than the time of the currently cached
      * value or if no value is cached for 'key'.
      */
     TEMPLATE(typename KeyType)
@@ -384,18 +411,19 @@ public:
     }
 
     /**
-     * The invalidate methods below guarantee the following:
+     * The invalidate+ methods below guarantee the following:
      *  - All affected keys already in the cache (or returned to callers) will be invalidated and
      *    removed from the cache
      *  - All affected keys, which are in the process of being loaded (i.e., acquireAsync has not
      *    yet completed) will be internally interrupted and rescheduled again, as if 'acquireAsync'
      *    was called *after* the call to invalidate
      *
-     * In essence, the invalidate calls serve as a "barrier" for the affected keys.
+     * In essence, the invalidate+ calls serve as an externally induced "barrier" for the affected
+     * keys.
      */
     TEMPLATE(typename KeyType)
     REQUIRES(IsComparable<KeyType>)
-    void invalidate(const KeyType& key) {
+    void invalidateKey(const KeyType& key) {
         stdx::lock_guard lg(_mutex);
         if (auto it = _inProgressLookups.find(key); it != _inProgressLookups.end())
             it->second->invalidateAndCancelCurrentLookupRound(lg);
@@ -403,53 +431,39 @@ public:
     }
 
     /**
-     * Invalidates all cached entries and in progress lookups with keys that matches the preidcate.
+     * Invalidates only the entries whose key is matched by the predicate.
      */
     template <typename Pred>
-    void invalidateKeyIf(const Pred& predicate) {
+    void invalidateKeyIf(const Pred& pred) {
         stdx::lock_guard lg(_mutex);
         for (auto& entry : _inProgressLookups) {
-            if (predicate(entry.first))
+            if (pred(entry.first))
                 entry.second->invalidateAndCancelCurrentLookupRound(lg);
         }
-        _cache.invalidateIf([&](const Key& key, const StoredValue*) { return predicate(key); });
+        _cache.invalidateIf([&](const Key& key, const StoredValue*) { return pred(key); });
     }
 
     /**
-     * Invalidates all cached entries with stored values that matches the preidcate.
+     * Invalidates all entries.
      */
-    template <typename Pred>
-    void invalidateCachedValueIf(const Pred& predicate) {
-        stdx::lock_guard lg(_mutex);
-        _cache.invalidateIf(
-            [&](const Key&, const StoredValue* value) { return predicate(value->value); });
-    }
-
     void invalidateAll() {
         invalidateKeyIf([](const Key&) { return true; });
     }
 
     /**
-     * Returns a vector of ValueHandles for all of the keys that satisfy matchPredicate.
+     * The method below guarantees only that the affected key/value(s) already in the cache (or
+     * returned to callers) will be invalidated and removed from the cache. However, any affected
+     * keys, which are in the process of being loaded (i.e., acquireAsync has not yet completed)
+     * will not be interrupted and will eventually end-up on the cache.
+     *
+     * Because the behaviour described above does not provide any guarantees about the in-progress
+     * lookups, it should be considered as "best-effort".
      */
     template <typename Pred>
-    std::vector<ValueHandle> getValueHandlesIfKey(const Pred& matchPredicate) {
-        auto invalidatingCacheValues = [&]() {
-            stdx::lock_guard lg(_mutex);
-            return _cache.getEntriesIf(
-                [&](const Key& key, const StoredValue*) { return matchPredicate(key); });
-        }();
-
-        std::vector<ValueHandle> valueHandles;
-        valueHandles.reserve(invalidatingCacheValues.size());
-        std::transform(invalidatingCacheValues.begin(),
-                       invalidatingCacheValues.end(),
-                       std::back_inserter(valueHandles),
-                       [](auto& invalidatingCacheValue) {
-                           return ValueHandle(std::move(invalidatingCacheValue));
-                       });
-
-        return valueHandles;
+    void invalidateLatestCachedValueIf_IgnoreInProgress(const Pred& pred) {
+        stdx::lock_guard lg(_mutex);
+        _cache.invalidateIf(
+            [&](const Key& key, const StoredValue* value) { return pred(key, value->value); });
     }
 
     /**
diff --git a/src/mongo/util/read_through_cache_test.cpp b/src/mongo/util/read_through_cache_test.cpp
index ae6d89a1a70..c1174b8e23d 100644
--- a/src/mongo/util/read_through_cache_test.cpp
+++ b/src/mongo/util/read_through_cache_test.cpp
@@ -145,7 +145,7 @@ TEST_F(ReadThroughCacheTest, FetchInvalidateAndRefetch) {
             ASSERT(cache.acquire(_opCtx, "TestKey"));
             ASSERT_EQ(i, cache.countLookups);
 
-            cache.invalidate("TestKey");
+            cache.invalidateKey("TestKey");
         }
     };
 
@@ -221,8 +221,10 @@ TEST_F(ReadThroughCacheTest, FetchInvalidateValueAndRefetch) {
             ASSERT(cache.acquire(_opCtx, "TestKey"));
             ASSERT_EQ(i, cache.countLookups);
 
-            cache.invalidateCachedValueIf(
-                [i](const CachedValue& value) { return value.counter == 100 * i; });
+            cache.invalidateLatestCachedValueIf_IgnoreInProgress(
+                [i](const std::string&, const CachedValue& value) {
+                    return value.counter == 100 * i;
+                });
         }
     };
 
@@ -284,7 +286,7 @@ TEST_F(ReadThroughCacheTest, InvalidateCacheSizeZeroReissuesLookup) {
         ASSERT_EQ(1000, cache.acquire(_opCtx, "TestKey")->counter);
         ASSERT_EQ(1, cache.countLookups);
 
-        cache.invalidate("TestKey");
+        cache.invalidateKey("TestKey");
         auto valueAfterInvalidate = cache.acquire(_opCtx, "TestKey");
         ASSERT(!value.isValid());
         ASSERT(valueAfterInvalidate);
@@ -535,7 +537,7 @@ TEST_F(ReadThroughCacheAsyncTest, InvalidateReissuesLookup) {
     // Wait for the first lookup attempt to start and invalidate it before letting it proceed
     lookupStartedBarriers[0].countDownAndWait();
     ASSERT_EQ(1, countLookups.load());
-    cache.invalidate("TestKey");
+    cache.invalidateKey("TestKey");
     ASSERT(!future.isReady());
     completeLookupBarriers[0].countDownAndWait();  // Lets lookup attempt 1 proceed
     ASSERT(!future.isReady());
@@ -543,7 +545,7 @@ TEST_F(ReadThroughCacheAsyncTest, InvalidateReissuesLookup) {
     // Wait for the second lookup attempt to start and invalidate it before letting it proceed
     lookupStartedBarriers[1].countDownAndWait();
     ASSERT_EQ(2, countLookups.load());
-    cache.invalidate("TestKey");
+    cache.invalidateKey("TestKey");
     ASSERT(!future.isReady());
     completeLookupBarriers[1].countDownAndWait();  // Lets lookup attempt 2 proceed
     ASSERT(!future.isReady());
@@ -604,7 +606,7 @@ TEST_F(ReadThroughCacheAsyncTest, ShutdownWithConcurrentInvalidate) {
     auto future = cache.acquireAsync("async", CacheCausalConsistency::kLatestCached);
 
     lookupStartedBarrier.countDownAndWait();
-    cache.invalidate("async");
+    cache.invalidateKey("async");
     completeLookupBarrier.countDownAndWait();
 
     ASSERT_THROWS_CODE(future.get(), DBException, ErrorCodes::InterruptedAtShutdown);