Opened 11 years ago
Closed 10 years ago
#486 closed defect (worksforme)
ngx_http_upstream module lack the check of broken peer connection before using it
| Reported by: | marc li | Owned by: | |
|---|---|---|---|
| Priority: | minor | Milestone: | |
| Component: | nginx-module | Version: | 1.5.x |
| Keywords: | Cc: | ||
| uname -a: | Linux tcpper-vm 3.5.0-44-generic #67~precise1-Ubuntu SMP Wed Nov 13 16:16:57 UTC 2013 x86_64 x86_64 x86_64 GNU/Linux | ||
| nginx -V: |
nginx version: nginx/1.4.4
built by gcc 4.6.3 (Ubuntu/Linaro 4.6.3-1ubuntu5) TLS SNI support enabled configure arguments: --prefix=/home/tcpper/nginx --with-debug --with-pcre=../pcre-8.33 --with-pcre-jit --with-http_stub_status_module --with-http_gunzip_module --with-http_ssl_module --add-module=../ngx_devel_kit-0.2.19 --add-module=../lua-nginx-module-0.9.3 --add-module=../echo-nginx-module-0.48 --add-module=../ngx_pagespeed-1.7.30.1-beta --add-module=../upstream_jhash --without-http_ssi_module --without-http_userid_module --without-http_geo_module --without-http_map_module --without-http_split_clients_module --without-http_fastcgi_module --without-http_uwsgi_module --without-http_scgi_module --without-http_memcached_module --without-http_empty_gif_module --without-http_browser_module --with-ld-opt=-Wl,-rpath,/home/tcpper/luajit/lib |
||
Description
HI, guys,
ngx_http_upstream module use ngx_http_upstream_test_connect in order to valid connection, however for keepalived upstream connection which may be closed before the test (ngx_http_upstream_keepalive_close_handler may not be triggered before peer.get pick it as the new connection), the test may not be adequate.
So i suggest that we do a MSG_PEEK in nix_http_upstream_test_connect as below:
diff -ur nginx-1.4.4/src/http/ngx_http_upstream.c nginx-1.4.4-local/src/http/ngx_http_upstream.c
--- nginx-1.4.4/src/http/ngx_http_upstream.c 2013-11-19 19:25:25.000000000 +0800
+++ nginx-1.4.4-local/src/http/ngx_http_upstream.c 2014-01-14 18:01:58.061499032 +0800
@@ -1886,6 +1886,22 @@
}
}
+ /*
+ * Maybe getsockopt is not enough, let's pick one byte from the connection
+ * To prevent broken keepalived conn from being used
+ */
+ char buf[1];
+ int n = recv(c->fd, buf, 1, MSG_PEEK);
+ err = ngx_socket_errno;
+
+ ngx_log_debug1(NGX_LOG_DEBUG_HTTP, c->log, err,
+ "http upstream ngx_upstream_test_connect: %d", n);
+
+ (n < 0 && err != NGX_EAGAIN)) {
+ return NGX_ERROR;
+ }
+
return NGX_OK;
}
3Q~
The suggested change doesn't look right. The ngx_http_upstream_test_connect() function is to detect and properly report connect() errors, not to test if a connection is closed. On the other hand, possibility of keepalive connections being reused at the same event loop iteration they are reported to be closed is expected to be very small, and if you see this happening frequently - you may want to focus on why it happens instead.