Jim Jagielski wrote:
>
> On Sep 29, 2005, at 9:40 AM, Brian Akins wrote:
>
>> Mladen Turk wrote:
>>
>>
>>> if (access_status != OK) {
>>> if (access_status != HTTP_SERVICE_UNAVAILABLE)
>>> return access_status;
>>> else
>>> goto cleanup;
>>> }
>>>
>>
>> I guess that makes sense. I just want to chatch the following cases:
>>
>> No available workers - all in error or busy
>> worker failed - ie connect timeout
>> random weirdness - for whatever reason we are going to return
>> HTTP_BAD_GATEWAY
>>
>>
>> The goal is, if possible, in request_status, a developer may try to
>> "catch" the error so that the end user does not see it. In our
>> situation, for example, we would rather return a stale page rather
>> than a BAD GATEWAY.
>>
>
> The rub, as mentioned, is to ensure that when jumping to
> cleanup, balancer is never non-NULL with a NULL
> worker, so we need to check if that ever happens. I still think
> it's safer to, in ap_proxy_pre_request, force *balancer
> to NULL if we ever return something other than OK...
>
>
What about the following patch? I think it should address all the things
discussed.
Regards
Rüdiger
Index: mod_proxy.c
===================================================================
--- mod_proxy.c (Revision 280422)
+++ mod_proxy.c (Arbeitskopie)
@@ -679,8 +679,20 @@
char *url = uri;
/* Try to obtain the most suitable worker */
access_status = ap_proxy_pre_request(&worker, &balancer, r, conf,
&url);
- if (access_status != OK)
- return access_status;
+ if (access_status != OK) {
+ /*
+ * Only return if access_status is not HTTP_SERVICE_UNAVAILABLE
+ * This gives other modules the chance to hook into the
+ * request_status hook and decide what to do in this situation.
+ */
+ if (access_status != HTTP_SERVICE_UNAVAILABLE) return
access_status;
+ /*
+ * Ensure that balancer is NULL if worker is NULL to prevent
+ * potential problems in the post_request hook.
+ */
+ if (!worker) balancer = NULL;
+ goto cleanup;
+ }
if (balancer && balancer->max_attempts_set && !max_attempts)
max_attempts = balancer->max_attempts;
/* firstly, try a proxy, unless a NoProxy directive is active */
