[Patch] proxy cache for 304 Not Modified

MagicBear magicbearmo at gmail.com
Fri Sep 16 20:34:39 UTC 2011


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.

>
> >
> > # 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.
> >
> > diff -ruN a/http/modules/ngx_http_proxy_module.c
> > b/http/modules/ngx_http_proxy_module.c
> > --- a/http/modules/ngx_http_proxy_module.c      2011-09-15
> > 22:23:03.284431407 +0800
> > +++ b/http/modules/ngx_http_proxy_module.c      2011-09-16
> > 01:41:44.654428632 +0800
> > @@ -543,7 +543,7 @@
> >      { 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 -ruN a/http/ngx_http_upstream.c b/http/ngx_http_upstream.c
> > --- a/http/ngx_http_upstream.c  2011-09-15 22:23:03.284431407 +0800
> > +++ b/http/ngx_http_upstream.c  2011-09-16 01:41:44.654428632 +0800
> > @@ -16,6 +16,8 @@
> >      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 @@
> >        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 }
> > @@ -1618,6 +1624,80 @@
> >              u->buffer.last = u->buffer.pos;
> >          }
> >
> > +#if (NGX_HTTP_CACHE)
> > +
>
> Not sure if it's appropriate place.  Probably
> ngx_http_upstream_test_next() would be better, near
> cache_use_stale processing.

I have move it to inside function.

>
> BTW, please use "-p" switch in diff, it emits function names and
> thus makes review easier.

This patch I have open this switch~

>
> > +        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?

> (There are also multiple style issues in the patch.  I have to
> manually reformat it to make code readable for review.  Please
> make sure to follow nginx coding style with further postings.)
>

I am sorry for that, I using notepad++ to edit the code, but I didn't
notice that notepad++ default using \t to format, and nginx using 4
space, because they seem to be same at notepad++ and console.

> > +            ngx_int_t  rc;
> > +
> > +            rc = u->reinit_request(r);
> > +
> > +            if (rc == NGX_OK) {
>
> You may want to invert this check to make code more readable.
>
> > +                u->cache_status = NGX_HTTP_CACHE_BYPASS;
>
> Setting cache_status to NGX_HTTP_CACHE_BYPASS looks misleading.
> Probably some other status should be introduced for this.
>
> > +                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.
>
> > +                                       valid =
> > ngx_http_file_cache_valid(u->conf->cache_valid,
> > +
> >                               u->headers_in.status_n);
> > +                                       if (valid) {
> > +                                               r->cache->valid_sec = now +
> > valid;
> > +                                       }
> > +                               }
> > +
> > +                               if (valid) {
> > +                                       r->cache->last_modified =
> > r->headers_out.last_modified_time;
> > +                                       r->cache->date = now;
> > +                                       r->cache->body_start = (u_short)
> > (u->buffer.pos - u->buffer.start);
> > +
> > +                                       // update Header
> > +                                       ngx_http_file_cache_set_header(r,
> > u->buffer.start);
> > +
> > +                                       ngx_log_debug1(NGX_LOG_DEBUG_HTTP,
> > r->connection->log, 0,
> > +
> >  "update cache \"%s\" header to new expired." , r->cache->file.name.data);
> > +
> > +                                       // Reopen file via RW
> > +                                       ngx_fd_t fd =
> > ngx_open_file(r->cache->file.name.data, NGX_FILE_RDWR, NGX_FILE_OPEN, 0);
> > +
> > +                                       if (fd == NGX_INVALID_FILE) {
> > +                                               ngx_log_error(NGX_LOG_CRIT,
> > r->connection->log, ngx_errno,
> > +
> > ngx_open_file_n " \"%s\" failed", r->cache->file.name.data);
> > +                                               return;
> > +                                       }
> > +
> > +                                       // Write cache
> > +                                       if (write(fd, u->buffer.start,
> > sizeof(ngx_http_file_cache_header_t)) < 0)
> > +                                       {
> > +                                               ngx_log_error(NGX_LOG_CRIT,
> > r->connection->log, ngx_errno,
> > +
> > "write proxy_cache \"%s\" failed", r->cache->file.name.data);
> > +                                               return;
> > +                                       }
> > +
> > +                                       if (ngx_close_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);
> > +                                       }
> > +                                       ngx_log_debug1(NGX_LOG_DEBUG_HTTP,
> > r->connection->log, 0,
> > +
> >  "update cache \"%s\" header to new expired done." ,
> > r->cache->file.name.data);
> > +                               } else {
> > +                                       u->cacheable = 0;
> > +                                       r->headers_out.last_modified_time =
> > -1;
> > +                               }
>
> All this logic to update valid_sec in cache file should be
> abstracted into a function in ngx_http_file_cache.c.
>
> It probably should just write valid_sec and nothing more.
>

I have add a function ngx_http_file_cache_set_valid at
ngx_http_file_cache.c now, it will only update valid_sec field.

> > +            }
> > +
> > +            ngx_http_upstream_finalize_request(r, u, rc);
> > +            return;
> > +        }
> > +
> > +#endif
> > +
> >          if (ngx_http_upstream_test_next(r, u) == NGX_OK) {
> >              return;
> >          }
> > @@ -4006,6 +4086,32 @@
> >
> >      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)
> > +{
> > +    u_char *u;
>
> Please don't call character pointers as "u".  This letter is
> generally used for r->upstream pointer.  Better to name it "p".
>

I have changed this to p, thx for your advice.

> > +
> > +    if (r->upstream == NULL || r->upstream->cache_status == 0 ||
> > r->cache==NULL || r->cache->last_modified <= 0) {
>
> There is no need to check r->cache if cache_status != 0.
>
> I also believe that r->cache->last_modified == 0 is valid, though
> not sure if it may appear to be 0 unintentionally.
>
> > +        v->not_found = 1;
> > +        return NGX_OK;
> > +    }
> > +
> > +    v->valid = 1;
> > +    v->no_cacheable = 0;
> > +    v->not_found = 0;
> > +       u = ngx_pcalloc(r->pool, 30);
>
> There is no need to use 30 here, 29 is enough...
>
> > +    if (u == NULL) {
> > +        return NGX_ERROR;
> > +    }
> > +
> > +    v->len = 29;
> > +       ngx_http_time(u, r->cache->last_modified);
> > +    v->data = u;
>
> ...and please use sizeof("Mon, 28 Sep 1970 06:00:00 GMT") - 1 instead.
>
> Generic pattern may be found in ngx_http_variable_sent_last_modified():
>
>    p = ngx_pnalloc(r->pool,
>                    sizeof("Last-Modified: Mon, 28 Sep 1970 06:00:00 GMT") - 1);
>    if (p == NULL) {
>        return NGX_ERROR;
>    }
>
>    v->len = ngx_http_time(p, r->headers_out.last_modified_time) - p;
>    v->valid = 1;
>    v->no_cacheable = 0;
>    v->not_found = 0;
>    v->data = p;
>

And I have changed the code to this.

> > +
> > +    return NGX_OK;
> > +}
> >
> >  #endif
> >
> >
> > MagicBear
>
>
> Maxim Dounin
>
> _______________________________________________
> nginx-devel mailing list
> nginx-devel at nginx.org
> http://mailman.nginx.org/mailman/listinfo/nginx-devel


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


# 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)
+{
+    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);
+        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)
+    {
+        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);
+    }
+}
+

 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)))
+    {
+        ngx_int_t  rc;
+
+        rc = u->reinit_request(r);
+
+        if (rc == NGX_OK) {
+            u->cache_status = NGX_HTTP_CACHE_UPDATING;
+            rc = ngx_http_upstream_cache_send(r, u);
+
+            r->cache->valid_sec = ngx_time() + valid;
+            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;

     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)
+{
+    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



MagicBear



More information about the nginx-devel mailing list