[PATCH] making ngx_http_upstream_test_connect catch "connection refused" with kqueue

[Maxim Dounin]

Hello!

On Mon, Nov 05, 2012 at 10:18:28PM -0800, agentzh wrote:

> Hello!
> 
> smallfish and I both ran into a small issue with the
> ngx_http_upstream_test_connect which does not catch errors like
> "Connection refused" when kqueue is used.
> 
> The current logic only checks c->write->pending_eof but this error
> actually produced a read event on (at least) both Mac OS X and
> FreeBSD.
> 
> I've attached a simple patch (for nginx 1.2.4) to fix this by checking
> both the read and write events.
> 
> To reproduce the issue, please consider the following Nginx
> configuration snippet (assuming nothing is listening on the local port
> 1234):
> 
>     location = /t {
>         proxy_pass http://127.0.0.1:1234/;
>     }
> 
> Accessing /t gives the following message in error.log:
> 
>     [error] 28474#0: *1 kevent() reported about an closed connection
> (61: Connection refused) while reading response header from upstream
> 
> We can see that ngx_http_upstream missed it in
> ngx_http_upstream_test_connect but caught it when reading the
> (upstream) response header.
> 
> After applying my patch, Nginx can now catch it in the context of connecting:
> 
>     [error] 34067#0: *1 kevent() reported that connect() failed (61:
> Connection refused) while connecting to upstream

Thanks for catching this!

> --- nginx-1.2.4/src/http/ngx_http_upstream.c	2012-08-06 10:34:08.000000000 -0700
> +++ nginx-1.2.4-patched/src/http/ngx_http_upstream.c	2012-11-05
> 21:17:38.000000000 -0800
> @@ -1808,10 +1808,22 @@ ngx_http_upstream_test_connect(ngx_conne
> 
>  #if (NGX_HAVE_KQUEUE)
> 
> +    ngx_event_t *ev;
> +
>      if (ngx_event_flags & NGX_USE_KQUEUE_EVENT)  {
>          if (c->write->pending_eof) {
> +            ev = c->write;
> +
> +        } else if (c->read->pending_eof) {
> +            ev = c->read;
> +
> +        } else {
> +            ev = NULL;
> +        }
> +
> +        if (ev) {
>              c->log->action = "connecting to upstream";
> -            (void) ngx_connection_error(c, c->write->kq_errno,
> +            (void) ngx_connection_error(c, ev->kq_errno,
>                                      "kevent() reported that connect() failed");
>              return NGX_ERROR;
>          }

I don't see a reason to introduce another local variable here, 
what about something like

--- a/src/http/ngx_http_upstream.c
+++ b/src/http/ngx_http_upstream.c
@@ -1809,9 +1809,16 @@ ngx_http_upstream_test_connect(ngx_conne
 #if (NGX_HAVE_KQUEUE)
 
     if (ngx_event_flags & NGX_USE_KQUEUE_EVENT)  {
-        if (c->write->pending_eof) {
+        if (c->write->pending_eof || c->read->pending_eof) {
+            if (c->write->pending_eof) {
+                err = c->write->kq_errno;
+
+            } else {
+                err = c->read->kq_errno;
+            }
+
             c->log->action = "connecting to upstream";
-            (void) ngx_connection_error(c, c->write->kq_errno,
+            (void) ngx_connection_error(c, err, 
                                     "kevent() reported that connect() failed");
             return NGX_ERROR;
         }

instead?

-- 
Maxim Dounin
http://nginx.com/support.html