Upstream error handling issue

[Maxim Dounin]

Hello!

On Mon, Aug 19, 2013 at 06:36:17PM +0300, Aviram Cohen wrote:

> Hello!
> 
> Regarding the patch - it seems that if ngx_http_upstream_send_request()
> fails (the call to the function is in line 1342),
> ngx_http_run_posted_requests() is still not called. Should a call to this
> function be added there as well?

Yes, thanks, something like this should be better:

--- a/src/http/ngx_http_upstream.c
+++ b/src/http/ngx_http_upstream.c
@@ -1338,13 +1338,19 @@ ngx_http_upstream_ssl_handshake(ngx_conn
         c->write->handler = ngx_http_upstream_handler;
         c->read->handler = ngx_http_upstream_handler;
 
+        c = r->connection;
+
         ngx_http_upstream_send_request(r, u);
 
+        ngx_http_run_posted_requests(c);
         return;
     }
 
+    c = r->connection;
+
     ngx_http_upstream_next(r, u, NGX_HTTP_UPSTREAM_FT_ERROR);
 
+    ngx_http_run_posted_requests(c);
 }
 
 #endif


> 
> Regards
> 
> 
> On Mon, Aug 19, 2013 at 6:17 PM, Maxim Dounin <mdounin at mdounin.ru> wrote:
> 
> > Hello!
> >
> > On Mon, Aug 19, 2013 at 05:17:24PM +0300, Aviram Cohen wrote:
> >
> > > Hello!
> > >
> > > I have encountered a potential bug in Nginx's upstream module -
> > > When the upstream server is an SSL server, if an error occurs in
> > > ngx_http_upstream_ssl_handshake() - the
> > > function ngx_http_run_posted_requests() is never called.
> > > This happens when initiating an SSL connection, the SSL module handles
> > the
> > > handshake, and not the upstream module (meaning
> > ngx_http_upstream_handler()
> > > is not involved in the process), and so if an error occurs, there's no
> > one
> > > who calls ngx_http_run_posted_requests().
> > >
> > > The effect of this issue is the requests that "spawn" subrequests that
> > use
> > > the upstream error get stuck in case of an SSL error.
> > > I can suggest two possible fixes (in the file ngx_http_upstream.c):
> > >   - Add a call to ngx_http_run_posted_requests() to the end
> > > of ngx_http_upstream_finalize_request().
> > >   - Add a call to ngx_http_run_posted_requests() after calling
> > > ngx_http_upstream_finalize_request() during error handling of the SSL
> > > connection establishment.
> > >
> > > Can anyone verify this issue and the suggested solution? If so, I'll be
> > > more than happy to submit a patch.
> >
> > Yes, it looks like there is a problem.
> >
> > The ngx_http_run_posted_requests() is usually called by an event
> > handler, so adding a call to ngx_http_upstream_ssl_handshake()
> > might be more appropriate.  Something like this should fix the
> > problem:
> >
> > diff --git a/src/http/ngx_http_upstream.c b/src/http/ngx_http_upstream.c
> > --- a/src/http/ngx_http_upstream.c
> > +++ b/src/http/ngx_http_upstream.c
> > @@ -1343,8 +1343,11 @@ ngx_http_upstream_ssl_handshake(ngx_conn
> >          return;
> >      }
> >
> > +    c = r->connection;
> > +
> >      ngx_http_upstream_next(r, u, NGX_HTTP_UPSTREAM_FT_ERROR);
> >
> > +    ngx_http_run_posted_requests(c);
> >  }
> >
> >  #endif
> >
> >
> > --
> > Maxim Dounin
> > http://nginx.org/en/donation.html
> >
> > _______________________________________________
> > nginx-devel mailing list
> > nginx-devel at nginx.org
> > http://mailman.nginx.org/mailman/listinfo/nginx-devel
> >

> _______________________________________________
> nginx-devel mailing list
> nginx-devel at nginx.org
> http://mailman.nginx.org/mailman/listinfo/nginx-devel


-- 
Maxim Dounin
http://nginx.org/en/donation.html