bneradt commented on a change in pull request #7401:
URL: https://github.com/apache/trafficserver/pull/7401#discussion_r547504018
##########
File path: proxy/http/HttpTransact.cc
##########
@@ -4464,7 +4464,7 @@
HttpTransact::handle_cache_operation_on_forward_server_response(State *s)
// before issuing a 304
if (s->cache_info.action == CACHE_DO_WRITE || s->cache_info.action ==
CACHE_DO_NO_ACTION ||
s->cache_info.action == CACHE_DO_REPLACE) {
- if (s->is_cacheable_and_negative_caching_is_enabled) {
+ if (s->is_cacheable_and_negative_caching_is_enabled &&
is_negative_caching_appropriate(s)) {
Review comment:
See my thoughts here:
https://github.com/apache/trafficserver/issues/7380#issuecomment-749740797
> Prior to the 8eb6826, when this function set
s->is_cacheable_and_negative_caching_is_enabled (renamed from s->
negative_caching), it called is_negative_caching_appropriate(s).
To add more detail to this: `is_negative_caching_appropriate()` was called
before and after 8eb6826. Before 8eb6826, `cacheable` meant something pretty
complicated and unintuitive if negative caching was enabled. If negative
caching was **disabled**, `cacheable` meant simply that, all things considered
(status code, cache-control header, etc.), the response looks cacheable. If
negative caching was **enabled**, however, `cacheable` meant that the response
looks cacheable in all respects other than response code considerations. This
had the intention that `s->negative_caching` would be inspected to see whether
it was cacheable with negative response status codes. The intention was
explicitly marked by this comment:
https://github.com/apache/trafficserver/blob/f3647619921ada5214b78d6ddb594a10fbc99a15/proxy/http/HttpTransact.cc#L6354
However, this made `cacheable` unintuitive for negative caching and this
resulted in the negative caching bug in which all negative responses, not just
the specified ones, got cached.
To fix the negative caching bug, I updated the `is_response_cacheable()`
function, from which `cacheable` is populated, to take into account negative
caching. Therefore `cacheable` now simply is true if the response looks
cacheable (with or without negative caching enabled, as the case may be), and
it is false otherwise. And `is_cacheable_and_negative_caching_is_enabled` is
set using the `cacheable` value.
Thus the problem isn't that `is_cacheable_and_negative_caching_is_enabled`
isn't set with `is_negative_caching_appropriate()` in consideration, the
problem is that it is logically OR'd with the other features of the response
which makes it cacheable. Thus `is_cacheable_and_negative_caching_is_enabled`
means literally what it's name indicates: it will be true if both negative
caching is enabled and the response is cacheable (either because it is some
otherwise cacheable response, like a 200, or if it is a negative response that
was marked for negative caching).
The solution, I believe, will be to do what @shinya1020 is doing in this
diff, but apply it more broadly to the value of
`is_cacheable_and_negative_caching_is_enabled`. Thus I suggest that we
initialize `is_cacheable_and_negative_caching_is_enabled` like so
(`s->txn_conf->negative_caching_enabled` is taken into account in
`is_negative_caching_appropriate()` so it doesn't need to be repeated on
initialization):
s->is_cacheable_and_negative_caching_is_enabled = cacheable &&
is_negative_caching_appropriate(s);
And, with this change, we should rename the variable. I suggest:
`is_cacheable_due_to_negative_caching_configuration`
This should resolve both the leak in #7380 and the issue of the Expires
header.
##########
File path: proxy/http/HttpTransact.cc
##########
@@ -4464,7 +4464,7 @@
HttpTransact::handle_cache_operation_on_forward_server_response(State *s)
// before issuing a 304
if (s->cache_info.action == CACHE_DO_WRITE || s->cache_info.action ==
CACHE_DO_NO_ACTION ||
s->cache_info.action == CACHE_DO_REPLACE) {
- if (s->is_cacheable_and_negative_caching_is_enabled) {
+ if (s->is_cacheable_and_negative_caching_is_enabled &&
is_negative_caching_appropriate(s)) {
Review comment:
In addition to renaming
`is_cacheable_due_to_negative_caching_configuration` we should update the
comment here:
https://github.com/apache/trafficserver/blob/8eb68266167d8f8b3fa3a00ca9f6b7889e8ec101/proxy/http/HttpTransact.h#L741-L756
##########
File path: proxy/http/HttpTransact.cc
##########
@@ -4464,7 +4464,7 @@
HttpTransact::handle_cache_operation_on_forward_server_response(State *s)
// before issuing a 304
if (s->cache_info.action == CACHE_DO_WRITE || s->cache_info.action ==
CACHE_DO_NO_ACTION ||
s->cache_info.action == CACHE_DO_REPLACE) {
- if (s->is_cacheable_and_negative_caching_is_enabled) {
+ if (s->is_cacheable_and_negative_caching_is_enabled &&
is_negative_caching_appropriate(s)) {
Review comment:
In case it's helpful, I did these suggested changes on the following
branch:
https://github.com/bneradt/trafficserver/tree/fix_negative_caching_leak
@masaori335 and @shinya1020 : can you please try things out with this branch
to see whether it addresses the header and leak issues? If so, we can update
this PR with that change. Or I can create a new PR if you prefer. Just let me
know.
Thanks,
Brian
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
