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

[Xiaolong Hong]

> Hello!
> 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.

Yes, we were plagued with this problem because we needed to make a
distinction between upstream timeout and client timeout in our production
system.

> 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.

I'm sorry I missed another condition and the correct one may be

```
if (upstream->read->active && !upstream->read->ready
    && !(u->length == 0 || (upstream->read->eof && u->length == -1))
    && !upstream->read->error && upstream->read->eof)
{
```

It is assuredly overcomplicated, or it can be in another function to check
the correct condition.

> 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.

In case of ngx_http_upstream_process_non_buffered_request(), NGINX receives
from upstream and immediately sends to downstream. I'm afraid the problem
still will not be solved accord with "after response is received from
upstream".
Furthermore, kqueue notifies about the end of file or a pending error while
epoll don't.

Do you have any other solutions or advices?

Thanks to inform me.


2018-01-23 0:34 GMT+08:00 Maxim Dounin <mdounin at mdounin.ru>:

> 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/
> _______________________________________________
> nginx-devel mailing list
> nginx-devel at nginx.org
> http://mailman.nginx.org/mailman/listinfo/nginx-devel
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.nginx.org/pipermail/nginx-devel/attachments/20180201/cd8af985/attachment.html>