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

[Robinson Nie]

When a downstream error occurs, the reading upstream(
p->upstream->recv_chain() ) is executed not only once but multiple
times until it triggers p->upstream->read->ready becomes 0, or p->
upstream_eof / p-> upstream_error / p -> upstream_done becomre !0.

This patch wants to avoid these invalid reading upstreams and finalize
upstream & downstream as soon as possible.

Thank you for your advice!


2018-07-31 22:52 GMT+08:00 Maxim Dounin <mdounin at mdounin.ru>:
> 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/
> _______________________________________________
> nginx-devel mailing list
> nginx-devel at nginx.org
> http://mailman.nginx.org/mailman/listinfo/nginx-devel