Hello Maxim!
2011/9/16 Maxim Dounin <mdounin@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@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@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@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
_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel
