httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jim Jagielski <...@jaguNET.com>
Subject Re: svn commit: r1516930 - in /httpd/httpd/trunk: docs/manual/mod/mod_proxy.xml modules/proxy/mod_proxy.c modules/proxy/mod_proxy.h modules/proxy/mod_proxy_ajp.c modules/proxy/mod_proxy_http.c modules/proxy/proxy_util.c
Date Fri, 23 Aug 2013 18:29:12 GMT
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 <jim@jaguNET.com> wrote:

> I explained the rationale.
> 
> Thx for the ping_timeout_set catch, fwiw
> 
> On Aug 23, 2013, at 2:08 PM, Ruediger Pluem <rpluem@apache.org> wrote:
> 
>> 
>> 
>> jim@apache.org 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
>> 
> 


Mime
View raw message