Hello!

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

> # HG changeset patch
> # User Xiaolong Hong <imhongxiaolong@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/
_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel