[Patch] proxy cache for 304 Not Modified
Maxim Dounin
mdounin at mdounin.ru
Wed Sep 21 18:00:47 UTC 2011
Hello!
On Tue, Sep 20, 2011 at 12:47:56AM +0800, MagicBear wrote:
> 2011/9/19 Maxim Dounin <mdounin at mdounin.ru>:
> > 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.
> >
>
> I has modified to check 200 code, at before I have think user can
> custom 304 recheck time, but I think that's not need now.
I don't think that using validity time from 200 is correct either.
What happens if response wasn't 200 (e.g. 206)? What happens if
response was cached up to time specified in Cache-Control
header?
You may want to actually use ngx_http_file_cache_valid() with
original response status code (i.e. after
ngx_http_upstream_cache_send()), and only if valid_sec wasn't set
by original response header parsing.
This may also require resetting valid_sec to 0 before
ngx_http_file_cache_valid() (not sure, needs checking).
But see below, I still want to see rationale based on RFC2616.
[...]
> >> + 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 ",".
There are still multiple places where spaces aren't used where
they should be. In particular, exactly this line in new patch
still have no space 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).
> >
>
> This function is still considering, I was unknow how to make open file
> cache to writable.
As I already outlined above, this will require open file cache
interface changes.
[...]
> >> + 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.
I don't see any answers here.
> 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);
This is still 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)
And this.
Addtionally, space is missing after "," in offsetof().
You may also want to line up continuation lines with arguments in
first line. Also to improve readability it makes sense to use
separate variable for return code.
Resulting code will look like this:
rc = ngx_write_file(&file, (u_char *) &r->cache->valid_sec,
sizeof(r->cache->valid_sec),
offsetof(ngx_http_file_cache_header_t, valid_sec));
if (rc == NGX_ERROR) {
...
}
> + {
> + ngx_log_error(NGX_LOG_EMERG, r->connection->log, ngx_errno,
> + "write proxy_cache \"%s\" failed",
> r->cache->file.name.data);
Again, more than 80 chars.
> + 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);
And here.
> + }
> +}
> +
> +
> +void
> ngx_http_file_cache_update(ngx_http_request_t *r, ngx_temp_file_t *tf)
> {
> off_t fs_size;
> 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-20
> 00:39:39.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 }
> @@ -1682,6 +1688,34 @@ ngx_http_upstream_test_next(ngx_http_req
>
> status = u->headers_in.status_n;
>
> +#if (NGX_HTTP_CACHE)
> + time_t valid;
> +
> + if (u->cache_status == NGX_HTTP_CACHE_EXPIRED &&
> + status == NGX_HTTP_NOT_MODIFIED &&
> + 0!=(valid=ngx_http_file_cache_valid(u->conf->cache_valid,
> NGX_HTTP_OK)))
> + {
> + ngx_int_t rc;
> +
> + rc = u->reinit_request(r);
> +
> + if (rc == NGX_OK) {
> + u->cache_status = NGX_HTTP_CACHE_EXPIRED;
Setting cache_status to NGX_HTTP_CACHE_EXPIRED useless, it's
already set.
> + rc = ngx_http_upstream_cache_send(r, u);
> +
> + if (r->cache->valid_sec == 0) {
> + r->cache->valid_sec = ngx_time() + valid;
> + }
See above.
> +
> + ngx_http_file_cache_set_valid(r);
> + }
> +
> + ngx_http_upstream_finalize_request(r, u, rc);
> + return NGX_OK;
> + }
> +
> +#endif
> +
> for (un = ngx_http_upstream_next_errors; un->status; un++) {
>
> if (status != un->status) {
> @@ -4006,6 +4040,33 @@ 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);
Please line up "sizeof" with "r->pool". Code in
ngx_http_variable_sent_last_modified() uses much longer lines and
deviates from this rule to preserve line length under 80 chars.
> + 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
Additional question to consider:
What happens with the cache file created by the If-Modified-Since
request? It should be freed properly, and I don't immediately see
where it happens in the patch.
Maxim Dounin
More information about the nginx-devel
mailing list