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

[Robinson Nie]

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!


2018-07-31 1:21 GMT+08:00 Maxim Dounin <mdounin at mdounin.ru>:
> Hello!
>
> On Mon, Jul 30, 2018 at 09:33:11PM +0800, Robinson Nie wrote:
>
>> Here is a patch to solve the following problem: When the proxy is turned
>> on, if the downstream error (p->output_filter() returns an error), the
>> current code will still try to read upstream & write downstream, which is
>> actually unnecessary.
>>
>> # HG changeset patch
>> # User dapeng.ndp<dapeng.ndp at alibaba-inc.com>
>> # Date 1532956361 -28800
>> #      Mon Jul 30 21:12:41 2018 +0800
>> # Node ID cbacf180df090540846aefa4a4a5453e307be219
>> # Parent  f7e79596baf209151682f2f7d220161c034657ac
>> ngx_event_pipe: add judgment conditions for downstream errors.
>>
>> Avoid writing downstream & reading upstream, if there is an error due to
>> writing downstream.
>>
>> diff -r f7e79596baf2 -r cbacf180df09 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        Mon Jul 30 21:12:41 2018 +0800
>> @@ -247,6 +247,7 @@
>>              } else if (!p->cacheable
>>                         && p->downstream->data == p->output_ctx
>>                         && p->downstream->write->ready
>> +                       && !p->downstream_error
>>                         && !p->downstream->write->delayed)
>>              {
>>                  /*
>
> If there was a downstream error, nginx won't write to the
> downstream anymore, see the ngx_event_pipe_write_to_downstream()
> function.  Instead, it will call the ngx_event_pipe_drain_chains()
> function to discard all pending output buffers.
>
> Reading from the upstream may be desired or not depending on
> additional conditions.  In particular, if caching is used, nginx
> will have to read data from the upstream to save the response to
> disk even in case of errors.  If further reading of the upstream
> response is not needed and p->downstream_error is set, nginx will
> stop processing of the request in ngx_http_upstream_process_request().
> Assuming p->downstream_error is set, this will happen once reading
> from the upstream blocks.
>
> Note well that the specific condition you are modifying is about
> flushing the already read buffers if writing to the upstream is
> possible.  From semantic point of view, it is possible when
> p->downstream_error is set (it will simply discard the buffers),
> so the change looks wrong.  Also, if you are trying to avoid
> reading from the upstream if p->downstream_error is set, it looks
> like a suboptimal place, as it will not prevent reading from the
> upstream unless all buffers are filled.
>
> Given the above, it is not clear what you are trying to improve.
> The commit log is clearly misleading, and needs to be fixed.  It
> is also not clear if any changes are needed here.  You may want to
> elaborate more on what you are trying to improve.
>
> --
> Maxim Dounin
> http://mdounin.ru/
> _______________________________________________
> nginx-devel mailing list
> nginx-devel at nginx.org
> http://mailman.nginx.org/mailman/listinfo/nginx-devel