2011/9/19 Maxim Dounin <mdounin@mdounin.ru>:
> 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.
>
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.
> [...]
>
>> > > + 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.
>
I am sorry, I have forgot to check this value may set by other function.
> [...]
>
>> 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.
>
Changed
>> + 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).
>
This function is still considering, I was unknow how to make open file
cache to writable.
>>
>> 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
Here is the new patch: http://m-b.cc/share/patch-nginx-proxy-304-1.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.
Testing what happens if cache file was updated/removed/whatever while
we were talking to backend.
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-20
00:39:29.000000000 +0800
@@ -767,6 +767,41 @@ ngx_http_file_cache_set_header(ngx_http_
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)
{
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;
+ rc = ngx_http_upstream_cache_send(r, u);
+
+ if (r->cache->valid_sec == 0) {
+ 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
+
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);
+ 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
_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel
