diff --git a/source/extensions/filters/http/cache/simple_http_cache/simple_http_cache.cc b/source/extensions/filters/http/cache/simple_http_cache/simple_http_cache.cc index 24471f8154fcb..fefb0677a34a4 100644 --- a/source/extensions/filters/http/cache/simple_http_cache/simple_http_cache.cc +++ b/source/extensions/filters/http/cache/simple_http_cache/simple_http_cache.cc @@ -65,9 +65,10 @@ class SimpleInsertContext : public InsertContext { response_headers_ = Http::createHeaderMap(response_headers); metadata_ = metadata; if (end_stream) { - commit(); + insert_success(commit()); + } else { + insert_success(true); } - insert_success(true); } void insertBody(const Buffer::Instance& chunk, InsertCallback ready_for_next_chunk, @@ -77,30 +78,31 @@ class SimpleInsertContext : public InsertContext { body_.add(chunk); if (end_stream) { - commit(); + ready_for_next_chunk(commit()); + } else { + ready_for_next_chunk(true); } - ready_for_next_chunk(true); } void insertTrailers(const Http::ResponseTrailerMap& trailers, InsertCallback insert_complete) override { ASSERT(!committed_); trailers_ = Http::createHeaderMap(trailers); - commit(); - insert_complete(true); + insert_complete(commit()); } void onDestroy() override {} private: - void commit() { + bool commit() { committed_ = true; if (VaryHeaderUtils::hasVary(*response_headers_)) { - cache_.varyInsert(key_, std::move(response_headers_), std::move(metadata_), body_.toString(), - request_headers_, vary_allow_list_, std::move(trailers_)); + return cache_.varyInsert(key_, std::move(response_headers_), std::move(metadata_), + body_.toString(), request_headers_, vary_allow_list_, + std::move(trailers_)); } else { - cache_.insert(key_, std::move(response_headers_), std::move(metadata_), body_.toString(), - std::move(trailers_)); + return cache_.insert(key_, std::move(response_headers_), std::move(metadata_), + body_.toString(), std::move(trailers_)); } } @@ -209,12 +211,13 @@ SimpleHttpCache::Entry SimpleHttpCache::lookup(const LookupRequest& request) { } } -void SimpleHttpCache::insert(const Key& key, Http::ResponseHeaderMapPtr&& response_headers, +bool SimpleHttpCache::insert(const Key& key, Http::ResponseHeaderMapPtr&& response_headers, ResponseMetadata&& metadata, std::string&& body, Http::ResponseTrailerMapPtr&& trailers) { absl::WriterMutexLock lock(&mutex_); map_[key] = SimpleHttpCache::Entry{std::move(response_headers), std::move(metadata), std::move(body), std::move(trailers)}; + return true; } SimpleHttpCache::Entry @@ -252,7 +255,7 @@ SimpleHttpCache::varyLookup(const LookupRequest& request, iter->second.metadata_, iter->second.body_, std::move(trailers_map)}; } -void SimpleHttpCache::varyInsert(const Key& request_key, +bool SimpleHttpCache::varyInsert(const Key& request_key, Http::ResponseHeaderMapPtr&& response_headers, ResponseMetadata&& metadata, std::string&& body, const Http::RequestHeaderMap& request_headers, @@ -270,7 +273,7 @@ void SimpleHttpCache::varyInsert(const Key& request_key, VaryHeaderUtils::createVaryIdentifier(vary_allow_list, vary_header_values, request_headers); if (!vary_identifier.has_value()) { // Skip the insert if we are unable to create a vary key. - return; + return false; } varied_request_key.add_custom_fields(vary_identifier.value()); @@ -292,6 +295,7 @@ void SimpleHttpCache::varyInsert(const Key& request_key, map_[request_key] = SimpleHttpCache::Entry{std::move(vary_only_map), {}, std::move(entry_list), {}}; } + return true; } InsertContextPtr SimpleHttpCache::makeInsertContext(LookupContextPtr&& lookup_context, diff --git a/source/extensions/filters/http/cache/simple_http_cache/simple_http_cache.h b/source/extensions/filters/http/cache/simple_http_cache/simple_http_cache.h index 464d2d45a8879..5194edcdf0e34 100644 --- a/source/extensions/filters/http/cache/simple_http_cache/simple_http_cache.h +++ b/source/extensions/filters/http/cache/simple_http_cache/simple_http_cache.h @@ -47,12 +47,12 @@ class SimpleHttpCache : public HttpCache, public Singleton::Instance { CacheInfo cacheInfo() const override; Entry lookup(const LookupRequest& request); - void insert(const Key& key, Http::ResponseHeaderMapPtr&& response_headers, + bool insert(const Key& key, Http::ResponseHeaderMapPtr&& response_headers, ResponseMetadata&& metadata, std::string&& body, Http::ResponseTrailerMapPtr&& trailers); // Inserts a response that has been varied on certain headers. - void varyInsert(const Key& request_key, Http::ResponseHeaderMapPtr&& response_headers, + bool varyInsert(const Key& request_key, Http::ResponseHeaderMapPtr&& response_headers, ResponseMetadata&& metadata, std::string&& body, const Http::RequestHeaderMap& request_headers, const VaryAllowList& vary_allow_list, Http::ResponseTrailerMapPtr&& trailers); diff --git a/test/extensions/filters/http/cache/http_cache_implementation_test_common.cc b/test/extensions/filters/http/cache/http_cache_implementation_test_common.cc index e8200a5ffc4ad..939c4b2f830ec 100644 --- a/test/extensions/filters/http/cache/http_cache_implementation_test_common.cc +++ b/test/extensions/filters/http/cache/http_cache_implementation_test_common.cc @@ -20,6 +20,7 @@ using ::envoy::extensions::filters::http::cache::v3::CacheConfig; using ::testing::_; using ::testing::AnyNumber; +using ::testing::Not; namespace Envoy { namespace Extensions { @@ -486,7 +487,7 @@ TEST_P(HttpCacheImplementationTest, VaryOnDisallowedKey) { LookupContextPtr first_value_vary = lookup(request_path); EXPECT_EQ(CacheEntryStatus::Unusable, lookup_result_.cache_entry_status_); const std::string body("one"); - ASSERT_THAT(insert(move(first_value_vary), response_headers, body), IsOk()); + ASSERT_THAT(insert(move(first_value_vary), response_headers, body), Not(IsOk())); first_value_vary = lookup(request_path); EXPECT_EQ(CacheEntryStatus::Unusable, lookup_result_.cache_entry_status_); }