caching: Support out-of-thread (async) HttpCache implementations

[yosrym93]

Commit Message:
Added support to out-of-thread (async) implementations of HttpCache.
Signed-off-by: Yosry Ahmed yosryahmed@google.com

Additional Description:

• Cache callbacks are now posted to the dispatcher to make sure they are run on the worker thread.
• Simplified the CacheFilter state management and tests as all caches are now treated in the same way.

Risk Level: Low
Testing: Updated existing tests
Docs Changes: N/A
Release Notes: N/A

[toddmgreer]

In addition to calling onDestroy on lookup_ and insert_, we also need to remember that our onDestroy_ has been called. They might have posted callbacks that haven't happened yet; when we get them, we need to ignore them. I suggest adding a new FilterState for this. (Another approach would be to hand these pointers to dispatcher().deferredDelete, but making it a state seems cleaner.)

[toddmgreer]

@mattklein123 This PR relies on details of event handling that aren't documented, but that I think are intended. Specifically, it assumes that all callbacks posted before the return of CacheFilter::onDestroy will be executed before this CacheFilter is deleted. (If this isn't intended, then I don't understand the purpose of deferred deletion.) Can you confirm? (Either way, we should update the docs.)

[mattklein123]

I haven't tracked this PR, but no, this is not guaranteed. Can you describe a bit more what is going on or do you want me to review the entire PR?

[toddmgreer]

Most of CacheFilter's plugin cache implementations will complete requests asynchronously, and there's no guarantee of what thread those completions will run on, so their callbacks get posted to our dispatcher. Suppose a plugin posts a callback, then (perhaps due to a client disconnect) CacheFilter::onDestroy is called. That callback must happen before the CacheFilter gets deleted (or else it will access a deleted CacheFilter). AFAICT, the current code does give us the correct order (because it runs pending callbacks before it runs deferred deletion), and I'll be surprised if it ever needs to change, but we need to make sure.

I think that all we need to do is add documentation and tests to lock in the current behavior of running pending callbacks before deferred deletion.

If we can't rely on this, we'll have to go back to a prior iteration of the original CacheFilter PR, and use either weak_ptrs or shared_ptrs to the CacheFilter, but that complicates the code and adds bus-locked operations.

[mattklein123]

The solution here is to not use post, but use a "timer" which can be cancelled. Can we switch to that?

[yosrym93]

@jmarantz the problem arises if the cache lookup has already finished and posted the callback to the dispatcher, but then the client closes the connection and the filter chain is destroyed. Currently, there's no guarantee that the posted callback will run before the filter is deleted. If the filter is deleted first, the posted callback will run on a destroyed filter.
One solution here is to capture a weak_ptr to the CacheFilter in the posted callback, and check that the CacheFilter is still alive before accessing any of its members/methods.

[jmarantz]

Can we arrange a destroy-notify callback from the filter chain? The cache system could then mark the filter structure as stale and avoid referencing it further.

[jmarantz]

of course weak_ptr does the job too, if the thing being pointed to is already a shared_ptr.

[jmarantz]

one more thing: the advantage of a callback over a weak_ptr is that you might be able to actively cancel an outstanding lookup request (if that is possible).

[mattklein123]

@mattklein123 I am following the discussion at #12364. Can you elaborate how is it related to our problem here?

It's loosely related in the sense that we have TLS post/callbacks that are being running beyond the acceptable lifetime.

one more thing: the advantage of a callback over a weak_ptr is that you might be able to actively cancel an outstanding lookup request (if that is possible).

This is how a cancellable post would be accomplished within dispatcher. This is what I recommend if you want a core solution, otherwise you can implement this yourself.

[mattklein123]

I assigned myself so can help review the entire PR once we sort out the implementation plan.

/wait

[toddmgreer]

Calling onDestroy on lookup_ and insert_ takes care of the cancellation callback. Holding a weak_ptr<CacheFilter> in the callback lambda takes care of CacheFilter getting deleted before the post happens. (See PR #7198 for an earlier state of this approach.)

…

On Tue, Aug 18, 2020 at 1:49 PM Matt Klein ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In source/extensions/filters/http/cache/cache_filter.cc <#12622 (comment)>: > @@ -32,8 +33,12 @@ CacheFilter::CacheFilter(const envoy::extensions::filters::http::cache::v3alpha: : time_source_(time_source), cache_(http_cache) {} void CacheFilter::onDestroy() { - lookup_ = nullptr; - insert_ = nullptr; + if (lookup_) { @mattklein123 <https://github.com/mattklein123> I am following the discussion at #12364 <#12364>. Can you elaborate how is it related to our problem here? It's loosely related in the sense that we have TLS post/callbacks that are being running beyond the acceptable lifetime. one more thing: the advantage of a callback over a weak_ptr is that you might be able to actively cancel an outstanding lookup request (if that is possible). This is how a cancellable post would be accomplished within dispatcher. This is what I recommend if you want a core solution, otherwise you can implement this yourself. — You are receiving this because you were assigned. Reply to this email directly, view it on GitHub <#12622 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AFRAWPPRY7CMV7FFNGKPGF3SBLSLXANCNFSM4P5NVFGQ> .

[yosrym93]

I added safeguard against the "callback posted then filter deleted" case we have been discussing by using a weak_ptr. I tried to make it as simple as possible. I added a TODO to look into other solutions as they arise (guaranteed dispatcher ordering of posts and deletions, cancellable posts, etc.).
I also added a test for this that fails if the filter is accessed after being deleted when run with the ASAN sanitizer (otherwise the behavior is naturally undefined - it may give false positives).
Looking forward for everyone to take a look!

[mattklein123]

Thanks generally LGTM but a few small bugs/questions. Thank you!

/wait

[yosrym93]

@mattklein123 I fixed the test. Turns out the lookup in the test did not find any cache entries so headers was an empty unique_ptr, that's why there were no leaks. I fixed the test and now it fails when the pointer is wrapped inside the if condition.

[mattklein123]

@mattklein123 I fixed the test. Turns out the lookup in the test did not find any cache entries so headers was an empty unique_ptr, that's why there were no leaks. I fixed the test and now it fails when the pointer is wrapped inside the if condition.

Great!

[mattklein123]

Thanks!