[Patch] proxy cache for 304 Not Modified

Maxim Dounin mdounin at mdounin.ru
Mon Sep 19 10:47:36 UTC 2011


Hello!

On Sat, Sep 17, 2011 at 04:34:39AM +0800, MagicBear wrote:

> Hello Maxim!
> 
> 2011/9/16 Maxim Dounin <mdounin at mdounin.ru>
> >
> > Hello!
> >
> > On Fri, Sep 16, 2011 at 01:54:04AM +0800, ビリビリⅤ wrote:
> >
> > > Hello guys,
> > > I have wrote a module to make nginx support 304 to decrease bandwidth usage.
> > > note: I have a newbie for nginx module development, so the above module may
> > > have some problem. Welcome to test it and feedback another problem with me.
> > > Note:
> > > I cannot found where can using nginx internal cache update without delete
> > > the old cache, so I using reopen the cache file to write a new expire
> > > header. Can anyone help me to fix this problem?
> > >
> > > You can download full patch file from here:
> > > http://m-b.cc/share/proxy_304.txt
> >
> > See review below.
> >
> > This is definitely step in right direction, re-checking cache
> > items should be supported.  Though I can't say I'm happy with the
> > patch.  Hope it will be improved. :)
> 
> Thanks for your advice.

[...]

> > > +        if (u->cache_status == NGX_HTTP_CACHE_EXPIRED &&
> > > +                       u->headers_in.status_n == NGX_HTTP_NOT_MODIFIED &&
> > > +                       ngx_http_file_cache_valid(u->conf->cache_valid,
> > > u->headers_in.status_n))
> > > +        {
> >
> > The ngx_http_file_cache_valid() test seems to be incorrect (and
> > completely unneeded), as you are going to preserve reply which is
> > already in cache, not 304 reply.
> >
> 
> I review the code, this is completely unneeded, you are right, now I
> move get valid time to here, I think that may be more good?

No, the new code is still incorrect.  It still uses validity time 
for 304 reply, not the original reply as stored in cache.

Consider the following config:

    proxy_cache ...
    proxy_cache_valid 200 1h;
    proxy_cache_ignore_headers Expires Cache-Control;

We have 200 reply cached, tried to validate it with 
If-Modified-Since request and got 304.

The ngx_http_file_cache_valid() here will be for status_n == 304 
and will return 0, as there is no validity time set for 304 
replies.  But we are actually caching 200 reply, the one we 
originally had in cache, so we should use 1h here.

[...]

> > > +                rc = ngx_http_upstream_cache_send(r, u);
> > > +
> > > +                               time_t  now, valid;
> > > +
> > > +                               now = ngx_time();
> > > +
> > > +                               valid = r->cache->valid_sec;
> > > +
> > > +                               if (valid == 0) {
> >
> > How can r->cache->valid_sec be non-zero here?  As far as I
> > understand this should never happen.  Even if does happen, this is
> > unwise to trust it.

It looks like I was completely wrong here: r->cache->valid_sec is 
set by ngx_http_upstream_process_cache_control() and
ngx_http_upstream_process_expires().  This check should be 
preserved.

[...]

> Here is the new patch
> full file at: http://m-b.cc/share/patch-nginx-proxy-304.txt

Thank you, this one looks much better.  See below for more 
comments.

> _______________________________________________
> 
> 
> # User MagicBear <magicbearmo at gmail.com>
> Upstream:
> add $upstream_last_modified variant.
> add handler for 304 Unmodified.
> Proxy:
> change to send If-Modified-Since header.
> 
> TODO:
> change write TO not block IO.
> 
> diff -ruNp a/src/http/modules/ngx_http_proxy_module.c
> nginx-1.1.3/src/http/modules/ngx_http_proxy_module.c
> --- a/src/http/modules/ngx_http_proxy_module.c  2011-09-16
> 02:13:16.274428192 +0800
> +++ nginx-1.1.3/src/http/modules/ngx_http_proxy_module.c
> 2011-09-16 02:13:57.544428180 +0800
> @@ -543,7 +543,7 @@ static ngx_keyval_t  ngx_http_proxy_cach
>      { ngx_string("Connection"), ngx_string("close") },
>      { ngx_string("Keep-Alive"), ngx_string("") },
>      { ngx_string("Expect"), ngx_string("") },
> -    { ngx_string("If-Modified-Since"), ngx_string("") },
> +    { ngx_string("If-Modified-Since"), ngx_string("$upstream_last_modified") },
>      { ngx_string("If-Unmodified-Since"), ngx_string("") },
>      { ngx_string("If-None-Match"), ngx_string("") },
>      { ngx_string("If-Match"), ngx_string("") },
> diff -ruNp a/src/http/ngx_http_cache.h nginx-1.1.3/src/http/ngx_http_cache.h
> --- a/src/http/ngx_http_cache.h 2011-07-29 23:09:02.000000000 +0800
> +++ nginx-1.1.3/src/http/ngx_http_cache.h       2011-09-17
> 04:08:27.000000000 +0800
> @@ -133,6 +133,7 @@ ngx_int_t ngx_http_file_cache_create(ngx
>  void ngx_http_file_cache_create_key(ngx_http_request_t *r);
>  ngx_int_t ngx_http_file_cache_open(ngx_http_request_t *r);
>  void ngx_http_file_cache_set_header(ngx_http_request_t *r, u_char *buf);
> +void ngx_http_file_cache_set_valid(ngx_http_request_t *r);
>  void ngx_http_file_cache_update(ngx_http_request_t *r, ngx_temp_file_t *tf);
>  ngx_int_t ngx_http_cache_send(ngx_http_request_t *);
>  void ngx_http_file_cache_free(ngx_http_cache_t *c, ngx_temp_file_t *tf);
> diff -ruNp a/src/http/ngx_http_file_cache.c
> nginx-1.1.3/src/http/ngx_http_file_cache.c
> --- a/src/http/ngx_http_file_cache.c    2011-08-26 01:29:34.000000000 +0800
> +++ nginx-1.1.3/src/http/ngx_http_file_cache.c  2011-09-17
> 04:25:36.000000000 +0800
> @@ -765,6 +765,38 @@ ngx_http_file_cache_set_header(ngx_http_
>      *p = LF;
>  }
> 
> +void
> +ngx_http_file_cache_set_valid(ngx_http_request_t *r)

Two blank lines between functions, please.

> +{
> +    ngx_file_t  file;
> +
> +    ngx_memzero(&file, sizeof(ngx_file_t));
> +
> +    file.name = r->cache->file.name;
> +    file.log = r->connection->log;
> +
> +    file.fd = ngx_open_file(file.name.data, NGX_FILE_RDWR,
> +                            NGX_FILE_OPEN, NGX_FILE_DEFAULT_ACCESS);
> +
> +    if (file.fd == NGX_INVALID_FILE) {
> +        ngx_log_error(NGX_LOG_EMERG, r->connection->log, ngx_errno,
> +                      ngx_open_file_n " \"%s\" failed", r->cache->file.name.data);

Please wrap lines longer than 80 chars.

> +        return;
> +    }
> +
> +    if (ngx_write_file(&file, (u_char *)&r->cache->valid_sec,
> sizeof(r->cache->valid_sec),
> offsetof(ngx_http_file_cache_header_t,valid_sec)) == NGX_ERROR)

Same as above.  Please use spaces after "(u_char *)" cast and 
after ",".

> +    {
> +        ngx_log_error(NGX_LOG_EMERG, r->connection->log, ngx_errno,
> +                      "write proxy_cache \"%s\" failed",
> r->cache->file.name.data);
> +        return;
> +    }
> +
> +    if (ngx_close_file(file.fd) == NGX_FILE_ERROR) {
> +        ngx_log_error(NGX_LOG_ALERT, r->connection->log, ngx_errno,
> +                      ngx_close_file_n " \"%s\" failed",
> r->cache->file.name.data);
> +    }
> +}
> +

One more question to consider: what happens if cache file was
updated/removed/whatever while we were talking to backend.  
Reopening the file may not be safe in this case.

Probably solution would be to request write permissions on cache 
file on initial open, though a) this may cause problems e.g. on 
Windows (not sure, needs investigation) and b) requires open file cache 
changes as it isn't currently able to open files for writing.

On the other hand, just updating valid_sec may be safe in any case 
(not sure, needs investigation).  

> 
>  void
>  ngx_http_file_cache_update(ngx_http_request_t *r, ngx_temp_file_t *tf)
> diff -ruNp a/src/http/ngx_http_upstream.c
> nginx-1.1.3/src/http/ngx_http_upstream.c
> --- a/src/http/ngx_http_upstream.c      2011-09-16 02:13:16.274428192 +0800
> +++ nginx-1.1.3/src/http/ngx_http_upstream.c    2011-09-17
> 04:23:02.000000000 +0800
> @@ -16,6 +16,8 @@ static ngx_int_t ngx_http_upstream_cache
>      ngx_http_upstream_t *u);
>  static ngx_int_t ngx_http_upstream_cache_status(ngx_http_request_t *r,
>      ngx_http_variable_value_t *v, uintptr_t data);
> +static ngx_int_t ngx_http_upstream_last_modified(ngx_http_request_t *r,
> +    ngx_http_variable_value_t *v, uintptr_t data);
>  #endif
> 
>  static void ngx_http_upstream_init_request(ngx_http_request_t *r);
> @@ -342,6 +344,10 @@ static ngx_http_variable_t  ngx_http_ups
>        ngx_http_upstream_cache_status, 0,
>        NGX_HTTP_VAR_NOCACHEABLE, 0 },
> 
> +    { ngx_string("upstream_last_modified"), NULL,
> +      ngx_http_upstream_last_modified, 0,
> +      NGX_HTTP_VAR_NOCACHEABLE, 0 },
> +
>  #endif
> 
>      { ngx_null_string, NULL, NULL, 0, 0, 0 }
> @@ -1680,6 +1686,31 @@ ngx_http_upstream_test_next(ngx_http_req
>      ngx_uint_t                 status;
>      ngx_http_upstream_next_t  *un;
> 
> +#if (NGX_HTTP_CACHE)
> +    time_t  valid;
> +
> +    if (u->cache_status == NGX_HTTP_CACHE_EXPIRED &&
> +        u->headers_in.status_n == NGX_HTTP_NOT_MODIFIED &&
> +        0!=(valid=ngx_http_file_cache_valid(u->conf->cache_valid,
> u->headers_in.status_n)))

See above, this is incorrect.

> +    {
> +        ngx_int_t  rc;
> +
> +        rc = u->reinit_request(r);
> +
> +        if (rc == NGX_OK) {
> +            u->cache_status = NGX_HTTP_CACHE_UPDATING;

Why NGX_HTTP_CACHE_UPDATING?

> +            rc = ngx_http_upstream_cache_send(r, u);
> +
> +            r->cache->valid_sec = ngx_time() + valid;

And here should be r->cache->valid_sec test (see above, I was 
wrong in previous comment).  You shouldn't blindly trust me, 
especially when I'm asking questions.  :)

Additional question to consider: what should happen if original 
200 reply comes with "Cache-Control: max-age=<seconds>" (or 
"Expires: <time>").  Should we use it?  Or should we use another 
one from 304 reply?

Please provide rationale based on RFC2616.

> +            ngx_http_file_cache_set_valid(r);
> +        }
> +
> +        ngx_http_upstream_finalize_request(r, u, rc);
> +        return NGX_OK;
> +    }
> +
> +#endif
> +
>      status = u->headers_in.status_n;

You may want to move the code after this assignment, it should 
save some typing and improve readability.

> 
>      for (un = ngx_http_upstream_next_errors; un->status; un++) {
> @@ -4006,6 +4037,32 @@ ngx_http_upstream_cache_status(ngx_http_
> 
>      return NGX_OK;
>  }
> +
> +ngx_int_t
> +ngx_http_upstream_last_modified(ngx_http_request_t *r,
> +    ngx_http_variable_value_t *v, uintptr_t data)

Two blank lines between functions, please.

> +{
> +    u_char *p;
> +
> +    if (r->upstream == NULL || r->upstream->cache_status == 0) {
> +        v->not_found = 1;
> +        return NGX_OK;
> +    }
> +
> +    p = ngx_pcalloc(r->pool,
> +                sizeof("Mon, 28 Sep 1970 06:00:00 GMT") - 1);
> +    if (p == NULL) {
> +        return NGX_ERROR;
> +    }
> +
> +    v->len = ngx_http_time(p, r->cache->last_modified) - p;
> +    v->valid = 1;
> +    v->no_cacheable = 0;
> +    v->not_found = 0;
> +    v->data = p;
> +
> +    return NGX_OK;
> +}
> 
>  #endif

Maxim Dounin



More information about the nginx-devel mailing list