Merge r617822, r627097, r660207 from trunk:

* Do not retry a request in the case that we either failed to sent a part of the
  request body or if the request is not idempotent.

PR: 44334

* As per niq's comment, better destinct the types of idempotence.

* Introduce a flag to decide whether we sent an body to the backend or not.

Submitted by: rpluem
Reviewed by: jim

git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/branches/2.2.x@660580 13f79535-47bb-0310-9956-ffa450edef68

diff --git a/CHANGES b/CHANGES
index 8f28b8eb5eb5a392b5286ba56c310e963d6fbf12..969cd0b99798537ed40866cf65342aa4782632b2 100644 (file)
--- a/CHANGES
+++ b/CHANGES
@@ -1,6 +1,10 @@
                                                          -*- coding: utf-8 -*-
 Changes with Apache 2.2.9
 
+  *) mod_proxy_ajp: Do not retry request in the case that we either failed to
+     sent a part of the request body or if the request is not idempotent.
+     PR 44334 [Ruediger Pluem]
+
   *) mod_rewrite: Initialize hash needed by ap_register_rewrite_mapfunc early
      enough. PR 44641 [Daniel Lescohier <daniel.lescohier cnet.com>]
 

diff --git a/STATUS b/STATUS
index 47779cd68c8c9c61b51ce74e1e7bca3a991da536..cb2b88c22829a99fba4223c2aa1fe4ae8c0ec56c 100644 (file)
--- a/STATUS
+++ b/STATUS
@@ -90,16 +90,6 @@ RELEASE SHOWSTOPPERS:
 PATCHES ACCEPTED TO BACKPORT FROM TRUNK:
   [ start all new proposals below, under PATCHES PROPOSED. ]
 
- * mod_proxy_ajp: Do not retry request in the case that we either failed to
-   sent a part of the request body or if the request is not idempotent. PR44334
-   Trunk version of patch:
-         http://svn.apache.org/viewvc?rev=617822&view=rev
-         http://svn.apache.org/viewvc?rev=627097&view=rev
-         http://svn.apache.org/viewvc?rev=660207&view=rev
-   Backport version for 2.2.x of patch:
-         Trunk version of patch works
-   +1: rpluem, niq, jim
-
  * mod_proxy: Do not try a direct connection if the connection via a
    remote proxy failed before and the request has a request body.
    Trunk version of patch:

diff --git a/modules/proxy/mod_proxy_ajp.c b/modules/proxy/mod_proxy_ajp.c
index bad2b26e8c04225c4ba6920cdeec2f54801deb63..ff83cdd181ac0379376856e209f05a634f55a0fd 100644 (file)
--- a/modules/proxy/mod_proxy_ajp.c
+++ b/modules/proxy/mod_proxy_ajp.c
@@ -89,6 +89,37 @@ static int proxy_ajp_canon(request_rec *r, char *url)
     return OK;
 }
 
+#define METHOD_NON_IDEMPOTENT       0
+#define METHOD_IDEMPOTENT           1
+#define METHOD_IDEMPOTENT_WITH_ARGS 2
+
+static int is_idempotent(request_rec *r)
+{
+    /*
+     * RFC2616 (9.1.2): GET, HEAD, PUT, DELETE, OPTIONS, TRACE are considered
+     * idempotent. Hint: HEAD requests use M_GET as method number as well.
+     */
+    switch (r->method_number) {
+        case M_GET:
+        case M_DELETE:
+        case M_PUT:
+        case M_OPTIONS:
+        case M_TRACE:
+            /*
+             * If the request has arguments it might have side-effects and thus
+             * it might be undesirable to resent it to a backend again
+             * automatically.
+             */
+            if (r->args) {
+                return METHOD_IDEMPOTENT_WITH_ARGS;
+            }
+            return METHOD_IDEMPOTENT;
+        /* Everything else is not considered idempotent. */
+        default:
+            return METHOD_NON_IDEMPOTENT;
+    }
+}
+
 /*
  * XXX: AJP Auto Flushing
  *
@@ -122,7 +153,7 @@ static int ap_proxy_ajp_request(apr_pool_t *p, request_rec *r,
     apr_bucket_brigade *input_brigade;
     apr_bucket_brigade *output_brigade;
     ajp_msg_t *msg;
-    apr_size_t bufsiz;
+    apr_size_t bufsiz = 0;
     char *buff;
     apr_uint16_t size;
     const char *tenc;
@@ -138,6 +169,7 @@ static int ap_proxy_ajp_request(apr_pool_t *p, request_rec *r,
     proxy_server_conf *psf =
     ap_get_module_config(r->server->module_config, &proxy_module);
     apr_size_t maxsize = AJP_MSG_BUFFER_SZ;
+    int send_body = 0;
 
     if (psf->io_buffer_size_set)
        maxsize = psf->io_buffer_size;
@@ -161,8 +193,17 @@ static int ap_proxy_ajp_request(apr_pool_t *p, request_rec *r,
                      conn->worker->hostname);
         if (status == AJP_EOVERFLOW)
             return HTTP_BAD_REQUEST;
-        else
-            return HTTP_SERVICE_UNAVAILABLE;
+        else {
+            /*
+             * This is only non fatal when the method is idempotent. In this
+             * case we can dare to retry it with a different worker if we are
+             * a balancer member.
+             */
+            if (is_idempotent(r) == METHOD_IDEMPOTENT) {
+                return HTTP_SERVICE_UNAVAILABLE;
+            }
+            return HTTP_INTERNAL_SERVER_ERROR;
+        }
     }
 
     /* allocate an AJP message to store the data of the buckets */
@@ -231,9 +272,14 @@ static int ap_proxy_ajp_request(apr_pool_t *p, request_rec *r,
                              "proxy: send failed to %pI (%s)",
                              conn->worker->cp->addr,
                              conn->worker->hostname);
-                return HTTP_SERVICE_UNAVAILABLE;
+                /*
+                 * It is fatal when we failed to send a (part) of the request
+                 * body.
+                 */
+                return HTTP_INTERNAL_SERVER_ERROR;
             }
             conn->worker->s->transferred += bufsiz;
+            send_body = 1;
         }
     }
 
@@ -249,7 +295,16 @@ static int ap_proxy_ajp_request(apr_pool_t *p, request_rec *r,
                      "proxy: read response failed from %pI (%s)",
                      conn->worker->cp->addr,
                      conn->worker->hostname);
-        return HTTP_SERVICE_UNAVAILABLE;
+        /*
+         * This is only non fatal when we have not sent (parts) of a possible
+         * request body so far (we do not store it and thus cannot sent it
+         * again) and the method is idempotent. In this case we can dare to
+         * retry it with a different worker if we are a balancer member.
+         */
+        if (!send_body && (is_idempotent(r) == METHOD_IDEMPOTENT)) {
+            return HTTP_SERVICE_UNAVAILABLE;
+        }
+        return HTTP_INTERNAL_SERVER_ERROR;
     }
     /* parse the reponse */
     result = ajp_parse_type(r, conn->data);