[PATCH] ngx_event_pipe: add judgment conditions for downstream errors.

[Maxim Dounin]

Hello!

On Tue, Jul 31, 2018 at 11:35:35AM +0800, Robinson Nie wrote:

> Yes, you are right.
> 
> This patch is somewhat ambiguous, so the updated patch is as follows:
> 
> # HG changeset patch
> # User dapeng.ndp<dapeng.ndp at alibaba-inc.com>
> # Date 1533007686 -28800
> #      Tue Jul 31 11:28:06 2018 +0800
> # Node ID 5b7a4bba5238658b9b256cea013d07300c29540d
> # Parent  f7e79596baf209151682f2f7d220161c034657ac
> ngx_event_pipe: When the downstream error occurs, avoid further
> reading upstream.
> 
> When the proxy is turned on and there is no need to cache data locally. If the
> downstream is wrong(p->downstream_error is set), avoid further reading upstream,
> and finalize upstream & downstream as soon as possible.
> 
> diff -r f7e79596baf2 -r 5b7a4bba5238 src/event/ngx_event_pipe.c
> --- a/src/event/ngx_event_pipe.c Tue Jul 24 18:46:54 2018 +0300
> +++ b/src/event/ngx_event_pipe.c Tue Jul 31 11:28:06 2018 +0800
> @@ -112,6 +112,10 @@
>          return NGX_OK;
>      }
> 
> +    if (!p->cacheable && p->downstream_error) {
> +        return NGX_OK;
> +    }
> +
>  #if (NGX_THREADS)
> 
>      if (p->aio) {
> 
> 
> Please help review, thanks!

As far as I see, this patch will prevent correct operation with 
threaded writes.

As previously suggested, you may want to first elaborate on what 
problem you are trying to fix.  With the current code, the 
decision on whether nginx needs to stop reading from the upstream in 
case of downstream error is made by the caller (in the 
ngx_http_upstream_process_request() in case of the upstream 
module).  While moving this decision from the caller to the event 
pipe code is possible, it certainly have some downsides.  And the 
possible benefits - saving one iteration of reading from the 
upstream till it blocks - might not be a good enough reason for 
the change.

I suspect that what you are trying to address is actually worker 
monopolization while working with fast clients / upstream servers, 
as described here:

https://trac.nginx.org/nginx/ticket/1431

With the downstream error such monopolization is more likely since 
there is no client to block on, and "one iteration of reading from 
the upstream till it blocks" might be actually something important 
enough.  This problem is not limited to the downstream error case 
though, and not even to the event pipe, and addressing it via 
trying to improve some corner cases in the event pipe code looks 
wrong.

-- 
Maxim Dounin
http://mdounin.ru/