Fixed upstream->read timer when downstream->write not ready

[Maxim Dounin]

Hello!

On Fri, Jan 19, 2018 at 08:43:06PM +0800, xiaolong hong wrote:

> # HG changeset patch
> # User Xiaolong Hong <imhongxiaolong at gmail.com>
> # Date 1516354115 -28800
> #      Fri Jan 19 17:28:35 2018 +0800
> # Node ID f017b8c1a99433cc3321475968556aee50609145
> # Parent  93abb5a855d6534f0356882f45be49f8c6a95a8b
> Fixed upstream->read timer when downstream->write not ready.
> 
> diff -r 93abb5a855d6 -r f017b8c1a994 src/http/ngx_http_upstream.c
> --- a/src/http/ngx_http_upstream.c      Thu Jan 11 21:43:49 2018 +0300
> +++ b/src/http/ngx_http_upstream.c      Fri Jan 19 17:28:35 2018 +0800
> @@ -3625,7 +3625,9 @@ ngx_http_upstream_process_non_buffered_r
>          return;
>      }
> 
> -    if (upstream->read->active && !upstream->read->ready) {
> +    if (upstream->read->active && !upstream->read->ready
> +        && !(u->length == 0 || (upstream->read->eof && u->length == -1)))
> +    {
>          ngx_add_timer(upstream->read, u->conf->read_timeout);
> 
>      } else if (upstream->read->timer_set) {
> 
> --------
> 
> When downstream hung and nginx received the last buffer from upstream, both
> downstream->write timer and upstream->read timer would be added in the
> meantime because downstream->write->ready and upstream->read->ready would
> opportunely be 0.
> 
> Actually if clcf->send_timeout less then u->conf->read_timeout,
> upstream->read timer would be waked before downstream->write timer, it
> caused mistakes to report the "upstream timed out" error deviating from the
> fact that upstream worked normally but downstream hung.
> 
> This problem could be fixed to check upstream eof when trying to add
> upstream->read timer.

Thank you for the patch.

The problem looks valid - indeed, if in non-buffered proxy mode 
writing to the client blocked, it is possible that we'll continue 
reading from upstream after the response was fully received, and 
will keep upstream read timer set.  This in turn can result in 
proxy_read_timeout being triggered if it is smaller than send_timeout.

I can't say I like the suggested change though.  The resulting 
conditions look overcomplicated.  Also, likely we have similar 
problem in ngx_http_upstream_process_upgraded(), and it probably 
needs fixing too.

Also, suggested condition is incorrect, as it will keep timer if 
upstream->read->eof is set, but u->length isn't -1, or if 
upstream->read->error is set.

In case of ngx_http_upstream_process_non_buffered_request() a 
better approach might be to don't try to do any processing in the 
proxy module after response is received from upstream, but rather 
finalize the upstream connection and pass responsibility to 
ngx_writer() instead, as we do in the normal (aka buffered) case.  
For ngx_http_upstream_process_upgraded(), additional ->eof / 
->error checks are probably needed.

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