Skip to content

[cache] Make an aborted insert operation call the callback with false #23136

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
mattklein123 merged 2 commits into from
Sep 19, 2022
Merged

[cache] Make an aborted insert operation call the callback with false #23136

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
427d23c
[cache] Make an aborted insert operation call the callback with false
ravenblackx Sep 15, 2022
9531554
empty commit
ravenblackx Sep 16, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 18 additions & 14 deletions source/extensions/filters/http/cache/simple_http_cache/simple_http_cache.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,9 +65,10 @@ class SimpleInsertContext : public InsertContext {
response_headers_ = Http::createHeaderMap<Http::ResponseHeaderMapImpl>(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,
Expand All @@ -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<Http::ResponseTrailerMapImpl>(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_));

@jmarantz jmarantz Sep 16, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this didn't really change this PR but should we be thinking about a zero-copy way to transfer the body?

@ravenblackx ravenblackx Sep 16, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think simple_http_cache is just supposed to be an example, not a serious implementation. But also I think zero-copy isn't really an option anyway because the original version is still being used for the response - you can't have one used for the response and a separate one for the cache with no copying. (And you can't use the same one because the response path is going to destroy it.)

}
}

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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,
Expand All @@ -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());
Expand All @@ -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,
Expand Down
4 changes: 2 additions & 2 deletions source/extensions/filters/http/cache/simple_http_cache/simple_http_cache.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
3 changes: 2 additions & 1 deletion test/extensions/filters/http/cache/http_cache_implementation_test_common.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
using ::envoy::extensions::filters::http::cache::v3::CacheConfig;
using ::testing::_;
using ::testing::AnyNumber;
using ::testing::Not;

namespace Envoy {
namespace Extensions {
Expand Down Expand Up @@ -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_);
}
Expand Down