Bug 59869 - mod_proxy failonstatus option is broken since 2.4.17 on Solaris 10
Summary: mod_proxy failonstatus option is broken since 2.4.17 on Solaris 10
Status: RESOLVED FIXED
Alias: None
Product: Apache httpd-2
Classification: Unclassified
Component: mod_proxy (show other bugs)
Version: 2.4.23
Hardware: All Solaris
: P2 regression (vote)
Target Milestone: ---
Assignee: Apache HTTPD Bugs Mailing List
URL:
Keywords: FixedInTrunk, PatchAvailable
Depends on:
Blocks:
 
Reported: 2016-07-16 17:40 UTC by Nikita Akhnin
Modified: 2016-11-14 12:21 UTC (History)
0 users



Attachments
Patch against trunk (1.65 KB, text/plain)
2016-07-18 07:51 UTC, Ruediger Pluem
Details
Patch against trunk (1.65 KB, patch)
2016-07-18 07:55 UTC, Ruediger Pluem
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nikita Akhnin 2016-07-16 17:40:09 UTC
Configuration:
<Proxy "balancer://cluster">
  BalancerMember "http://localhost:17293/webapp" route=node1 retry=10 ping=1 connectiontimeout=1
  BalancerMember "http://localhost:17294/webapp" route=node2 retry=10 ping=1 connectiontimeout=1
</Proxy>

ProxyPass / "balancer://cluster/" stickysession=JSESSIONID|jsessionid failonstatus=502,503

For 2.4.16 and later versions, when backend node returns 503, Apache forces it into error state.
Since 2.4.17 till 2.4.23, when 503 returned, nothing happens, node status is OK and Apache keep on proxying requests on this node

This behavior is reproduced both on Sparc machine and x86 VirtualBox VM
Comment 1 Ruediger Pluem 2016-07-18 07:51:45 UTC
Created attachment 34050 [details]
Patch against trunk

Does the attached patch against trunk (applies to 2.4.x with offset) fix your problem?

Index: modules/proxy/mod_proxy.c
===================================================================
--- modules/proxy/mod_proxy.c   (revision 1753164)
+++ modules/proxy/mod_proxy.c   (working copy)
@@ -1273,17 +1273,25 @@
      * the error page on the proxy or if the error was not generated by the
      * backend itself but by the proxy e.g. a bad gateway) in order to give
      * ap_proxy_post_request a chance to act correctly on the status code.
+     * But only do the above if access_status is not OK or DONE, because
+     * in this case r->status might contain the true status and overwriting
+     * it with OK or DONE would be wrong.
      */
-    saved_status = r->status;
-    r->status = access_status;
-    ap_proxy_post_request(worker, balancer, r, conf);
-    /*
-     * Only restore r->status if it has not been changed by
-     * ap_proxy_post_request as we assume that this change was intentional.
-     */
-    if (r->status == access_status) {
-        r->status = saved_status;
+    if ((access_status != OK) || (access_status != DONE)) {
+        saved_status = r->status;
+        r->status = access_status;
+        ap_proxy_post_request(worker, balancer, r, conf);
+        /*
+         * Only restore r->status if it has not been changed by
+         * ap_proxy_post_request as we assume that this change was intentional.
+         */
+        if (r->status == access_status) {
+            r->status = saved_status;
+        }
     }
+    else {
+        ap_proxy_post_request(worker, balancer, r, conf);
+    }

     proxy_run_request_status(&access_status, r);
     AP_PROXY_RUN_FINISHED(r, attempts, access_status);
Comment 2 Ruediger Pluem 2016-07-18 07:55:34 UTC
Created attachment 34051 [details]
Patch against trunk

Fix logical error in if confdition:

Index: modules/proxy/mod_proxy.c
===================================================================
--- modules/proxy/mod_proxy.c   (revision 1753164)
+++ modules/proxy/mod_proxy.c   (working copy)
@@ -1273,17 +1273,25 @@
      * the error page on the proxy or if the error was not generated by the
      * backend itself but by the proxy e.g. a bad gateway) in order to give
      * ap_proxy_post_request a chance to act correctly on the status code.
+     * But only do the above if access_status is not OK and not DONE, because
+     * in this case r->status might contain the true status and overwriting
+     * it with OK or DONE would be wrong.
      */
-    saved_status = r->status;
-    r->status = access_status;
-    ap_proxy_post_request(worker, balancer, r, conf);
-    /*
-     * Only restore r->status if it has not been changed by
-     * ap_proxy_post_request as we assume that this change was intentional.
-     */
-    if (r->status == access_status) {
-        r->status = saved_status;
+    if ((access_status != OK) && (access_status != DONE)) {
+        saved_status = r->status;
+        r->status = access_status;
+        ap_proxy_post_request(worker, balancer, r, conf);
+        /*
+         * Only restore r->status if it has not been changed by
+         * ap_proxy_post_request as we assume that this change was intentional.
+         */
+        if (r->status == access_status) {
+            r->status = saved_status;
+        }
     }
+    else {
+        ap_proxy_post_request(worker, balancer, r, conf);
+    }

     proxy_run_request_status(&access_status, r);
     AP_PROXY_RUN_FINISHED(r, attempts, access_status);
Comment 3 Nikita Akhnin 2016-07-18 10:28:03 UTC
Yes, the problem is fixed.

Can I expect that this patch will be included in the next release?
Comment 4 Ruediger Pluem 2016-07-20 18:26:32 UTC
Committed to trunk as r1753592. Backport proposal for 2.4.x will follow in a few days.
Comment 5 Ruediger Pluem 2016-07-26 09:59:26 UTC
Proposed for backport to 2.4.x as r1754075.
Comment 6 Yann Ylavic 2016-11-14 12:21:13 UTC
Backported to 2.4.24 in r1756562.