On Tue, 26 Apr 2011 21:31:41 -0600, Alex Rousskov wrote:
On 04/26/2011 05:23 PM, Amos Jeffries wrote:
On Tue, 26 Apr 2011 10:18:50 -0600, Alex Rousskov wrote:
Hello,
Can somebody double check the following code, please?
/*
* At this point the response is stale, unless one of
* the override options kicks in.
* NOTE: max-stale config blocks the overrides.
*/
int max_stale = (R->max_stale >= 0 ? R->max_stale :
Config.maxStale);
if ( max_stale >= 0 && staleness < max_stale) {
debugs(22, 3, "refreshCheck: YES: max-stale limit");
if (request)
request->flags.fail_on_validation_err = 1;
return STALE_MAX_STALE;
}
It makes no sense to me, and I see 60-second stale objects being
declared STALE_MAX_STALE with the default configuration where
max-stale
is 1 week. The code declares the object stale if the object
"staleness"
is _less_ than the configured maximum. I would expect the opposite!
It is not clear from the squid.conf documentation what the exact
intent
of the max-stale option is with the relation to other checks such
as
refresh_pattern.max, but at the minimum, should not the comparison
be
reversed?
Yes. This appears to be wrong. Frankly I'm a little confused as to
how
it got that way. I did test this rather thoroughly since it was a
tricky
directive when I started the portage.
"max_stale" directive is a global default. "refresh_pattern
max-stale=N"
is a specific override of the global. Cache-Control weighs in
occasionally *if* it is smaller than either of the config values.
All of them are "up to" limits in accordance with RFC 2616. Where
the
client MAY receive a stale object UP TO an unspecified configured
limit.
(Thus we can avoid violations and Cache-Control weighing in if the
local
admin wants shorter staleness).
IMO, the priority of these limits is not clear from the
documentation.
We have, at least:
1. global max_stale
2. refresh_pattern max-stale
3. Cache-Control: max-stale
It is clear that the refresh_pattern max-stale overwrites global
max-stale, but other possible combinations are not documented. Your
"minimum value wins" explanation above should be morphed into
squid.conf
comments, I guess, but it does not match the code either, so there is
more work to do than just updating the docs.
Also, there are at least two different questions we must answer here:
A) What happens when the staleness does not exceed the winning
max-stale
value? Your answer above is, essentially, the object is considered
FRESH
in that case (unless the checks before the above code overwrite
that?).
B) What happens when the staleness exceeds than the winning max-stale
value? It is not clear whether the object must be considered stale
(i.e., the winning max-stale overwrites all other checks below it) OR
the object may still be considered fresh if some other check below it
says so.
By the time these tests are runs the object is already declared stale.
It is just a matter of by how much and whether the stale object may be
returned.
They all have no effect on 1xx-4xx server responses or fresh HITs.
Only
on 5xx when revalidating a stale HIT.
That is possible, but in the particular case that triggered this
thread,
the code in question was the reported reason for why the response was
considered stale beyond repair and, hence, not cachable:
refreshCheck: Matched '. 0 50%% 604800'
age: 60
check_time: Tue, 26 Apr 2011 08:44:08 GMT
entry->timestamp: Tue, 26 Apr 2011 08:43:08 GMT
STALE: age 60 >= min 0
Staleness = 60
refreshCheck: YES: max-stale limit
refreshIsCachable() returned non-cacheable..
StoreEntry::expireNow: '8D46C584D5269CF1B213D425C8F8944F'
StoreEntry::setReleaseFlag: '8D46C584D5269CF1B213D425C8F8944F'
It is possible that bugs elsewhere led to that, of course, but on the
surface of things, it looks like the refreshCheck code was
responsible
for 0% hit ratio in that specific environment (I know that we do get
hits under different conditions where incoming responses are not
stale).
Interesting, okay. So the incoming object was already too much "stale"
according to the broken logic (<). Fixing that operator is the right
fix. You want to do the honours?
Amos
