[PATCH] mod_proxy run cleanup on balancer failure
Here is a trivial patch that will allow proxy_handler to run the request_status hook if pre_request fails. This is necessary if all balncer members are in an error state, so that other modules get a chance to recover from the error. In my case, I force the cache to serve old data. --- mod_proxy.c.orig2005-09-28 10:24:26.102380589 -0400 +++ mod_proxy.c 2005-09-28 10:24:58.443016349 -0400 @@ -680,7 +680,7 @@ /* 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; +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 */ -- Brian Akins Lead Systems Engineer CNN Internet Technologies
Re: [PATCH] mod_proxy run cleanup on balancer failure
Brian Akins wrote: > Here is a trivial patch that will allow proxy_handler to run the > request_status hook if pre_request fails. This is necessary if all > balncer members are in an error state, so that other modules get a If all workers are in error state, worker will be NULL. Are you sure that the post_request hook that is run in the balancer case is prepared for such a situation? So it might be saver to do something like this to solve your problem. Any comments from the proxy gurus? Index: mod_proxy.c === --- mod_proxy.c (Revision 280422) +++ mod_proxy.c (Arbeitskopie) @@ -680,7 +680,7 @@ /* 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; +goto run_request_status; if (balancer && balancer->max_attempts_set && !max_attempts) max_attempts = balancer->max_attempts; /* firstly, try a proxy, unless a NoProxy directive is active */ @@ -774,6 +774,7 @@ } } +run_request_status: proxy_run_request_status(&access_status, r); return access_status; [..cut..] Regards Rüdiger
Re: [PATCH] mod_proxy run cleanup on balancer failure
Brian Akins wrote:
Here is a trivial patch that will allow proxy_handler to run the
request_status hook if pre_request fails. This is necessary if all
balncer members are in an error state, so that other modules get a
chance to recover from the error. In my case, I force the cache to
serve old data.
if (access_status != OK)
-return access_status;
+goto cleanup;
goto cleanup; will actually run the request_status optional hook,
so the patch makes sense, although I would make it as follows:
if (access_status != OK) {
if (access_status != HTTP_SERVICE_UNAVAILABLE)
return access_status;
else
goto cleanup;
}
There is no need to run the request_status hook if the
balancer returns DECLINED, because in that case the URL is
not meant for balancer.
If there are no free endpoints or all are in error state,
the HTTP_SERVICE_UNAVAILABLE will be returned, and only
then calling the request_status hook makes sense.
Regards,
Mladen.
Re: [PATCH] mod_proxy run cleanup on balancer failure
On Sep 28, 2005, at 5:09 PM, [EMAIL PROTECTED] wrote: Brian Akins wrote: Here is a trivial patch that will allow proxy_handler to run the request_status hook if pre_request fails. This is necessary if all balncer members are in an error state, so that other modules get a If all workers are in error state, worker will be NULL. Are you sure that the post_request hook that is run in the balancer case is prepared for such a situation? So it might be saver to do something like this to solve your problem. Any comments from the proxy gurus? Hmmm... It might be best to, in ap_proxy_pre_request() set *balancer to NULL if the result from proxy_run_pre_request() isn't OK. Then we can call jump to cleanup. Also looks like there might be a bug in ap_proxy_pre_request() as well ~ line 1353... (balancer != NULL) will always be true I think... I think it should be *balancer checking
Re: [PATCH] mod_proxy run cleanup on balancer failure
Jim Jagielski wrote: > > On Sep 28, 2005, at 5:09 PM, [EMAIL PROTECTED] wrote: > >> >> >> Brian Akins wrote: >> [..cut..] > > Hmmm... It might be best to, in ap_proxy_pre_request() set > *balancer to NULL if the result from proxy_run_pre_request() > isn't OK. Then we can call jump to cleanup. Yes, this sounds like the better solution here. > > Also looks like there might be a bug in ap_proxy_pre_request() > as well ~ line 1353... (balancer != NULL) will always > be true I think... I think it should be *balancer Yes, I think you are right. It should be *balancer. Regards Rüdiger
Re: [PATCH] mod_proxy run cleanup on balancer failure
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.
--
Brian Akins
Lead Systems Engineer
CNN Internet Technologies
Re: [PATCH] mod_proxy run cleanup on balancer failure
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...
Re: [PATCH] mod_proxy run cleanup on balancer failure
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 */
Re: [PATCH] mod_proxy run cleanup on balancer failure
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
This means if access_status is DECLINED, we would return. Would
that work for you??
Re: [PATCH] mod_proxy run cleanup on balancer failure
Jim Jagielski wrote: This means if access_status is DECLINED, we would return. Would that work for you?? Yes. Just need to run request_status in case of error. -- Brian Akins Lead Systems Engineer CNN Internet Technologies
Re: [PATCH] mod_proxy run cleanup on balancer failure
Except for the formatting +1 :)
On Sep 29, 2005, at 12:01 PM, [EMAIL PROTECTED] wrote:
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 */
Re: [PATCH] mod_proxy run cleanup on balancer failure
As I try to improve my Apache code style awareness. What is wrong with the
formatting?
Regards
Rüdiger
Jim Jagielski wrote:
> Except for the formatting +1 :)
>
> On Sep 29, 2005, at 12:01 PM, [EMAIL PROTECTED] wrote:
>
>>
>> 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 */
>>
>>
>
>
>
Re: [PATCH] mod_proxy run cleanup on balancer failure
Sorry :)
The single line if statements. The pref is
if (foo)
banana();
rather than
if (foo) banana();
On Sep 29, 2005, at 2:32 PM, [EMAIL PROTECTED] wrote:
As I try to improve my Apache code style awareness. What is wrong
with the
formatting?
Regards
Rüdiger
Jim Jagielski wrote:
Except for the formatting +1 :)
On Sep 29, 2005, at 12:01 PM, [EMAIL PROTECTED] wrote:
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 */
Re: [PATCH] mod_proxy run cleanup on balancer failure
[EMAIL PROTECTED] wrote:
As I try to improve my Apache code style awareness. What is wrong with the
formatting?
http://httpd.apache.org/dev/styleguide.html
The basic objection to your patch would be that statements after
your if's; put them on the next line.
HTH,
Sander
Regards
Rüdiger
Jim Jagielski wrote:
Except for the formatting +1 :)
On Sep 29, 2005, at 12:01 PM, [EMAIL PROTECTED] wrote:
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 */
Re: [PATCH] mod_proxy run cleanup on balancer failure
Jim Jagielski wrote:
> Sorry :)
Thanks. So I guess this should be fine:
Index: mod_proxy.c
===
--- mod_proxy.c (Revision 280422)
+++ mod_proxy.c (Arbeitskopie)
@@ -679,8 +679,22 @@
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 */
[..cut..]
Re: [PATCH] mod_proxy run cleanup on balancer failure
+1
[EMAIL PROTECTED] wrote:
>
>
>
> Jim Jagielski wrote:
> > Sorry :)
>
> Thanks. So I guess this should be fine:
>
> Index: mod_proxy.c
> ===
> --- mod_proxy.c (Revision 280422)
> +++ mod_proxy.c (Arbeitskopie)
> @@ -679,8 +679,22 @@
> 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 */
>
> [..cut..]
>
--
===
Jim Jagielski [|] [EMAIL PROTECTED] [|] http://www.jaguNET.com/
"If you can dodge a wrench, you can dodge a ball."
Re: [PATCH] mod_proxy run cleanup on balancer failure
Can we get a vote on this? Jim gave it +1. So do I. I would like this
to be in a beta "release." This fix is important to our environment.
[EMAIL PROTECTED] wrote:
Jim Jagielski wrote:
Sorry :)
Thanks. So I guess this should be fine:
Index: mod_proxy.c
===
--- mod_proxy.c (Revision 280422)
+++ mod_proxy.c (Arbeitskopie)
@@ -679,8 +679,22 @@
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 */
[..cut..]
--
Brian Akins
Lead Systems Engineer
CNN Internet Technologies
Re: [PATCH] mod_proxy run cleanup on balancer failure
Brian Akins wrote: Can we get a vote on this? Jim gave it +1. So do I. I would like this to be in a beta "release." This fix is important to our environment. Never mind. See this in trunk... -- Brian Akins Lead Systems Engineer CNN Internet Technologies
Re: [PATCH] mod_proxy run cleanup on balancer failure
On 10/28/2005 03:46 PM, Brian Akins wrote: > Brian Akins wrote: > >> Can we get a vote on this? Jim gave it +1. So do I. I would like >> this to be in a beta "release." This fix is important to our >> environment. > > > > Never mind. See this in trunk... It is also backported to 2.2.x. So it will be part of 2.1.9 Regards Rüdiger
