httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Yann Ylavic <ylavic....@gmail.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 Tue, 08 Oct 2013 20:24:52 GMT
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 <jim@jagunet.com> wrote:

> OK, I gotcha now...
>
> Do you have a patch file? tia!
>
> On Oct 8, 2013, at 12:56 PM, Yann Ylavic <ylavic.dev@gmail.com> wrote:
>
> > On Tue, Oct 8, 2013 at 6:26 PM, Jim Jagielski <jim@jagunet.com> 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 <ylavic.dev@gmail.com> 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, <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/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.",
> > >
> > >
> > >
> >
> >
>
>

Mime
View raw message