[cache_filter] Separate upstream from downstream #36309
Conversation
Signed-off-by: Raven Black <ravenblack@dropbox.com>
|
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to |
|
/coverage |
|
Coverage for this Pull Request will be rendered here: https://storage.googleapis.com/envoy-pr/36309/coverage/index.html The coverage results are (re-)rendered each time the CI |
Signed-off-by: Raven Black <ravenblack@dropbox.com>
Signed-off-by: Raven Black <ravenblack@dropbox.com>
Signed-off-by: Raven Black <ravenblack@dropbox.com>
Signed-off-by: Raven Black <ravenblack@dropbox.com>
Signed-off-by: Raven Black <ravenblack@dropbox.com>
Signed-off-by: Raven Black <ravenblack@dropbox.com>
Signed-off-by: Raven Black <ravenblack@dropbox.com>
Signed-off-by: Raven Black <ravenblack@dropbox.com>
Signed-off-by: Raven Black <ravenblack@dropbox.com>
|
|
||
| static LookupStatus resolveLookupStatus(absl::optional<CacheEntryStatus> cache_entry_status, | ||
| FilterState filter_state); | ||
|
|
||
| private: | ||
| class UpstreamRequest : public Http::AsyncClient::StreamCallbacks, public InsertQueueCallbacks { |
There was a problem hiding this comment.
IMO, it would be improve readability and reviewability if UpstreamRequest had its own .h/.cc files (possibly in an "internal" namespace)
| std::unique_ptr<CacheInsertQueue> insert_queue_; | ||
| // upstream_request_ belongs to the object itself, so that it can be disconnected | ||
| // from the filter and still complete the cache-write in the event that the | ||
| // downstream disconnects. The filter and the UpstreamRequest must communicate to |
There was a problem hiding this comment.
This seems like a good place to mention what CacheFilter functions UpstreamRequest calls when it's about to be destroyed
| class UpstreamRequest : public Http::AsyncClient::StreamCallbacks, public InsertQueueCallbacks { | ||
| public: | ||
| void sendHeaders(Http::RequestHeaderMap& request_headers); | ||
| void disconnectFilter(); |
There was a problem hiding this comment.
Can disconnectFilter document that once it returns, UpstreamRequest will issue no more calls to CacheFilter?
| FilterState filter_state_; | ||
| std::shared_ptr<HttpCache> cache_; | ||
| Http::AsyncClient::Stream* stream_ = nullptr; | ||
| std::unique_ptr<UpstreamRequest> self_ownership_; |
There was a problem hiding this comment.
What is the benefit of self_ownership_? AFAICT, every self_ownership = nullptr could be replaced with delete this, and the code would be more clear.
There was a problem hiding this comment.
I think we have linters that aren't big fans of bare pointers, plus I am not a fan of bare pointers (I think the ownership semantics are much clearer with explicitly self_ownership), plus in one of the later iterations it's going to become a smart pointer that isn't owned by itself so it's more convenient to not have it be a bare pointer temporarily and then have to rejigger it. :)
| static UpstreamRequest* create(CacheFilter* filter, std::shared_ptr<HttpCache> cache, | ||
| Http::AsyncClient& async_client, | ||
| const Http::AsyncClient::StreamOptions& options); | ||
| UpstreamRequest(CacheFilter* filter, std::shared_ptr<HttpCache> cache, |
There was a problem hiding this comment.
If we're going to have a create method, the constructor should probably be private
There was a problem hiding this comment.
That's annoying with smart pointers, unfortunately. (Yeah you can do a unique_ptr wrapped around a new instead of a make_unique, I think that's more problematic than a public constructor in what is essentially a private context anyway.)
| } | ||
| watermarked_ = false; | ||
| } | ||
| fragments_.clear(); | ||
| // Clearing self-ownership might provoke the destructor, so take a copy of the | ||
| // abort callback to avoid reading from 'this' after it may be deleted. | ||
| auto abort_callback = std::move(abort_callback_); | ||
| auto callbacks = std::move(callbacks_); |
There was a problem hiding this comment.
Seems simpler just to call insertQueueAborted before resetting self_ownership_
There was a problem hiding this comment.
Added more comment explaining why that would be a mistake. (It's likely this is one of the things that will be tidier after shared-cache-streaming responses make the ownership semantics less tangled.)
| } | ||
| } | ||
|
|
||
| void CacheFilter::UpstreamRequest::sendHeaders(Http::RequestHeaderMap& request_headers) { | ||
| stream_->sendHeaders(request_headers, true); |
There was a problem hiding this comment.
suggested comment:
// If this request had a body or trailers, CacheFilter::decodeHeaders would have bypassed cache lookup and insertion, so this class wouldn't be instantiated.
| void CacheFilter::UpstreamRequest::onHeaders(Http::ResponseHeaderMapPtr&& headers, | ||
| bool end_stream) { | ||
| if (filter_state_ == FilterState::ValidatingCachedResponse && isResponseNotModified(*headers)) { | ||
| return processSuccessfulValidation(std::move(headers)); |
There was a problem hiding this comment.
When we get a 304 response while validating an entry, I'm unclear on what the sequence of callbacks will be. We seem to be expecting that we'll get onHeaders then (because we abort()) onReset, and that we won't get onComplete. Is that right? What guarantees do we have?
There was a problem hiding this comment.
I asked about this too, and was told the AsyncHttpClient guarantees that no other callbacks are called after reset(). Though hm, I'm not certain if that means onReset doesn't get called either, we may need to call it ourselves. Maybe I should validate this behavior in an integration test.
There was a problem hiding this comment.
Turns out there is already an integration test that covers this path, and I think it would complain in one of the *san builds if a memory leak was provoked, which would be the effect if onReset didn't get called, so we have a guarantee there. (I also, to be on the safe side, tested in the integration test with some logging to make sure it was behaving as expected.)
| lookup_(std::move(filter->lookup_)), is_head_request_(filter->is_head_request_), | ||
| request_allows_inserts_(filter->request_allows_inserts_), config_(filter->config_), | ||
| filter_state_(filter->filter_state_), cache_(std::move(cache)), | ||
| stream_(async_client.start(*this, options)) { |
There was a problem hiding this comment.
Does async_client.start guarantee that all callbacks will happen on our thread?
There was a problem hiding this comment.
Yes, the async clients are inherently thread-local.
| } | ||
| if (insert_queue_) { |
There was a problem hiding this comment.
Does insert_queue_ guarantee that all callbacks happen on this thread?
There was a problem hiding this comment.
Kind of, that's why everything has to be dispatcher-posted. :)
|
/wait |
Signed-off-by: Raven Black <ravenblack@dropbox.com>
Signed-off-by: Raven Black <ravenblack@dropbox.com>
Signed-off-by: Raven Black <ravenblack@dropbox.com>
Signed-off-by: Raven Black <ravenblack@dropbox.com>
Signed-off-by: Raven Black <ravenblack@dropbox.com>
Signed-off-by: Raven Black <ravenblack@dropbox.com>
Signed-off-by: Raven Black <ravenblack@dropbox.com>
Commit Message: [cache_filter] Separate upstream from downstream
Additional Description: A recurring issue with cache_filter is that because it uses the regular upstream from a client's connection to populate the cache, a client being slow to download, or disconnecting mid-stream, can cause the cache-populating upstream to be delayed by watermarks or aborted. If the cache were to attempt to stream the results while they're still being populated, this can lead to many clients all receiving an unexpected disconnect or an unnecessarily slow response.
This PR makes the cache filter use a separate upstream made with AsyncHttpClient, so even if the client disconnects the upstream can persist and continue to populate the cache entry.
There are two flaws in this version.
Subsequent PRs are intended to address these:
Risk Level: Low, filter is WIP and previously bug-ridden and crash-prone. Integration test passes with no changes after this change.
Testing: Mostly covered by existing tests - one test changed from "expect abort" to "expect cache write completion", as is the intent of this PR.
Docs Changes: n/a
Release Notes: n/a
Platform Specific Features: n/a