[cache] Add completion callback to updateHeaders #23666
Conversation
Signed-off-by: Raven Black <ravenblack@dropbox.com>
Signed-off-by: Raven Black <ravenblack@dropbox.com>
Signed-off-by: Raven Black <ravenblack@dropbox.com>
"It is a programming error to call this twice." according to http_cache.h Signed-off-by: Raven Black <ravenblack@dropbox.com>
| @@ -252,9 +252,13 @@ class HttpCache { | |||
| // | |||
| // This is called when an expired cache entry is successfully validated, to | |||
| // update the cache entry. | |||
| // | |||
| // The on_complete callback should be called with true if the update is successful, | |||
There was a problem hiding this comment.
I've made this change, but I'm not entirely comfortable with it - this is an interface against which the person reading the documentation is likely implementing a cache instance (since there's no other reason to be looking at this particular code). "Is" sounds like something that already happens. I would favor "must be", as a description of what the implementer needs to make happen. But if you want "is", you can have "is". :)
There was a problem hiding this comment.
"must be" works for me!
But "should be" sounds too weak.
| cache()->updateHeaders(*lookup_context, response_headers, metadata); | ||
| auto update_promise = std::make_shared<std::promise<bool>>(); | ||
| cache()->updateHeaders(*lookup_context, response_headers, metadata, | ||
| [update_promise](bool result) { update_promise->set_value(result); }); |
There was a problem hiding this comment.
can we test the value of result after the promise is called?
There was a problem hiding this comment.
Made updateHeaders return the result, so it can be validated in either direction at the callsite, and added the corresponding validations (and also made the name of one of the tests more descriptive accordingly).
| EXPECT_NE(lookup_result.cache_entry_status_, CacheEntryStatus::Unusable); | ||
| EXPECT_NE(lookup_result.headers_, nullptr); | ||
| response_headers_ptr = move(lookup_result.headers_); | ||
| headers_promise->set_value(move(lookup_result.headers_)); |
There was a problem hiding this comment.
pre-existing, but where does move come from? should this be std::move?
Signed-off-by: Raven Black <ravenblack@dropbox.com>
Signed-off-by: Raven Black <ravenblack@dropbox.com>
|
/assign @jmarantz |
There was a problem hiding this comment.
Actually I'll go ahead and merge -- you can write a PR to change "is" to "must be" if you like, though there was a pre-existing parallel use of "is" in the paragraph above the description of updateHeaders() which is why I think I suggested that in the first place.
Commit Message: Add completion callback to updateHeaders in HttpCache
Additional Description: This is necessary to facilitate testing of asynchronous cache implementations.
Risk Level: Low - cache filter is still tagged WIP.
Testing: Covered by existing tests (and future tests that rely on this change.)
Docs Changes: n/a
Release Notes: May require any existing HttpCache implementations to update to match the new interface.
Platform Specific Features: n/a