Hello!

On Mon, Dec 03, 2018 at 12:11:15PM +0000, Elliot Thomas wrote:

> This patch applies cache locking behaviour to stale cache entries, so
> in the case where the *_cache_lock directives are used, the same
> locking behaviour is used for stale content as it is for new entries.
>
> Previously, this was only done for new cache entries.
> (see: http://mailman.nginx.org/pipermail/nginx/2016-January/049734.html)
>
> This is useful when serving stale content is not permissable but sending
> many requests upstream is undesriable.
>
>
> This patch exposes the ngx_http_file_cache_lock function; this function
> is then called in ngx_http_upstream if a cache entry has expired.
>
> I have attached two versions of this patch, a "minimal" one that avoids
> changes where not strictly necessary, and a "cleaner" version which is
> more invasive but (in my opinion) cleaner.
>
> Both patches cause no (additional) failures in the nginx test suite.
> I tested with as many modules as I could reasonably enable.

Thank you for the patches. The "cleaner" version looks better,
though it looks like it would be a good idea to clean it even
more. See comments below.

[...]

> -----------------------------
> http://www.bbc.co.uk
> This e-mail (and any attachments) is confidential and
> may contain personal views which are not the views of the BBC unless specifically stated.
> If you have received it in
> error, please delete it from your system.
> Do not use, copy or disclose the
> information in any way nor act in reliance on it and notify the sender
> immediately.
> Please note that the BBC monitors e-mails
> sent or received.
> Further communication will signify your consent to
> this.
> -----------------------------

Note that posting patches to a public mailing list with such a
disclaimer might not be a good idea. If you cannot remove it,
please make sure to add an explicit comment that you understand
that you are posting to a public mailing list, and you've read the
http://nginx.org/en/docs/contributing_changes.html article. In
particular, that you agree with the "License" part.

> # HG changeset patch
> # User Elliot Thomas <elliot.thomas@bbc.co.uk>
> # Date 1543836716 0
> # Mon Dec 03 11:31:56 2018 +0000
> # Node ID 28084fdcef2873df221b13d1a70ab04242e4edc5
> # Parent 2117637f64e981e0e14c3a4b0509252fefd8a78a
> Apply cache locking behaviour to stale cache entries.
>
> Previously, this was only done for new cache entries.
>
> diff -r 2117637f64e9 -r 28084fdcef28 src/http/ngx_http_cache.h
> --- a/src/http/ngx_http_cache.h Tue Nov 27 17:40:21 2018 +0300
> +++ b/src/http/ngx_http_cache.h Mon Dec 03 11:31:56 2018 +0000
> @@ -188,6 +188,7 @@
> ngx_int_t ngx_http_file_cache_create(ngx_http_request_t *r);
> 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);
> +ngx_int_t ngx_http_file_cache_lock(ngx_http_request_t *r, ngx_int_t rc);
> ngx_int_t ngx_http_file_cache_set_header(ngx_http_request_t *r, u_char *buf);
> void ngx_http_file_cache_update(ngx_http_request_t *r, ngx_temp_file_t *tf);
> void ngx_http_file_cache_update_header(ngx_http_request_t *r);
> diff -r 2117637f64e9 -r 28084fdcef28 src/http/ngx_http_file_cache.c
> --- a/src/http/ngx_http_file_cache.c Tue Nov 27 17:40:21 2018 +0300
> +++ b/src/http/ngx_http_file_cache.c Mon Dec 03 11:31:56 2018 +0000
> @@ -11,8 +11,6 @@
> #include <ngx_md5.h>
>
>
> -static ngx_int_t ngx_http_file_cache_lock(ngx_http_request_t *r,
> - ngx_http_cache_t *c);
> static void ngx_http_file_cache_lock_wait_handler(ngx_event_t *ev);
> static void ngx_http_file_cache_lock_wait(ngx_http_request_t *r,
> ngx_http_cache_t *c);
> @@ -391,22 +389,21 @@
>
> done:
>
> - if (rv == NGX_DECLINED) {
> - return ngx_http_file_cache_lock(r, c);
> - }
> -
> return rv;
> }

This changes makes the "done" label useless. It can be removed,
and corresponding gotos in the function replaced with explicit
returns (as it was before the introduction of cache locks in
revision 70ba81827472, http://hg.nginx.org/nginx/rev/70ba81827472).

> -static ngx_int_t
> -ngx_http_file_cache_lock(ngx_http_request_t *r, ngx_http_cache_t *c)
> +ngx_int_t
> +ngx_http_file_cache_lock(ngx_http_request_t *r, ngx_int_t rc)
> {
> ngx_msec_t now, timer;
> + ngx_http_cache_t *c;
> ngx_http_file_cache_t *cache;
>
> + c = r->cache;
> +
> if (!c->lock) {

Note that c->lock is not really needed now, as
u->conf->cache_lock can be used directly by the caller. It might
be a good idea to get rid of it (not sure though).

> - return NGX_DECLINED;
> + return rc;
> }
>
> now = ngx_current_msec;
> @@ -431,7 +428,7 @@
> c->updating, c->wait_time);
>
> if (c->updating) {
> - return NGX_DECLINED;
> + return rc;
> }
>
> if (c->lock_timeout == 0) {

I don't like the idea of pass-through of the return code. Rather,
return code of the ngx_http_file_cache_lock() should be checked by
the caller, and appropriate actions takens depending on it.

> diff -r 2117637f64e9 -r 28084fdcef28 src/http/ngx_http_upstream.c
> --- a/src/http/ngx_http_upstream.c Tue Nov 27 17:40:21 2018 +0300
> +++ b/src/http/ngx_http_upstream.c Mon Dec 03 11:31:56 2018 +0000
> @@ -923,6 +923,11 @@
> u->cache_status = NGX_HTTP_CACHE_HIT;
> }
>
> + if (rc == NGX_DECLINED || rc == NGX_HTTP_CACHE_STALE)
> + {

Style: "{" should be on the same line as "if" unless there isn't
enough room.

> + rc = ngx_http_file_cache_lock(r, rc);
> + }
> +
> switch (rc) {
>
> case NGX_OK:

Overall, this part of the ngx_http_upstream_cache() function seems
to be overcomplicated, and it is really not clear how this is
expected to interact with various possible rc values and various
possible ngx_http_file_cache_lock() outcomes.

It may be a good idea to move ngx_http_file_cache_lock() further
down instead of trying to slip it in between these switches().
This will require separate handling of ngx_http_file_cache_lock()
return codes, but this actually looks like a good change, see
comments above.

Also, moving ngx_http_file_cache_lock() further down will allow us
to avoid cache locks when proxy_cache_max_range_offset prevents
caching anyway.

[...]

--
Maxim Dounin
http://mdounin.ru/
_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel