Sure, here it is. Please note 2 chances compared to the previous patch (pasted) : - the slow path ap_request_has_body used last to compute do_100_continue, - step Three-and-a-Half moved into step 3, with the associated comment updated.
For the latter change, it avoids the systematic double-check (needlessly, IMHO) when light ping is configured and the connection is reused. Thank you for taking that into account. On Tue, Oct 8, 2013 at 8:52 PM, Jim Jagielski <[email protected]> wrote: > OK, I gotcha now... > > Do you have a patch file? tia! > > On Oct 8, 2013, at 12:56 PM, Yann Ylavic <[email protected]> wrote: > > > On Tue, Oct 8, 2013 at 6:26 PM, Jim Jagielski <[email protected]> wrote: > > Does it matter whether or not it's a heavy ping or not? > > It doesn't matter what sort of test was used, the socket is down. > > > > Yes it is down, but for the ajp case for example, that determines the > return value GATEWAY_TIMEOUT vs INTERNAL_SERVER_ERROR, and according to the > comments, the former is when cping/cpong was used and that will not "affect > the whole worker", does it make sense when the light ping (socket > health-check) only was done before forwarding? > > > > For the ap_proxy_http_request error case, should mod_proxy retry a new > request when the heavy ping is off (light ping on)? > > > > For ap_proxy_create_hdrbrgd and ap_proxy_process_response, the "100 > continue" things have nothing to do with light ping only, have they? > > > > Regards. > > > > > > On Oct 8, 2013, at 11:53 AM, Yann Ylavic <[email protected]> wrote: > > > Hi, > > > > > > Some code in trunk still only check for ping_timeout_set without > ping_timeout positive value to handle the "heavy" ping (CPING/Expect: > 100-continue), see patch below. > > > > > > Regards, > > > Yann. > > > > > > Index: modules/proxy/mod_proxy_ajp.c > > > =================================================================== > > > --- modules/proxy/mod_proxy_ajp.c (revision 1530243) > > > +++ modules/proxy/mod_proxy_ajp.c (working copy) > > > @@ -341,7 +341,8 @@ static int ap_proxy_ajp_request(apr_pool_t *p, req > > > * we assume it is a request that cause a back-end timeout, > > > * but doesn't affect the whole worker. > > > */ > > > - if (APR_STATUS_IS_TIMEUP(status) && > conn->worker->s->ping_timeout_set) { > > > + if (APR_STATUS_IS_TIMEUP(status) && > conn->worker->s->ping_timeout_set > > > + && conn->worker->s->ping_timeout >= 0) { > > > return HTTP_GATEWAY_TIME_OUT; > > > } > > > > > > @@ -661,7 +662,9 @@ static int ap_proxy_ajp_request(apr_pool_t *p, req > > > * we assume it is a request that cause a back-end > timeout, > > > * but doesn't affect the whole worker. > > > */ > > > - if (APR_STATUS_IS_TIMEUP(status) && > conn->worker->s->ping_timeout_set) { > > > + if (APR_STATUS_IS_TIMEUP(status) && > > > + conn->worker->s->ping_timeout_set && > > > + conn->worker->s->ping_timeout >= 0) { > > > apr_table_set(r->notes, "proxy_timedout", "1"); > > > rv = HTTP_GATEWAY_TIME_OUT; > > > } > > > Index: modules/proxy/proxy_util.c > > > =================================================================== > > > --- modules/proxy/proxy_util.c (revision 1530243) > > > +++ modules/proxy/proxy_util.c (working copy) > > > @@ -3073,6 +3073,7 @@ PROXY_DECLARE(int) > ap_proxy_create_hdrbrgd(apr_poo > > > * We also make sure we won't be talking HTTP/1.0 as well. > > > */ > > > do_100_continue = (worker->s->ping_timeout_set > > > + && (worker->s->ping_timeout >= 0) > > > && ap_request_has_body(r) > > > && (PROXYREQ_REVERSE == r->proxyreq) > > > && !(apr_table_get(r->subprocess_env, > "force-proxy-request-1.0"))); > > > Index: modules/proxy/mod_proxy_http.c > > > =================================================================== > > > --- modules/proxy/mod_proxy_http.c (revision 1530243) > > > +++ modules/proxy/mod_proxy_http.c (working copy) > > > @@ -1241,6 +1241,7 @@ int ap_proxy_http_process_response(apr_pool_t * > p, > > > dconf = ap_get_module_config(r->per_dir_config, &proxy_module); > > > > > > do_100_continue = (worker->s->ping_timeout_set > > > + && (worker->s->ping_timeout >= 0) > > > && ap_request_has_body(r) > > > && (PROXYREQ_REVERSE == r->proxyreq) > > > && !(apr_table_get(r->subprocess_env, > "force-proxy-request-1.0"))); > > > @@ -1992,8 +1993,9 @@ static int proxy_http_handler(request_rec *r, > prox > > > */ > > > if ((status = ap_proxy_http_request(p, r, backend, worker, > > > conf, uri, locurl, > server_portstr)) != OK) { > > > - if ((status == HTTP_SERVICE_UNAVAILABLE) && > worker->s->ping_timeout_set && > > > - worker->s->ping_timeout > 0) { > > > + if ((status == HTTP_SERVICE_UNAVAILABLE) && > > > + worker->s->ping_timeout_set && > > > + worker->s->ping_timeout >= 0) { > > > backend->close = 1; > > > ap_log_rerror(APLOG_MARK, APLOG_INFO, status, r, > APLOGNO(01115) > > > "HTTP: 100-Continue failed to %pI (%s)", > > > > > > > > > > > > On Fri, Aug 23, 2013 at 6:48 PM, <[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/docs/manual/mod/mod_proxy.xml > > > URL: > http://svn.apache.org/viewvc/httpd/httpd/trunk/docs/manual/mod/mod_proxy.xml?rev=1516930&r1=1516929&r2=1516930&view=diff > > > > ============================================================================== > > > --- httpd/httpd/trunk/docs/manual/mod/mod_proxy.xml (original) > > > +++ httpd/httpd/trunk/docs/manual/mod/mod_proxy.xml Fri Aug 23 > 16:48:42 2013 > > > @@ -1003,7 +1003,9 @@ ProxyPass /mirror/foo http://backend.exa > > > <tr><td>ping</td> > > > <td>0</td> > > > <td>Ping property tells the webserver to "test" the > connection to > > > - the backend before forwarding the request. For AJP, it causes > > > + the backend before forwarding the request. For negative values > > > + the test is a simple socket check, for positive values it's > > > + a more functional check, dependent upon the protocol. For > AJP, it causes > > > <module>mod_proxy_ajp</module>to send a <code>CPING</code> > > > request on the ajp13 connection (implemented on Tomcat > 3.3.2+, 4.1.28+ > > > and 5.0.13+). For HTTP, it causes > <module>mod_proxy_http</module> > > > > > > Modified: httpd/httpd/trunk/modules/proxy/mod_proxy.c > > > URL: > http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/proxy/mod_proxy.c?rev=1516930&r1=1516929&r2=1516930&view=diff > > > > ============================================================================== > > > --- httpd/httpd/trunk/modules/proxy/mod_proxy.c (original) > > > +++ httpd/httpd/trunk/modules/proxy/mod_proxy.c Fri Aug 23 16:48:42 > 2013 > > > @@ -233,7 +233,7 @@ static const char *set_worker_param(apr_ > > > */ > > > if (ap_timeout_parameter_parse(val, &timeout, "s") != > APR_SUCCESS) > > > return "Ping/Pong timeout has wrong format"; > > > - if (timeout < 1000) > > > + if (timeout < 1000 && timeout >= 0) > > > return "Ping/Pong timeout must be at least one > millisecond"; > > > worker->s->ping_timeout = timeout; > > > worker->s->ping_timeout_set = 1; > > > > > > Modified: httpd/httpd/trunk/modules/proxy/mod_proxy.h > > > URL: > http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/proxy/mod_proxy.h?rev=1516930&r1=1516929&r2=1516930&view=diff > > > > ============================================================================== > > > --- httpd/httpd/trunk/modules/proxy/mod_proxy.h (original) > > > +++ httpd/httpd/trunk/modules/proxy/mod_proxy.h Fri Aug 23 16:48:42 > 2013 > > > @@ -972,6 +972,13 @@ PROXY_DECLARE(int) ap_proxy_pass_brigade > > > APR_DECLARE_OPTIONAL_FN(int, ap_proxy_clear_connection, > > > (request_rec *r, apr_table_t *headers)); > > > > > > + > > > +/** > > > + * @param socket socket to test > > > + * @return TRUE if socket is connected/active > > > + */ > > > +PROXY_DECLARE(int) ap_proxy_is_socket_connected(apr_socket_t *socket); > > > + > > > #define PROXY_LBMETHOD "proxylbmethod" > > > > > > /* The number of dynamic workers that can be added when reconfiguring. > > > > > > 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) { > > > + 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 */ > > > > > > Modified: httpd/httpd/trunk/modules/proxy/mod_proxy_http.c > > > URL: > http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/proxy/mod_proxy_http.c?rev=1516930&r1=1516929&r2=1516930&view=diff > > > > ============================================================================== > > > --- httpd/httpd/trunk/modules/proxy/mod_proxy_http.c (original) > > > +++ httpd/httpd/trunk/modules/proxy/mod_proxy_http.c Fri Aug 23 > 16:48:42 2013 > > > @@ -1975,13 +1975,25 @@ static int proxy_http_handler(request_re > > > } > > > } > > > > > > + /* Step Three-and-a-Half: See if the socket is still > connected (if desired) */ > > > + if (worker->s->ping_timeout_set && worker->s->ping_timeout < > 0 && > > > + !ap_proxy_is_socket_connected(backend->sock)) { > > > + backend->close = 1; > > > + ap_log_rerror(APLOG_MARK, APLOG_INFO, status, r, APLOGNO() > > > + "socket check failed to %pI (%s)", > > > + worker->cp->addr, worker->s->hostname); > > > + retry++; > > > + continue; > > > + } > > > + > > > /* Step Four: Send the Request > > > * On the off-chance that we forced a 100-Continue as a > > > * kinda HTTP ping test, allow for retries > > > */ > > > if ((status = ap_proxy_http_request(p, r, backend, worker, > > > conf, uri, locurl, > server_portstr)) != OK) { > > > - if ((status == HTTP_SERVICE_UNAVAILABLE) && > worker->s->ping_timeout_set) { > > > + if ((status == HTTP_SERVICE_UNAVAILABLE) && > worker->s->ping_timeout_set && > > > + worker->s->ping_timeout > 0) { > > > backend->close = 1; > > > ap_log_rerror(APLOG_MARK, APLOG_INFO, status, r, > APLOGNO(01115) > > > "HTTP: 100-Continue failed to %pI (%s)", > > > > > > Modified: httpd/httpd/trunk/modules/proxy/proxy_util.c > > > URL: > http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/proxy/proxy_util.c?rev=1516930&r1=1516929&r2=1516930&view=diff > > > > ============================================================================== > > > --- httpd/httpd/trunk/modules/proxy/proxy_util.c (original) > > > +++ httpd/httpd/trunk/modules/proxy/proxy_util.c Fri Aug 23 16:48:42 > 2013 > > > @@ -2245,7 +2245,7 @@ ap_proxy_determine_connection(apr_pool_t > > > #endif > > > > > > #if USE_ALTERNATE_IS_CONNECTED && defined(APR_MSG_PEEK) > > > -static int is_socket_connected(apr_socket_t *socket) > > > +PROXY_DECLARE(int) ap_proxy_is_socket_connected(apr_socket_t *socket) > > > { > > > apr_pollfd_t pfds[1]; > > > apr_status_t status; > > > @@ -2283,7 +2283,7 @@ static int is_socket_connected(apr_socke > > > > > > } > > > #else > > > -static int is_socket_connected(apr_socket_t *sock) > > > +PROXY_DECLARE(int) ap_proxy_is_socket_connected(apr_socket_t *socket) > > > > > > { > > > apr_size_t buffer_len = 1; > > > @@ -2466,7 +2466,7 @@ PROXY_DECLARE(int) ap_proxy_connect_back > > > (proxy_server_conf *) ap_get_module_config(sconf, > &proxy_module); > > > > > > if (conn->sock) { > > > - if (!(connected = is_socket_connected(conn->sock))) { > > > + if (!(connected = ap_proxy_is_socket_connected(conn->sock))) { > > > socket_cleanup(conn); > > > ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, s, APLOGNO(00951) > > > "%s: backend socket is disconnected.", > > > > > > > > > > > > > > >
httpd-trunk-mod_proxy_ping_timeout.patch
Description: Binary data
