[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