Hello!
On Sat, Sep 17, 2011 at 04:34:39AM +0800, MagicBear wrote:
> 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.
[...]
> > > + 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@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
_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel
