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@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@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel