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).
They all have no effect on 1xx-4xx server responses or fresh HITs. Only
on 5xx when revalidating a stale HIT.
if (max_stale >= 0 && staleness > max_stale)
...
return STALE_...
Please note that since Config.maxStale defaults to one week, the
max_stale variable is not negative and the above check kicks in for
all
stale objects by default, no matter what refresh_pattern max is!
Again,
it is not clear to me whether that is intentional. If it is, we
should
document this surprising effect better.
The limit is min(config, Cache-Control: max-stale). The above if() is
supposed to be a shortcut for min().
"1 week" is an arbitrary value copied from squid-2. It could be up to a
year, but IMO having a server 5xx'ing for a whole week before anyone
notices is already a bad situation.
FWIW there was no limit on staleness prior to the portage. 3.0 and 3.1
will simply emit stale HITs *forever* or until the validation succeeds.
Another option would be to make the check specific to sf.max cases:
if (sf.max && max_stale >= 0 && staleness <= max_stale)
...
return FRESH_...
but that has problems of its own (why would we need max_stale
suboption
in refresh_pattern then?).
ie with the config:
max_stale 3600 second
refresh_pattern *.html 0 100% 4320 max-stale=6000
When cache-control *does not* get involved object /index.html should be
stale HIT for 6000 seconds. Whereas /image.jpg could be stale HIT for up
to 3600 seconds.
When cache-control *does* get involved things get a bit tricky and
dodgy. The idea is that min(config, Cache-Control) limit is used, but
not strictly since the refresh_pattern estimation may not be applied.
Amos
