diff --git a/cql3/authorized_prepared_statements_cache.hh b/cql3/authorized_prepared_statements_cache.hh index 392fa4fda8..9810f42c0c 100644 --- a/cql3/authorized_prepared_statements_cache.hh +++ b/cql3/authorized_prepared_statements_cache.hh @@ -136,9 +136,9 @@ public: {} future<> insert(auth::authenticated_user user, cql3::prepared_cache_key_type prep_cache_key, value_type v) noexcept { - return _cache.get_ptr(key_type(std::move(user), std::move(prep_cache_key)), [v = std::move(v)] (const cache_key_type&) mutable { + return _cache.insert(key_type(std::move(user), std::move(prep_cache_key)), [v = std::move(v)] (const cache_key_type&) mutable { return make_ready_future(std::move(v)); - }).discard_result(); + }); } value_ptr find(const auth::authenticated_user& user, const cql3::prepared_cache_key_type& prep_cache_key) { diff --git a/test/boost/loading_cache_test.cc b/test/boost/loading_cache_test.cc index 7896d5c7d3..1914a22a7c 100644 --- a/test/boost/loading_cache_test.cc +++ b/test/boost/loading_cache_test.cc @@ -823,4 +823,42 @@ SEASTAR_TEST_CASE(test_prepared_statement_small_cache) { }, small_cache_config); } +SEASTAR_THREAD_TEST_CASE(test_loading_cache_insert) { + using namespace std::chrono; + loader loader; + loading_cache_for_test loading_cache(num_loaders, 1h, testlog); + auto stop_cache = seastar::defer([&loading_cache] { loading_cache.stop().get(); }); + + // insert() must populate the cache and invoke the loader exactly once. + loading_cache.insert(0, loader.get()).get(); + BOOST_REQUIRE_EQUAL(loader.load_count(), 1); + BOOST_REQUIRE_EQUAL(loading_cache.size(), 1); + auto vp = loading_cache.find(0); + BOOST_REQUIRE(vp != nullptr); + BOOST_REQUIRE_EQUAL(*vp, test_string); + + // A second insert() for the same key must not re-invoke the loader. + loading_cache.insert(0, loader.get()).get(); + BOOST_REQUIRE_EQUAL(loader.load_count(), 1); + BOOST_REQUIRE_EQUAL(loading_cache.size(), 1); +} + +// Regression test for SCYLLADB-1699: insert() on a cache constructed with +// expiry == 0 (caching disabled) must be a no-op rather than asserting in +// loading_cache::get_ptr(). +SEASTAR_THREAD_TEST_CASE(test_loading_cache_insert_caching_disabled) { + using namespace std::chrono; + loader loader; + loading_cache_for_test loading_cache(num_loaders, 0ms, testlog); + auto stop_cache = seastar::defer([&loading_cache] { loading_cache.stop().get(); }); + + auto f = loading_cache.insert(0, loader.get()); + loading_cache.insert(0, loader.get()).get(); + + // The loader must not have been invoked and the cache must remain empty. + BOOST_REQUIRE_EQUAL(loader.load_count(), 0); + BOOST_REQUIRE_EQUAL(loading_cache.size(), 0); + BOOST_REQUIRE(loading_cache.find(0) == nullptr); +} + BOOST_AUTO_TEST_SUITE_END() diff --git a/utils/loading_cache.hh b/utils/loading_cache.hh index b4e4983004..ad76e69bb3 100644 --- a/utils/loading_cache.hh +++ b/utils/loading_cache.hh @@ -65,8 +65,9 @@ struct do_nothing_loading_cache_stats { /// The values are going to be evicted from the cache if they are not accessed during the "expiration" period or haven't /// been reloaded even once during the same period. /// -/// If "expiration" is set to zero - the caching is going to be disabled and get_XXX(...) is going to call the "loader" callback -/// every time in order to get the requested value. +/// If "expiration" is set to zero - the caching is going to be disabled and get(...) is going to call the "loader" callback +/// every time in order to get the requested value. insert(...) is going to be a no-op in this mode. get_ptr(...) is not +/// safe to call when caching is disabled (it asserts) since it returns a handle into the cache. /// /// \note In order to avoid the eviction of cached entries due to "aging" of the contained value the user has to choose /// the "expiration" to be at least ("refresh" + "max load latency"). This way the value is going to stay in the cache and is going to be @@ -353,6 +354,24 @@ public: return get_ptr(k, _load); } + /// \brief Insert a value into the cache, loading it via \p load if not already present. + /// + /// Equivalent to get_ptr(k, load).discard_result() when caching is enabled, + /// but is a no-op when caching is disabled (i.e. the cache was constructed + /// with expiry == 0). Use this when you only want the side effect of + /// populating the cache and don't need a handle to the cached value. + /// + /// Unlike get_ptr(), it is safe to call this on a cache configured with + /// caching disabled. + template + requires std::is_invocable_r_v, LoadFunc, const key_type&> + future<> insert(const Key& k, LoadFunc&& load) { + if (!caching_enabled()) { + return make_ready_future<>(); + } + return get_ptr(k, std::forward(load)).discard_result(); + } + future get(const Key& k) { static_assert(ReloadEnabled == loading_cache_reload_enabled::yes, "");