Anyone else think this isn't worthwhile? I'll revert. No sense in adding stuff if not "useful".
On Aug 23, 2013, at 2:20 PM, Jim Jagielski <[email protected]> wrote: > I explained the rationale. > > Thx for the ping_timeout_set catch, fwiw > > On Aug 23, 2013, at 2:08 PM, Ruediger Pluem <[email protected]> wrote: > >> >> >> [email protected] wrote: >>> Author: jim >>> Date: Fri Aug 23 16:48:42 2013 >>> New Revision: 1516930 >>> >>> URL: http://svn.apache.org/r1516930 >>> Log: >>> Allow for a simple socket check in addition to the >>> higher level protocol-level checks for backends... >>> >>> Not sure if it makes sense to do both or not... Comments? >>> >>> Modified: >>> httpd/httpd/trunk/docs/manual/mod/mod_proxy.xml >>> httpd/httpd/trunk/modules/proxy/mod_proxy.c >>> httpd/httpd/trunk/modules/proxy/mod_proxy.h >>> httpd/httpd/trunk/modules/proxy/mod_proxy_ajp.c >>> httpd/httpd/trunk/modules/proxy/mod_proxy_http.c >>> httpd/httpd/trunk/modules/proxy/proxy_util.c >>> >>> Modified: httpd/httpd/trunk/modules/proxy/mod_proxy_ajp.c >>> URL: >>> http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/proxy/mod_proxy_ajp.c?rev=1516930&r1=1516929&r2=1516930&view=diff >>> ============================================================================== >>> --- httpd/httpd/trunk/modules/proxy/mod_proxy_ajp.c (original) >>> +++ httpd/httpd/trunk/modules/proxy/mod_proxy_ajp.c Fri Aug 23 16:48:42 2013 >>> @@ -759,22 +759,35 @@ static int proxy_ajp_handler(request_rec >>> >>> /* Handle CPING/CPONG */ >>> if (worker->s->ping_timeout_set) { >>> - status = ajp_handle_cping_cpong(backend->sock, r, >>> - worker->s->ping_timeout); >>> - /* >>> - * In case the CPING / CPONG failed for the first time we >>> might be >>> - * just out of luck and got a faulty backend connection, but >>> the >>> - * backend might be healthy nevertheless. So ensure that the >>> backend >>> - * TCP connection gets closed and try it once again. >>> - */ >>> - if (status != APR_SUCCESS) { >>> - backend->close = 1; >>> - ap_log_rerror(APLOG_MARK, APLOG_ERR, status, r, >>> APLOGNO(00897) >>> - "cping/cpong failed to %pI (%s)", >>> - worker->cp->addr, worker->s->hostname); >>> - status = HTTP_SERVICE_UNAVAILABLE; >>> - retry++; >>> - continue; >>> + if (worker->s->ping_timeout_set < 0) { >> >> How can this < 0? It is either 0 or 1. >> I guess it should be ping_timeout. >> >>> + if (!ap_proxy_is_socket_connected(backend->sock)) { >>> + backend->close = 1; >>> + ap_log_rerror(APLOG_MARK, APLOG_ERR, status, r, >>> APLOGNO() >>> + "socket check failed to %pI (%s)", >>> + worker->cp->addr, worker->s->hostname); >>> + status = HTTP_SERVICE_UNAVAILABLE; >>> + retry++; >>> + continue; >>> + } >>> + } >>> + else { >>> + status = ajp_handle_cping_cpong(backend->sock, r, >>> + worker->s->ping_timeout); >>> + /* >>> + * In case the CPING / CPONG failed for the first time we >>> might be >>> + * just out of luck and got a faulty backend connection, >>> but the >>> + * backend might be healthy nevertheless. So ensure that >>> the backend >>> + * TCP connection gets closed and try it once again. >>> + */ >>> + if (status != APR_SUCCESS) { >>> + backend->close = 1; >>> + ap_log_rerror(APLOG_MARK, APLOG_ERR, status, r, >>> APLOGNO(00897) >>> + "cping/cpong failed to %pI (%s)", >>> + worker->cp->addr, worker->s->hostname); >>> + status = HTTP_SERVICE_UNAVAILABLE; >>> + retry++; >>> + continue; >>> + } >>> } >>> } >>> /* Step Three: Process the Request */ >> >> >> To be honest. I think this additional check is a waste of cycles as we >> already know that the socket is connected at this >> point of time and even if it just disconnected it could happen also a few >> moments later when we passed this check. So I >> see no gain for the race condition we face regarding the connectivity state >> of the socket. But as it is only done with >> negative ping timeouts I am -0. >> >> Regards >> >> Rüdiger >> >
