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

[Maxim Dounin]

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/