[PATCH] Apply cache locking behaviour to stale cache entries.

[Elliot Thomas]

Hello!

On 10/12/2018, 19:07, "nginx-devel on behalf of Maxim Dounin"
<nginx-devel-bounces at nginx.org on behalf of mdounin at mdounin.ru> wrote:

...

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

Unfortunately, I have no control over the disclaimer (which does look a
bit silly on a public mailing list).

I will add a disclaimer to the disclaimer where appropriate.

...

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

Noted and changed.

...

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

I guess this could cause a change in behaviour. If my understanding of
Nginx is correct, u->conf->cache_lock can be different on a per-location
basis, but c->lock would be set by the first request.

But I can certainly remove this check/field if desired.

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

...

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

Sounds reasonable, I have reworked this part.

Now, for any "u->cacheable" upstream request that reaches the end of the
ngx_http_upstream() function, the locking function is called.

As the ngx_http_file_cache_lock() return code is no longer handled in
the same way as a ngx_http_file_cache_open() return code,
I took the liberty of changing them so they related to the action of
"acquiring a lock" rather than what ngx_http_file_cache_open() would do.

* NGX_OK - returned if acquiring the lock is successful or unnecessary.
    This was NGX_DECLINED.
* NGX_AGAIN - returned if lock is held (no change from previous usage).
    This has not changed.
* NGX_BUSY - returned if lock is held and lock timeout reached.
    This was NGX_HTTP_CACHE_SCARCE.

Attached is a revised patch.


Regards, Elliot.

---
Please ignore the following disclaimer, it’s a bit silly.
I have read the contribution guide, and am fine with it.





-----------------------------
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.
-----------------------------
-------------- next part --------------
A non-text attachment was scrubbed...
Name: proxy-cache-lock-on-stale-v2.patch
Type: application/octet-stream
Size: 3263 bytes
Desc: proxy-cache-lock-on-stale-v2.patch
URL: <http://mailman.nginx.org/pipermail/nginx-devel/attachments/20181214/3cc6233b/attachment.obj>