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. :)

>
> # 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.

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

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

(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.)

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

> + }
> +
> + 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".

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

> +
> + return NGX_OK;
> +}
>
> #endif
>
>
> MagicBear


Maxim Dounin

_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel