# HG changeset patch
# User Thorvaldur Thorvaldsson <thorvaldur.thorvalds...@gmail.com>
# Date 1450190015 -3600
# Tue Dec 15 15:33:35 2015 +0100
# Node ID b0d5d1f8fb0822973bf160934fcf40c3b5e87f02
# Parent def9c9c9ae05cfa7467b0ec96e76afa180c23dfb
Upstream: Cache stale responses if they may be revalidated.
Previously, the proxy cache would never store stale responses, e.g.,
when the "Cache-Control" header contained "max-age=0", even if the
"proxy_cache_revalidate" directive was "on" and the response included
both an "ETag" and a "Last-Modified" header. This came as a surprise.
Now, a header like "Cache-Control: max-age=0, must-revalidate" can be
used along with an ETag/Last-Modified header to make nginx cache
responses that always require revalidation, e.g., when authorization is
required (and cheap).
diff -r def9c9c9ae05 -r b0d5d1f8fb08 src/http/ngx_http_file_cache.c
--- a/src/http/ngx_http_file_cache.c Sat Dec 12 10:32:58 2015 +0300
+++ b/src/http/ngx_http_file_cache.c Tue Dec 15 15:33:35 2015 +0100
@@ -628,7 +628,7 @@
now = ngx_time();
- if (c->valid_sec < now) {
+ if (c->valid_sec <= now) {
ngx_shmtx_lock(&cache->shpool->mutex);
@@ -831,7 +831,7 @@
if (fcn->error) {
- if (fcn->valid_sec < ngx_time()) {
+ if (fcn->valid_sec <= ngx_time()) {
goto renew;
}
diff -r def9c9c9ae05 -r b0d5d1f8fb08 src/http/ngx_http_upstream.c
--- a/src/http/ngx_http_upstream.c Sat Dec 12 10:32:58 2015 +0300
+++ b/src/http/ngx_http_upstream.c Tue Dec 15 15:33:35 2015 +0100
@@ -2815,11 +2815,16 @@
valid = ngx_http_file_cache_valid(u->conf->cache_valid,
u->headers_in.status_n);
if (valid) {
- r->cache->valid_sec = now + valid;
+ valid += now;
+ r->cache->valid_sec = valid;
}
}
- if (valid) {
+ if (valid > now
+ || (valid && r->upstream->conf->cache_revalidate
+ && (u->headers_in.etag
+ || u->headers_in.last_modified_time != -1)))
+ {
r->cache->date = now;
r->cache->body_start = (u_short) (u->buffer.pos - u->buffer.start);
@@ -4272,11 +4277,6 @@
return NGX_OK;
}
- if (n == 0) {
- u->cacheable = 0;
- return NGX_OK;
- }
-
r->cache->valid_sec = ngx_time() + n;
}
#endif
@@ -4312,7 +4312,7 @@
expires = ngx_parse_http_time(h->value.data, h->value.len);
- if (expires == NGX_ERROR || expires < ngx_time()) {
+ if (expires == NGX_ERROR) {
u->cacheable = 0;
return NGX_OK;
}
@@ -4354,15 +4354,9 @@
if (p[0] != '@') {
n = ngx_atoi(p, len);
- switch (n) {
- case 0:
- u->cacheable = 0;
- /* fall through */
-
- case NGX_ERROR:
+ if (n == NGX_ERROR) {
return NGX_OK;
-
- default:
+ } else {
r->cache->valid_sec = ngx_time() + n;
return NGX_OK;
}
On Wed, Dec 16, 2015 at 5:41 PM, Thorvaldur Thorvaldsson
<thorvaldur.thorvalds...@gmail.com> wrote:
> Hi,
>
> Thanks again for your time, Maxim. And you're right,
> I was assuming max-age=0 in all its forms to be equivalent
> to no such header at all. I have a simple fix which I'll
> post in a moment and then go ahead with adding a
> relevant test case. Just let me know if I'm completely
> on a wrong track here.
>
> Best regards,
> Thorvaldur
>
> On Wed, Dec 16, 2015 at 4:52 PM, Maxim Dounin <mdou...@mdounin.ru> wrote:
>> Hello!
>>
>> On Wed, Dec 16, 2015 at 03:37:27PM +0100, Thorvaldur Thorvaldsson wrote:
>>
>>> # HG changeset patch
>>> # User Thorvaldur Thorvaldsson <thorvaldur.thorvalds...@gmail.com>
>>> # Date 1450190015 -3600
>>> # Tue Dec 15 15:33:35 2015 +0100
>>> # Node ID 2c1f00c7f857c12587f0ac47323f04c6a881843a
>>> # Parent def9c9c9ae05cfa7467b0ec96e76afa180c23dfb
>>> Upstream: Cache stale responses if they may be revalidated.
>>>
>>> Previously, the proxy cache would never store stale responses, e.g.,
>>> when the "Cache-Control" header contained "max-age=0", even if the
>>> "proxy_cache_revalidate" directive was "on" and the response included
>>> both an "ETag" and a "Last-Modified" header. This came as a surprise.
>>>
>>> Now, a header like "Cache-Control: max-age=0, must-revalidate" can be
>>> used along with an ETag/Last-Modified header to make nginx cache
>>> responses that always require revalidation, e.g., when authorization is
>>> required (and cheap).
>>
>> [...]
>>
>>> diff -r def9c9c9ae05 -r 2c1f00c7f857 src/http/ngx_http_upstream.c
>>> --- a/src/http/ngx_http_upstream.c Sat Dec 12 10:32:58 2015 +0300
>>> +++ b/src/http/ngx_http_upstream.c Tue Dec 15 15:33:35 2015 +0100
>>> @@ -2815,11 +2815,16 @@
>>> valid = ngx_http_file_cache_valid(u->conf->cache_valid,
>>> u->headers_in.status_n);
>>> if (valid) {
>>> - r->cache->valid_sec = now + valid;
>>> + valid += now;
>>> + r->cache->valid_sec = valid;
>>> }
>>> }
>>>
>>> - if (valid) {
>>> + if (valid > now
>>> + || (r->upstream->conf->cache_revalidate
>>> + && (u->headers_in.etag
>>> + || u->headers_in.last_modified_time != -1)))
>>> + {
>>> r->cache->date = now;
>>> r->cache->body_start = (u_short) (u->buffer.pos -
>>> u->buffer.start);
>>
>> As far as I see, this still allows caching of all
>> responses, even ones without Cache-Control/Expires/X-Accel-Expires
>> and proxy_cache_valid configured. This is not something that
>> should happen because of revalidation enabled.
>>
>> You may want to rethink how the patch is expected to work.
>>
>> [...]
>>
>> --
>> Maxim Dounin
>> http://nginx.org/
>>
>> _______________________________________________
>> nginx-devel mailing list
>> nginx-devel@nginx.org
>> http://mailman.nginx.org/mailman/listinfo/nginx-devel
_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel
