Re: AW: AW: AW: 2.2 mod_http_proxy and "partial" pages
On 01/10/2006 02:11 PM, William A. Rowe, Jr. wrote: > Plüm wrote: > >>> William A. Rowe, Jr. wrote: >>> Ruediger Pluem wrote: [..cut..] > > > The straightforward thing is to close the client socket. Obviously it's > not that trivial; unix can reuse the same fd almost immediately. Perhaps > close the write side? In any case, the connection should be marked > aborted. > > Number 2 follows from 1, of course we don't 'finish' the response. > > Remember, the back end is *busted* midstream. We have to convey that, but > we don't have to maintain the integrety of every byte sent by the (errant) > server, IMHO. In the case of the broken backend the patch already does this. Do I understand you correctly that the behaviour currently done for a broken backend should be applied to *any* type of error bucket? [..cut..] > as you might have guessed, my comments were aimed at those 'interesting' > applications that were otherwise cacheable - e.g. those folks who keep > arguing that things like mod_authnz_hostname should interact with the cache > (which yes I disagree with, but this would provide basic mechanisms to > handle > this case.) I think we disagree on this as I also belong to those folks, but thats another story and thread :-). Furthermore I do not see right now how this helps in the 'mod_authnz_hostname case' as the response should still be *cached*. It shouldn't be delivered from the cache without checking. But as said this point does not belong to this thread. [..cut..] Apart from getting very welcome improvement feedback I also wanted to check if someone is -1 on the patch. As I regard the "partial pages problem" as something that should be fixed in 2.2.1 I will propose it for backport once Brian did a successful test with the patch and I do not see any -1 on it. Regards Rüdiger
Re: AW: AW: AW: 2.2 mod_http_proxy and "partial" pages
Plüm wrote: William A. Rowe, Jr. wrote: Ruediger Pluem wrote: [..cut..] Quick consideration; Rather than look for HTTP_BAD_GATEWAY error bucket, we can actually generalize the problem. ANY metadata bucket that isn't recognized and handled by an intermediate filter probably indiciates a problem; and therefore the result is a non-cacheable, broken response. Actually two cases. In the error bucket case, it's non-cacheable, and broken. So what do you think should be done in this case with respect to the 'brokenness'. 1. set r->connection->keepalive to AP_CONN_CLOSE 2. Do not sent the last chunk marker in the case of a chunked encoding response 3. Do 1. and 2. The straightforward thing is to close the client socket. Obviously it's not that trivial; unix can reuse the same fd almost immediately. Perhaps close the write side? In any case, the connection should be marked aborted. Number 2 follows from 1, of course we don't 'finish' the response. Remember, the back end is *busted* midstream. We have to convey that, but we don't have to maintain the integrety of every byte sent by the (errant) server, IMHO. Next question is: Do we need to stop sending further data to the client immediately? Yup, again IMHO. In the unrecognized bucket type case, it's non-cacheable (a 'complex' response), but it is likely serveable to the front end client. In both cases, if mod_cache doesn't grok what it sees, then something 'interesting' is going on and we would not want to deposit into the cache. I agree with the goals, but making it non cacheable is not easy to add to the current patch, because the HTTP_OUTERROR filter is a protocol filter that is run after the CACHE_SAVE filter. My comments were ment to illustrate that a new filter is a waste of resources, stack setup and teardown. We've overengineered what (should be) some simple code. Delegating error handling which should occur in the (existing) filter stack, previously, off to it's own filter is (again IMHO) sort of lame :) But apart from the case that no content-length is present the CACHE_SAVE filter itself does not iterate over the brigade. So we would need to add an additional loop over the brigade inside of CACHE_SAVE filter to scan for these meta buckets. Yes; there is some complexity here. Didn't suggest it was trivial :-) Furthermore I think we need to keep in mind that, if we think that this reponse is not worth caching, we may should make any upstream proxies think the same. In the case of a broken backend this is reached (depending on the transfer encoding) by 1. sending less content then the content-length header announces 2. do not send the last-chunk marker. right. But in the case of an unrecognized bucket type we must let the upstream proxies know that it is not cacheable via headers. as you might have guessed, my comments were aimed at those 'interesting' applications that were otherwise cacheable - e.g. those folks who keep arguing that things like mod_authnz_hostname should interact with the cache (which yes I disagree with, but this would provide basic mechanisms to handle this case.) > But this could be impossible if the headers had been already sent. exactly so.
AW: AW: AW: 2.2 mod_http_proxy and "partial" pages
> > William A. Rowe, Jr. wrote: > > Ruediger Pluem wrote: [..cut..] > > > > Quick consideration; > > > > Rather than look for HTTP_BAD_GATEWAY error bucket, we can actually > > generalize the problem. ANY metadata bucket that isn't > recognized and > > handled by an intermediate filter probably indiciates a problem; and > > therefore the result is a non-cacheable, broken response. > > Actually two cases. In the error bucket case, it's > non-cacheable, and broken. So what do you think should be done in this case with respect to the 'brokenness'. 1. set r->connection->keepalive to AP_CONN_CLOSE 2. Do not sent the last chunk marker in the case of a chunked encoding response 3. Do 1. and 2. Next question is: Do we need to stop sending further data to the client immediately? > In the unrecognized bucket type case, it's non-cacheable (a > 'complex' response), > but it is likely serveable to the front end client. In both > cases, if mod_cache > doesn't grok what it sees, then something 'interesting' is > going on and we would > not want to deposit into the cache. I agree with the goals, but making it non cacheable is not easy to add to the current patch, because the HTTP_OUTERROR filter is a protocol filter that is run after the CACHE_SAVE filter. So setting r->no_cache there may be too late in the case that the error bucket and eos bucket are in the same brigade. This is the reason why we actually set r->no_cache on the proxy side in ap_proxy_backend_broke which is called from the scheme handlers. >From my current perspective this would mean that the CACHE_SAVE filter must be taught to deal with these buckets. But apart from the case that no content-length is present the CACHE_SAVE filter itself does not iterate over the brigade. So we would need to add an additional loop over the brigade inside of CACHE_SAVE filter to scan for these meta buckets. Furthermore I think we need to keep in mind that, if we think that this reponse is not worth caching, we may should make any upstream proxies think the same. In the case of a broken backend this is reached (depending on the transfer encoding) by 1. sending less content then the content-length header announces 2. do not send the last-chunk marker. But in the case of an unrecognized bucket type we must let the upstream proxies know that it is not cacheable via headers. But this could be impossible if the headers had been already sent. Regards Rüdiger
Re: AW: AW: AW: 2.2 mod_http_proxy and "partial" pages
William A. Rowe, Jr. wrote: Rather than look for HTTP_BAD_GATEWAY error bucket, we can actually generalize the problem. ANY metadata bucket that isn't recognized and handled by an intermediate filter probably indiciates a problem; and therefore the result is a non-cacheable, broken response. +1. Regards, Graham -- smime.p7s Description: S/MIME Cryptographic Signature
Re: AW: AW: AW: 2.2 mod_http_proxy and "partial" pages
William A. Rowe, Jr. wrote: Ruediger Pluem wrote: Thanks to Jim for reviewing the patch. He detected one missed patch and made some comments in the code clearer. The new patch list now: Quick consideration; Rather than look for HTTP_BAD_GATEWAY error bucket, we can actually generalize the problem. ANY metadata bucket that isn't recognized and handled by an intermediate filter probably indiciates a problem; and therefore the result is a non-cacheable, broken response. Actually two cases. In the error bucket case, it's non-cacheable, and broken. In the unrecognized bucket type case, it's non-cacheable (a 'complex' response), but it is likely serveable to the front end client. In both cases, if mod_cache doesn't grok what it sees, then something 'interesting' is going on and we would not want to deposit into the cache. This would guard against new designs causing new troublesome interactions with mod_cache. As some new bucket type is 'taught' to mod_cache, it can grow to handle otherwise harmless requests. But I suspect that the module should err on the side of paranoia. Bill
Re: AW: AW: AW: 2.2 mod_http_proxy and "partial" pages
Ruediger Pluem wrote: Thanks to Jim for reviewing the patch. He detected one missed patch and made some comments in the code clearer. The new patch list now: Quick consideration; Rather than look for HTTP_BAD_GATEWAY error bucket, we can actually generalize the problem. ANY metadata bucket that isn't recognized and handled by an intermediate filter probably indiciates a problem; and therefore the result is a non-cacheable, broken response. Thoughts? Bill
Re: AW: AW: AW: 2.2 mod_http_proxy and "partial" pages
On 01/05/2006 08:59 PM, Ruediger Pluem wrote:
>
> On 01/05/2006 01:51 PM, Ruediger Pluem wrote:
>
> [..cut..]
>
> I finally merged all the commits done to the trunk on this issue
Thanks to Jim for reviewing the patch. He detected one missed patch
and made some comments in the code clearer. The new patch list now:
r354628
r354636
r357461
r357519
r358022 <- new
r365374
r366181
r366554 <- new
r366558 <- new
I created a new merged patch that works with 2.2.x. So the same procedure
again please :).
I would like Brian (Akins) to give it a try and of course if someone else has a
look on it, it wouldn't hurt.
Regards
Rüdiger
Index: modules/http/http_core.c
===
--- modules/http/http_core.c(Revision 366698)
+++ modules/http/http_core.c(Arbeitskopie)
@@ -39,6 +39,7 @@
AP_DECLARE_DATA ap_filter_rec_t *ap_http_input_filter_handle;
AP_DECLARE_DATA ap_filter_rec_t *ap_http_header_filter_handle;
AP_DECLARE_DATA ap_filter_rec_t *ap_chunk_filter_handle;
+AP_DECLARE_DATA ap_filter_rec_t *ap_http_outerror_filter_handle;
AP_DECLARE_DATA ap_filter_rec_t *ap_byterange_filter_handle;
static const char *set_keep_alive_timeout(cmd_parms *cmd, void *dummy,
@@ -202,6 +203,8 @@
NULL, r, r->connection);
ap_add_output_filter_handle(ap_http_header_filter_handle,
NULL, r, r->connection);
+ap_add_output_filter_handle(ap_http_outerror_filter_handle,
+NULL, r, r->connection);
}
return OK;
@@ -237,6 +240,9 @@
ap_chunk_filter_handle =
ap_register_output_filter("CHUNK", ap_http_chunk_filter,
NULL, AP_FTYPE_TRANSCODE);
+ap_http_outerror_filter_handle =
+ap_register_output_filter("HTTP_OUTERROR", ap_http_outerror_filter,
+ NULL, AP_FTYPE_PROTOCOL);
ap_byterange_filter_handle =
ap_register_output_filter("BYTERANGE", ap_byterange_filter,
NULL, AP_FTYPE_PROTOCOL);
Index: modules/http/http_filters.c
===
--- modules/http/http_filters.c (Revision 366698)
+++ modules/http/http_filters.c (Arbeitskopie)
@@ -1318,3 +1318,29 @@
return bufsiz;
}
+/* Filter to handle any error buckets on output */
+apr_status_t ap_http_outerror_filter(ap_filter_t *f,
+ apr_bucket_brigade *b)
+{
+request_rec *r = f->r;
+apr_bucket *e;
+
+for (e = APR_BRIGADE_FIRST(b);
+ e != APR_BRIGADE_SENTINEL(b);
+ e = APR_BUCKET_NEXT(e))
+{
+if (AP_BUCKET_IS_ERROR(e)) {
+/*
+ * Start of error handling state tree. Just one condition
+ * right now :)
+ */
+if (((ap_bucket_error *)(e->data))->status == HTTP_BAD_GATEWAY) {
+/* stream aborted and we have not ended it yet */
+r->connection->keepalive = AP_CONN_CLOSE;
+}
+}
+}
+
+return ap_pass_brigade(f->next, b);
+}
+
Index: modules/http/mod_core.h
===
--- modules/http/mod_core.h (Revision 366698)
+++ modules/http/mod_core.h (Arbeitskopie)
@@ -42,6 +42,7 @@
extern AP_DECLARE_DATA ap_filter_rec_t *ap_http_input_filter_handle;
extern AP_DECLARE_DATA ap_filter_rec_t *ap_http_header_filter_handle;
extern AP_DECLARE_DATA ap_filter_rec_t *ap_chunk_filter_handle;
+extern AP_DECLARE_DATA ap_filter_rec_t *ap_http_outerror_filter_handle;
extern AP_DECLARE_DATA ap_filter_rec_t *ap_byterange_filter_handle;
/*
@@ -54,6 +55,10 @@
/* HTTP/1.1 chunked transfer encoding filter. */
apr_status_t ap_http_chunk_filter(ap_filter_t *f, apr_bucket_brigade *b);
+/* Filter to handle any error buckets on output */
+apr_status_t ap_http_outerror_filter(ap_filter_t *f,
+ apr_bucket_brigade *b);
+
char *ap_response_code_string(request_rec *r, int error_index);
/**
Index: modules/http/chunk_filter.c
===
--- modules/http/chunk_filter.c (Revision 366698)
+++ modules/http/chunk_filter.c (Arbeitskopie)
@@ -39,6 +39,12 @@
#include "mod_core.h"
+/*
+ * A pointer to this is used to memorize in the filter context that a bad
+ * gateway error bucket had been seen. It is used as an invented unique
pointer.
+ */
+static char bad_gateway_seen;
+
apr_status_t ap_http_chunk_filter(ap_filter_t *f, apr_bucket_brigade *b)
{
#define ASCII_CRLF "\015\012"
@@ -67,6 +73,16 @@
eos = e;
break;
}
+if (AP_BUCKET_IS_ERROR(e)
+&& (((ap_bucket_error *)(e->data))->status
+== HTTP_BAD_GATEWAY)) {
+/*
+ * We had a broken backend. Memorize this in the filt
Re: AW: AW: AW: 2.2 mod_http_proxy and "partial" pages
Ruediger Pluem wrote: > > I think with the adjustments you made to the comments it is now much > clearer what is done and this point is closed. Thanks for doing this. > np :) -- === Jim Jagielski [|] [EMAIL PROTECTED] [|] http://www.jaguNET.com/ "If you can dodge a wrench, you can dodge a ball."
Re: AW: AW: AW: 2.2 mod_http_proxy and "partial" pages
On 01/06/2006 08:03 PM, Jim Jagielski wrote: > > On Jan 6, 2006, at 1:47 PM, Jim Jagielski wrote: > [..cut..] >> > > I should clarify that: when the comment says "or" yet the > code does an "and" then it causes undue confusion, even > if the 2 do sync up. I think with the adjustments you made to the comments it is now much clearer what is done and this point is closed. Thanks for doing this. Regards Rüdiger
Re: AW: AW: AW: 2.2 mod_http_proxy and "partial" pages
On 01/06/2006 07:47 PM, Jim Jagielski wrote:
> Still not sure why you are using a specific error detection
> filter rather than the generic one in -trunk
>
Arghh. Sorry I must have missed to include your patch that changed this.
Thanks for catching this. I will provide a new patch.
[..cut..]
>> +/*
>> + * Ensure that we sent an EOS bucket thru the filter chain, if
>> we already
>> + * have sent some data. Maybe ap_proxy_backend_broke was called
>> and added
>> + * one to the brigade already. So we should not do this in this
>> case.
>> + */
>> +if (data_sent && !r->eos_sent && APR_BRIGADE_EMPTY
>> (output_brigade)) {
>> +e = apr_bucket_eos_create(r->connection->bucket_alloc);
>> +APR_BRIGADE_INSERT_TAIL(output_brigade, e);
>> +}
>> +
>>
>
> Also, if data_sent is true, then ap_proxy_backend_broke() already
> sent the EOS, so why are we checking if it's true again? I
> think the logic is wrong...
No, there is also the case that the client aborted the connection.
In this case
status == APR_SUCCESS
So the outer if condition around ap_proxy_backend_broke is not true and it is
not called. Anyway since we had sent some data we should sent an EOS if we
have not done already to make all filters on the chain aware of it.
Especially mod_disk_cache needs to know in order to remove the temporary
file created for the cache entry.
Regards
Rüdiger
Re: AW: AW: AW: 2.2 mod_http_proxy and "partial" pages
On Jan 6, 2006, at 1:47 PM, Jim Jagielski wrote:
Still not sure why you are using a specific error detection
filter rather than the generic one in -trunk
On Jan 5, 2006, at 2:59 PM, Ruediger Pluem wrote:
@@ -146,13 +162,20 @@
* 2) the trailer
* 3) the end-of-chunked body CRLF
*
- * If there is no EOS bucket, then do nothing.
+ * If there is no EOS bucket, or if we had seen an error
bucket with
+ * status HTTP_BAD_GATEWAY then do nothing. We have
memorized an
+ * error bucket that we had seen in the filter context.
+ * The error bucket with status HTTP_BAD_GATEWAY
indicates that the
+ * connection to the backend (mod_proxy) broke in the
middle of the
+ * response. In order to signal the client that something
went wrong
+ * we do not create the last-chunk marker and set c-
>keepalive to
+ * AP_CONN_CLOSE in the core output filter.
*
* XXX: it would be nice to combine this with the end-of-
chunk
* marker above, but this is a bit more straight-forward for
* now.
*/
-if (eos != NULL) {
+if (eos && !f->ctx) {
Code logic doesn't match comment.
I should clarify that: when the comment says "or" yet the
code does an "and" then it causes undue confusion, even
if the 2 do sync up.
Re: AW: AW: AW: 2.2 mod_http_proxy and "partial" pages
Still not sure why you are using a specific error detection
filter rather than the generic one in -trunk
On Jan 5, 2006, at 2:59 PM, Ruediger Pluem wrote:
@@ -146,13 +162,20 @@
* 2) the trailer
* 3) the end-of-chunked body CRLF
*
- * If there is no EOS bucket, then do nothing.
+ * If there is no EOS bucket, or if we had seen an error
bucket with
+ * status HTTP_BAD_GATEWAY then do nothing. We have
memorized an
+ * error bucket that we had seen in the filter context.
+ * The error bucket with status HTTP_BAD_GATEWAY indicates
that the
+ * connection to the backend (mod_proxy) broke in the
middle of the
+ * response. In order to signal the client that something
went wrong
+ * we do not create the last-chunk marker and set c-
>keepalive to
+ * AP_CONN_CLOSE in the core output filter.
*
* XXX: it would be nice to combine this with the end-of-
chunk
* marker above, but this is a bit more straight-forward for
* now.
*/
-if (eos != NULL) {
+if (eos && !f->ctx) {
Code logic doesn't match comment.
+/*
+ * Ensure that we sent an EOS bucket thru the filter chain, if
we already
+ * have sent some data. Maybe ap_proxy_backend_broke was
called and added
+ * one to the brigade already. So we should not do this in
this case.
+ */
+if (data_sent && !r->eos_sent && APR_BRIGADE_EMPTY
(output_brigade)) {
+e = apr_bucket_eos_create(r->connection->bucket_alloc);
+APR_BRIGADE_INSERT_TAIL(output_brigade, e);
+}
+
Also, if data_sent is true, then ap_proxy_backend_broke() already
sent the EOS, so why are we checking if it's true again? I
think the logic is wrong...
Re: AW: AW: AW: 2.2 mod_http_proxy and "partial" pages
On 01/05/2006 01:51 PM, Ruediger Pluem wrote:
[..cut..]
I finally merged all the commits done to the trunk on this issue
r354628
r354636
r357461
r357519
r365374
r366181
into one patch that works with 2.2.x. From my current point of view all
aspects of this issue should be considered by this patch.
I would like Brian (Akins) to give it a try and of course if someone else has a
look on it, it wouldn't hurt.
Regards
Rüdiger
Index: modules/http/http_core.c
===
--- modules/http/http_core.c(Revision 357457)
+++ modules/http/http_core.c(Arbeitskopie)
@@ -39,6 +39,7 @@
AP_DECLARE_DATA ap_filter_rec_t *ap_http_input_filter_handle;
AP_DECLARE_DATA ap_filter_rec_t *ap_http_header_filter_handle;
AP_DECLARE_DATA ap_filter_rec_t *ap_chunk_filter_handle;
+AP_DECLARE_DATA ap_filter_rec_t *ap_broken_backend_filter_handle;
AP_DECLARE_DATA ap_filter_rec_t *ap_byterange_filter_handle;
static const char *set_keep_alive_timeout(cmd_parms *cmd, void *dummy,
@@ -237,6 +238,10 @@
ap_chunk_filter_handle =
ap_register_output_filter("CHUNK", ap_http_chunk_filter,
NULL, AP_FTYPE_TRANSCODE);
+ap_broken_backend_filter_handle =
+ap_register_output_filter("BROKEN_BACKEND",
+ ap_http_broken_backend_filter,
+ NULL, AP_FTYPE_PROTOCOL);
ap_byterange_filter_handle =
ap_register_output_filter("BYTERANGE", ap_byterange_filter,
NULL, AP_FTYPE_PROTOCOL);
Index: modules/http/http_filters.c
===
--- modules/http/http_filters.c (Revision 357457)
+++ modules/http/http_filters.c (Arbeitskopie)
@@ -1043,6 +1043,9 @@
*/
ap_add_output_filter("CHUNK", NULL, r, r->connection);
}
+/* If we have a Proxy request, add the BROKEN_BACKEND filter now */
+if (r->proxyreq != PROXYREQ_NONE)
+ap_add_output_filter("BROKEN_BACKEND", NULL, r, r->connection);
/* Don't remove this filter until after we have added the CHUNK filter.
* Otherwise, f->next won't be the CHUNK filter and thus the first
@@ -1318,3 +1321,23 @@
return bufsiz;
}
+apr_status_t ap_http_broken_backend_filter(ap_filter_t *f,
+ apr_bucket_brigade *b)
+{
+request_rec *r = f->r;
+apr_bucket *e;
+
+for (e = APR_BRIGADE_FIRST(b);
+ e != APR_BRIGADE_SENTINEL(b);
+ e = APR_BUCKET_NEXT(e))
+{
+if (AP_BUCKET_IS_ERROR(e)
+&& (((ap_bucket_error *)(e->data))->status == HTTP_BAD_GATEWAY)) {
+/* stream aborted and we have not ended it yet */
+r->connection->keepalive = AP_CONN_CLOSE;
+}
+}
+
+return ap_pass_brigade(f->next, b);
+}
+
Index: modules/http/mod_core.h
===
--- modules/http/mod_core.h (Revision 357457)
+++ modules/http/mod_core.h (Arbeitskopie)
@@ -42,6 +42,7 @@
extern AP_DECLARE_DATA ap_filter_rec_t *ap_http_input_filter_handle;
extern AP_DECLARE_DATA ap_filter_rec_t *ap_http_header_filter_handle;
extern AP_DECLARE_DATA ap_filter_rec_t *ap_chunk_filter_handle;
+extern AP_DECLARE_DATA ap_filter_rec_t *ap_broken_backend_filter_handle;
extern AP_DECLARE_DATA ap_filter_rec_t *ap_byterange_filter_handle;
/*
@@ -54,6 +55,10 @@
/* HTTP/1.1 chunked transfer encoding filter. */
apr_status_t ap_http_chunk_filter(ap_filter_t *f, apr_bucket_brigade *b);
+/* Filter to close the connection to the client if backend broke */
+apr_status_t ap_http_broken_backend_filter(ap_filter_t *f,
+ apr_bucket_brigade *b);
+
char *ap_response_code_string(request_rec *r, int error_index);
/**
Index: modules/http/chunk_filter.c
===
--- modules/http/chunk_filter.c (Revision 357457)
+++ modules/http/chunk_filter.c (Arbeitskopie)
@@ -39,6 +39,12 @@
#include "mod_core.h"
+/*
+ * A pointer to this is used to memorize in the filter context that a bad
+ * gateway error bucket had been seen. It is used as an invented unique
pointer.
+ */
+static char bad_gateway_seen;
+
apr_status_t ap_http_chunk_filter(ap_filter_t *f, apr_bucket_brigade *b)
{
#define ASCII_CRLF "\015\012"
@@ -67,6 +73,16 @@
eos = e;
break;
}
+if (AP_BUCKET_IS_ERROR(e)
+&& (((ap_bucket_error *)(e->data))->status
+== HTTP_BAD_GATEWAY)) {
+/*
+ * We had a broken backend. Memorize this in the filter
+ * context.
+ */
+f->ctx = &bad_gateway_seen;
+continue;
+}
if (APR_BUCKET_IS_FLUSH(e)) {
flush = e;
more = apr_brigade_split(b,
Re: AW: AW: AW: 2.2 mod_http_proxy and "partial" pages
On Thursday 05 January 2006 11:58, Joe Orton wrote: > On Thu, Jan 05, 2006 at 12:48:25PM +0100, Ruediger Pluem wrote: > > But I remember myself that there had been casting issues in the past that > > created compiler warnings especially with gcc 4. The patch below compiles > > fine with my gcc 3.2.2 with -Wall. So if someone could give a comment > > if > > > > f->ctx = (void *)(1) > > > > is fine it would be great. > > André's trick of using invented unique pointers by doing: > > static char sentinel; (in global scope) > > f->ctx = &sentinel; > > is neater and avoids the casting mess. (pick a suitable variable name > though :) Why go to the trouble of inventing a pointer? You already have unique pointers in global scope. An obvious example: module some_module; f->ctx = &some_module; -- Nick Kew
Re: AW: AW: AW: 2.2 mod_http_proxy and "partial" pages
On 01/05/2006 12:58 PM, Joe Orton wrote: > On Thu, Jan 05, 2006 at 12:48:25PM +0100, Ruediger Pluem wrote: [..cut..] > > André's trick of using invented unique pointers by doing: > > static char sentinel; (in global scope) > > f->ctx = &sentinel; > > is neater and avoids the casting mess. (pick a suitable variable name > though :) Thanks Joe for *pointing* me to this. :-) This is really nice trick. I will use it. Regards Rüdiger
Re: AW: AW: AW: 2.2 mod_http_proxy and "partial" pages
On Thu, Jan 05, 2006 at 12:48:25PM +0100, Ruediger Pluem wrote: > But I remember myself that there had been casting issues in the past that > created compiler warnings especially with gcc 4. The patch below compiles > fine with my gcc 3.2.2 with -Wall. So if someone could give a comment > if > > f->ctx = (void *)(1) > > is fine it would be great. André's trick of using invented unique pointers by doing: static char sentinel; (in global scope) f->ctx = &sentinel; is neater and avoids the casting mess. (pick a suitable variable name though :) joe
Re: AW: AW: AW: 2.2 mod_http_proxy and "partial" pages
On 01/03/2006 06:15 PM, Jim Jagielski wrote:
>
> On Jan 2, 2006, at 4:18 PM, Ruediger Pluem wrote:
>
>>
>> 1. Proposal
>> If a subrequest has a broken backend also set r->no_cache for the
>> main request
>> and ensure that the chunk filter does not sent the last chunk marker
>> in this case.
>>
>> 2. Proposal
>> If a subrequest has a broken backend do not sent the error bucket.
>> Only set r->no_cache
>> to ensure that this subrequest response does not get cached.
>>
>
I am currently working on proposal #1. In order to ensure that the chunk
filter does not sent a last chunk marker in this case I try to memorize
a seen error bucket in the filter context.
But I remember myself that there had been casting issues in the past that
created compiler warnings especially with gcc 4. The patch below compiles
fine with my gcc 3.2.2 with -Wall. So if someone could give a comment
if
f->ctx = (void *)(1)
is fine it would be great.
Regards
Rüdiger
Index: chunk_filter.c
===
--- chunk_filter.c (Revision 366160)
+++ chunk_filter.c (Arbeitskopie)
@@ -47,7 +47,6 @@
apr_bucket_brigade *more;
apr_bucket *e;
apr_status_t rv;
-int bad_gateway_seen = 0;
for (more = NULL; b; b = more, more = NULL) {
apr_off_t bytes = 0;
@@ -71,8 +70,11 @@
if (AP_BUCKET_IS_ERROR(e)
&& (((ap_bucket_error *)(e->data))->status
== HTTP_BAD_GATEWAY)) {
-/* We had a broken backend. Memorize this. */
-bad_gateway_seen = 1;
+/*
+ * We had a broken backend. Memorize this in the filter
+ * context.
+ */
+f->ctx = (void *)(1);
continue;
}
if (APR_BUCKET_IS_FLUSH(e)) {
@@ -155,7 +157,8 @@
* 3) the end-of-chunked body CRLF
*
* If there is no EOS bucket, or if we had seen an error bucket with
- * status HTTP_BAD_GATEWAY then do nothing.
+ * status HTTP_BAD_GATEWAY then do nothing. We have memorized an
+ * error bucket that we had seen in the filter context.
* The error bucket with status HTTP_BAD_GATEWAY indicates that the
* connection to the backend (mod_proxy) broke in the middle of the
* response. In order to signal the client that something went wrong
@@ -166,7 +169,7 @@
* marker above, but this is a bit more straight-forward for
* now.
*/
-if (eos && !bad_gateway_seen) {
+if (eos && !f->ctx) {
/* XXX: (2) trailers ... does not yet exist */
e = apr_bucket_immortal_create(ASCII_ZERO ASCII_CRLF
/* */
Re: AW: AW: AW: 2.2 mod_http_proxy and "partial" pages
On Jan 2, 2006, at 4:18 PM, Ruediger Pluem wrote: 1. Proposal If a subrequest has a broken backend also set r->no_cache for the main request and ensure that the chunk filter does not sent the last chunk marker in this case. 2. Proposal If a subrequest has a broken backend do not sent the error bucket. Only set r->no_cache to ensure that this subrequest response does not get cached. Proposal #1
Re: AW: AW: AW: 2.2 mod_http_proxy and "partial" pages
On 1/3/06, Ruediger Pluem <[EMAIL PROTECTED]> wrote: > >>2. Proposal > >>If a subrequest has a broken backend do not sent the error bucket. Only > >>set r->no_cache to ensure that this subrequest response does not get > >>cached. > > > > > > I think we still need to ensure that an error bucket is sent too, right? > > Otherwise, the connection will be reused - what am I missing? -- justin > > No, you are not missing anything. The question to me was: Do we need to close > a keepalive on the main request just because a subrequest failed in the middle > of the response? > Or to be more precise: Should the behaviour to cut off the keepalive be the > default > behaviour in such cases with the chance for subrequest creators to remove the > error > bucket and to make the response cacheable again or should it be the other way > round > that the subrequest creator is reponsible for preventing caching and closing > the > keepalive by sending the error bucket by himself if he thinks that this is > needed? > While writing this I personally come to the conclusion that the 1. proposal > (sending the error bucket) is saver as a default behaviour. Oh, I didn't realize you intended it as an either/or scenario. Then, yes, I agree that #1 is correct. =) -- justin
Re: AW: AW: AW: 2.2 mod_http_proxy and "partial" pages
On 01/03/2006 03:52 AM, Justin Erenkrantz wrote: > On Mon, Jan 02, 2006 at 10:18:19PM +0100, Ruediger Pluem wrote: > [..cut..] > >>2. Proposal >>If a subrequest has a broken backend do not sent the error bucket. Only >>set r->no_cache to ensure that this subrequest response does not get >>cached. > > > I think we still need to ensure that an error bucket is sent too, right? > Otherwise, the connection will be reused - what am I missing? -- justin No, you are not missing anything. The question to me was: Do we need to close a keepalive on the main request just because a subrequest failed in the middle of the response? Or to be more precise: Should the behaviour to cut off the keepalive be the default behaviour in such cases with the chance for subrequest creators to remove the error bucket and to make the response cacheable again or should it be the other way round that the subrequest creator is reponsible for preventing caching and closing the keepalive by sending the error bucket by himself if he thinks that this is needed? While writing this I personally come to the conclusion that the 1. proposal (sending the error bucket) is saver as a default behaviour. Regards Rüdiger
Re: AW: AW: AW: 2.2 mod_http_proxy and "partial" pages
On Mon, Jan 02, 2006 at 10:18:19PM +0100, Ruediger Pluem wrote: > Before I take any further actions I would like to discuss the desired > behaviour > in the subrequest case: > > 1. Proposal > If a subrequest has a broken backend also set r->no_cache for the main > request and ensure that the chunk filter does not sent the last chunk > marker in this case. +1. > 2. Proposal > If a subrequest has a broken backend do not sent the error bucket. Only > set r->no_cache to ensure that this subrequest response does not get > cached. I think we still need to ensure that an error bucket is sent too, right? Otherwise, the connection will be reused - what am I missing? -- justin
Re: AW: AW: AW: 2.2 mod_http_proxy and "partial" pages
On 12/20/2005 10:14 PM, Ruediger Pluem wrote: [..cut..] > But you pointed me to an interesting thing: If the main response is > T-E chunked and the backend error happened during the subrequest, the > chunked filter may sometimes add the last-chunk marker (if the brigade > containing the error bucket does *not* contain and eos bucket) and > sometimes not (if the brigade containing the error bucket does contain > an eos bucket). > In the case that the broken backend happend on the main request the brigade > always contains both buckets as they both get added to the brigade. But in > the subrequest case I guess the eos bucket (of the subrequest) gets removed > and the main request adds its own one later and maybe to a different brigade > then the error bucket. Meanwhile I took some time to investigate this further. I checked with the subrequests caused by mod_include's #include virtual tag. In this case the error bucket and the eos bucket seems to get passed to the chunk filter on different brigades. This means the chunk filter does sent the last chunk marker in this case. OTOH the error bucket causes the keepalive connection on the main request to be closed and the partially generated page of mod_include gets cached. Before I take any further actions I would like to discuss the desired behaviour in the subrequest case: 1. Proposal If a subrequest has a broken backend also set r->no_cache for the main request and ensure that the chunk filter does not sent the last chunk marker in this case. 2. Proposal If a subrequest has a broken backend do not sent the error bucket. Only set r->no_cache to ensure that this subrequest response does not get cached. Further proposals are welcome. Furthermore I am wondering if the current behaviour of mod_include is correct. Shouldn't we prevent caching if any of the mod_include operations got wrong? The broken backend for the #include virtual tag is only one example for this. And if we decide that we should not cache in this situation, how do we let external caches know? Should we also do not sent the last chunk marker in this case? > I am even getting unsure if the brigade always contains error and eos bucket > in the main request case, as there might happen an brigade split on the way > to the chunk filter. Does anybody know if this can happen? > Anybody found an answer to this question? If this is not sure it may be a good idea to memorize the fact that the error bucket was seen in the context of the chunk filter. Regards Rüdiger
Re: AW: AW: AW: 2.2 mod_http_proxy and "partial" pages
On 12/20/2005 09:15 PM, Brian Akins wrote: > In my random tests I discovered something: > > We do not want to close connection to client if the proxy request is a > sub-request. We somehow have to flag an error in that case, but we do > not want to kill entire connection. I guess this is technically possible, but is it really useful? The subrequest returns an incomplete response and thus could make the whole response invalid. But maybe I just miss the correct usecase for this. So could you please explain me where thisbit you? But you pointed me to an interesting thing: If the main response is T-E chunked and the backend error happened during the subrequest, the chunked filter may sometimes add the last-chunk marker (if the brigade containing the error bucket does *not* contain and eos bucket) and sometimes not (if the brigade containing the error bucket does contain an eos bucket). In the case that the broken backend happend on the main request the brigade always contains both buckets as they both get added to the brigade. But in the subrequest case I guess the eos bucket (of the subrequest) gets removed and the main request adds its own one later and maybe to a different brigade then the error bucket. I am even getting unsure if the brigade always contains error and eos bucket in the main request case, as there might happen an brigade split on the way to the chunk filter. Does anybody know if this can happen? Regards Rüdiger
Re: AW: AW: AW: 2.2 mod_http_proxy and "partial" pages
In my random tests I discovered something: We do not want to close connection to client if the proxy request is a sub-request. We somehow have to flag an error in that case, but we do not want to kill entire connection. Just bit me in some qa stuff. I had to revert! Ideas? -- Brian Akins Lead Systems Engineer CNN Internet Technologies
Re: AW: AW: AW: 2.2 mod_http_proxy and "partial" pages
On Dec 18, 2005, at 5:12 PM, Ruediger Pluem wrote: On 12/18/2005 06:21 PM, Jim Jagielski wrote: [..cut..] My thoughts were something more like a ap_http_error_ofilter which simply checks for the error bucket and does appropriate things; something very general that the full http chain can use. Also an interesting idea. Do you already have further applications for this in mind apart from the current problem? Not yet, but certainly a generic solution provides greater flexibility for other things that might want to use it in the future :)
Re: AW: AW: AW: 2.2 mod_http_proxy and "partial" pages
On 12/18/2005 06:21 PM, Jim Jagielski wrote: [..cut..] > > My thoughts were something more like a ap_http_error_ofilter > which simply checks for the error bucket and does appropriate > things; something very general that the full http chain can > use. Also an interesting idea. Do you already have further applications for this in mind apart from the current problem? > > Again, since we're working on trunk, let's fold these in > and "save" tuning and semantics with the actual codebase. Committed as r357519. I try to commit earlier to the trunk in the future. Regards Rüdiger
Re: AW: AW: AW: 2.2 mod_http_proxy and "partial" pages
On Dec 18, 2005, at 10:59 AM, Ruediger Pluem wrote: How about the attached patch? It moves the code into a separate http protocol specific filter that is only inserted if the request is a proxy request. Of course it adds the effort of another loop over the brigade. My thoughts were something more like a ap_http_error_ofilter which simply checks for the error bucket and does appropriate things; something very general that the full http chain can use. Again, since we're working on trunk, let's fold these in and "save" tuning and semantics with the actual codebase.
Re: AW: AW: AW: 2.2 mod_http_proxy and "partial" pages
On 12/17/2005 06:30 PM, Jim Jagielski wrote:
[..cut..]
>
>
> I still think that having this http "specific" error mode
> "hidden" within the core output filter is misguided. Instead,
> a specific http output filter is, imo, a better place.
>
How about the attached patch?
It moves the code into a separate http protocol specific filter that
is only inserted if the request is a proxy request.
Of course it adds the effort of another loop over the brigade.
Regards
Rüdiger
Index: server/core_filters.c
===
--- server/core_filters.c (Revision 357461)
+++ server/core_filters.c (Arbeitskopie)
@@ -315,10 +315,8 @@
apr_size_t *bytes_written,
conn_rec *c);
-static void detect_error_bucket(apr_bucket *bucket, conn_rec *c);
+static void remove_empty_buckets(apr_bucket_brigade *bb);
-static void remove_empty_buckets(apr_bucket_brigade *bb, conn_rec *c);
-
static apr_status_t send_brigade_blocking(apr_socket_t *s,
apr_bucket_brigade *bb,
apr_size_t *bytes_written,
@@ -489,7 +487,7 @@
if (bb == NULL) {
return;
}
-remove_empty_buckets(bb, c);
+remove_empty_buckets(bb);
if (!APR_BRIGADE_EMPTY(bb)) {
c->data_in_output_filters = 1;
if (make_a_copy) {
@@ -528,7 +526,7 @@
struct iovec vec[MAX_IOVEC_TO_WRITE];
apr_size_t nvec = 0;
-remove_empty_buckets(bb, c);
+remove_empty_buckets(bb);
for (bucket = APR_BRIGADE_FIRST(bb);
bucket != APR_BRIGADE_SENTINEL(bb);
@@ -598,26 +596,16 @@
}
}
-remove_empty_buckets(bb, c);
+remove_empty_buckets(bb);
return APR_SUCCESS;
}
-static void detect_error_bucket(apr_bucket *bucket, conn_rec *c)
+static void remove_empty_buckets(apr_bucket_brigade *bb)
{
-if (AP_BUCKET_IS_ERROR(bucket)
-&& (((ap_bucket_error *)(bucket->data))->status == HTTP_BAD_GATEWAY)) {
-/* stream aborted and we have not ended it yet */
-c->keepalive = AP_CONN_CLOSE;
-}
-}
-
-static void remove_empty_buckets(apr_bucket_brigade *bb, conn_rec *c)
-{
apr_bucket *bucket;
while (((bucket = APR_BRIGADE_FIRST(bb)) != APR_BRIGADE_SENTINEL(bb)) &&
(APR_BUCKET_IS_METADATA(bucket) || (bucket->length == 0))) {
-detect_error_bucket(bucket, c);
APR_BUCKET_REMOVE(bucket);
apr_bucket_destroy(bucket);
}
@@ -690,7 +678,6 @@
for (i = offset; i < nvec; ) {
apr_bucket *bucket = APR_BRIGADE_FIRST(bb);
if (APR_BUCKET_IS_METADATA(bucket)) {
-detect_error_bucket(bucket, c);
APR_BUCKET_REMOVE(bucket);
apr_bucket_destroy(bucket);
}
Index: modules/http/http_core.c
===
--- modules/http/http_core.c(Revision 357461)
+++ modules/http/http_core.c(Arbeitskopie)
@@ -39,6 +39,7 @@
AP_DECLARE_DATA ap_filter_rec_t *ap_http_input_filter_handle;
AP_DECLARE_DATA ap_filter_rec_t *ap_http_header_filter_handle;
AP_DECLARE_DATA ap_filter_rec_t *ap_chunk_filter_handle;
+AP_DECLARE_DATA ap_filter_rec_t *ap_broken_backend_filter_handle;
AP_DECLARE_DATA ap_filter_rec_t *ap_byterange_filter_handle;
static const char *set_keep_alive_timeout(cmd_parms *cmd, void *dummy,
@@ -242,6 +243,10 @@
ap_chunk_filter_handle =
ap_register_output_filter("CHUNK", ap_http_chunk_filter,
NULL, AP_FTYPE_TRANSCODE);
+ap_broken_backend_filter_handle =
+ap_register_output_filter("BROKEN_BACKEND",
+ ap_http_broken_backend_filter,
+ NULL, AP_FTYPE_PROTOCOL);
ap_byterange_filter_handle =
ap_register_output_filter("BYTERANGE", ap_byterange_filter,
NULL, AP_FTYPE_PROTOCOL);
Index: modules/http/http_filters.c
===
--- modules/http/http_filters.c (Revision 357461)
+++ modules/http/http_filters.c (Arbeitskopie)
@@ -1055,6 +1055,9 @@
*/
ap_add_output_filter("CHUNK", NULL, r, r->connection);
}
+/* If we have a Proxy request, add the BROKEN_BACKEND filter now */
+if (r->proxyreq != PROXYREQ_NONE)
+ap_add_output_filter("BROKEN_BACKEND", NULL, r, r->connection);
/* Don't remove this filter until after we have added the CHUNK filter.
* Otherwise, f->next won't be the CHUNK filter and thus the first
@@ -1330,3 +1333,23 @@
return bufsiz;
}
+apr_status_t ap_http_broken_backend_filter(ap_filter_t *f,
+ apr_bucket_brigade *b)
+{
+request_rec *r = f->r;
+apr_bucket *e;
+
+for (e = APR_BRIGADE_FIRST(b);
+
Re: AW: AW: AW: 2.2 mod_http_proxy and "partial" pages
On 12/18/2005 02:39 PM, Ruediger Pluem wrote: > [..cut..] > Done as r357461. > Attached a patch that > > - fixes the same problem for mod_proxy_ajp > - puts the common code in proxy_util > - fixes a little return code issue that is related to Justins original patch > in r354628 Forget about this one for a while. I detected some problems with it and I will repost it once I solved these problems Regards Rüdiger
Re: AW: AW: AW: 2.2 mod_http_proxy and "partial" pages
On 12/17/2005 11:22 PM, Ruediger Pluem wrote:
>
> On 12/17/2005 06:30 PM, Jim Jagielski wrote:
[..cut..]
>
>
>>Even so, I say let's fold this into trunk as is, and
>>then work on abstracting it out.
>
Done as r357461.
Attached a patch that
- fixes the same problem for mod_proxy_ajp
- puts the common code in proxy_util
- fixes a little return code issue that is related to Justins original patch in
r354628
Regards
Rüdiger
Index: modules/proxy/mod_proxy_ajp.c
===
--- modules/proxy/mod_proxy_ajp.c (Revision 357461)
+++ modules/proxy/mod_proxy_ajp.c (Arbeitskopie)
@@ -138,6 +138,7 @@
int havebody = 1;
int isok = 1;
apr_off_t bb_len;
+int data_sent = 0;
#ifdef FLUSHING_BANDAID
apr_int32_t conn_poll_fd;
apr_pollfd_t *conn_poll;
@@ -348,6 +349,7 @@
"proxy: error processing body");
isok = 0;
}
+data_sent = 1;
apr_brigade_cleanup(output_brigade);
}
else {
@@ -363,6 +365,7 @@
"proxy: error processing body");
isok = 0;
}
+data_sent = 1;
break;
default:
isok = 0;
@@ -400,8 +403,6 @@
}
apr_brigade_destroy(input_brigade);
-apr_brigade_destroy(output_brigade);
-
if (status != APR_SUCCESS) {
/* We had a failure: Close connection to backend */
conn->close++;
@@ -409,9 +410,20 @@
"proxy: send body failed to %pI (%s)",
conn->worker->cp->addr,
conn->worker->hostname);
+/*
+ * If we already send data, signal a broken backend connection
+ * upwards in the chain.
+ */
+if (data_sent) {
+ap_proxy_backend_broke(r, output_brigade);
+/* Return DONE to avoid error messages being added to the stream */
+return DONE;
+}
return HTTP_SERVICE_UNAVAILABLE;
}
+apr_brigade_destroy(output_brigade);
+
/* Nice we have answer to send to the client */
if (result == CMD_AJP13_END_RESPONSE && isok) {
ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, r->server,
Index: modules/proxy/proxy_util.c
===
--- modules/proxy/proxy_util.c (Revision 357461)
+++ modules/proxy/proxy_util.c (Arbeitskopie)
@@ -2129,3 +2129,18 @@
lb_workers_limit = proxy_lb_workers + PROXY_DYNAMIC_BALANCER_LIMIT;
return lb_workers_limit;
}
+
+PROXY_DECLARE(apr_status_t) ap_proxy_backend_broke(request_rec *r,
+ apr_bucket_brigade *brigade)
+{
+apr_bucket *e;
+conn_rec *c = r->connection;
+
+r->no_cache = 1;
+e = ap_bucket_error_create(HTTP_BAD_GATEWAY, NULL, c->pool,
+ c->bucket_alloc);
+APR_BRIGADE_INSERT_TAIL(brigade, e);
+e = apr_bucket_eos_create(c->bucket_alloc);
+APR_BRIGADE_INSERT_TAIL(brigade, e);
+return ap_pass_brigade(r->output_filters, brigade);
+}
Index: modules/proxy/mod_proxy_http.c
===
--- modules/proxy/mod_proxy_http.c (Revision 357461)
+++ modules/proxy/mod_proxy_http.c (Arbeitskopie)
@@ -1199,6 +1199,7 @@
* are being read. */
int pread_len = 0;
apr_table_t *save_table;
+int backend_broke = 0;
bb = apr_brigade_create(p, c->bucket_alloc);
@@ -1485,13 +1486,8 @@
*/
ap_log_cerror(APLOG_MARK, APLOG_ERR, rv, c,
"proxy: error reading response");
-r->no_cache = 1;
-e = ap_bucket_error_create(HTTP_BAD_GATEWAY, NULL,
- c->pool, c->bucket_alloc);
-APR_BRIGADE_INSERT_TAIL(bb, e);
-e = apr_bucket_eos_create(c->bucket_alloc);
-APR_BRIGADE_INSERT_TAIL(bb, e);
-ap_pass_brigade(r->output_filters, bb);
+ap_proxy_backend_broke(r, bb);
+backend_broke = 1;
backend->close = 1;
break;
}
@@ -1559,7 +1555,7 @@
} while (interim_response);
/* If our connection with the client is to be aborted, return DONE. */
-if (c->aborted) {
+if (c->aborted || backend_broke) {
return DONE;
}
Index: modules/proxy/mod_proxy.h
===
--- modules/proxy/mod_proxy.h (Revision 357461)
+++ modules/proxy/mod_proxy.h (Arbeitskopie)
@@ -669,6 +669,16 @@
PROXY_DECLARE(int) ap_proxy_connecti
Re: AW: AW: AW: 2.2 mod_http_proxy and "partial" pages
On 12/17/2005 06:30 PM, Jim Jagielski wrote: > > On Dec 17, 2005, at 11:32 AM, Ruediger Pluem wrote: > [..cut..] > > I still think that having this http "specific" error mode > "hidden" within the core output filter is misguided. Instead, > a specific http output filter is, imo, a better place. Sounds reasonable. I think the code can be placed in http_filters.c and the filter itself could be added by ap_http_header_filter once it finished its own work (just the way it currently does with the chunk filter). > > Even so, I say let's fold this into trunk as is, and > then work on abstracting it out. Yes, I will do so. Even more I will propose it for backport. Although this fix is not "perfect" with respect to your remarks above it fixes an actual problem without being a total nasty hack and thus should be included in the 2.2.x branch. If it we manage to extract this to a separate filter before 2.2.1 I would be even more happy. Regards Rüdiger
Re: AW: AW: AW: 2.2 mod_http_proxy and "partial" pages
On Dec 17, 2005, at 11:32 AM, Ruediger Pluem wrote:
Index: server/core_filters.c
===
--- server/core_filters.c (Revision 357328)
+++ server/core_filters.c (Arbeitskopie)
@@ -315,8 +315,10 @@
apr_size_t
*bytes_written,
conn_rec *c);
-static void remove_empty_buckets(apr_bucket_brigade *bb);
+static void detect_error_bucket(apr_bucket *bucket, conn_rec *c);
+static void remove_empty_buckets(apr_bucket_brigade *bb, conn_rec
*c);
+
static apr_status_t send_brigade_blocking(apr_socket_t *s,
apr_bucket_brigade *bb,
apr_size_t *bytes_written,
@@ -487,7 +489,7 @@
if (bb == NULL) {
return;
}
-remove_empty_buckets(bb);
+remove_empty_buckets(bb, c);
if (!APR_BRIGADE_EMPTY(bb)) {
c->data_in_output_filters = 1;
if (make_a_copy) {
@@ -526,7 +528,7 @@
struct iovec vec[MAX_IOVEC_TO_WRITE];
apr_size_t nvec = 0;
-remove_empty_buckets(bb);
+remove_empty_buckets(bb, c);
for (bucket = APR_BRIGADE_FIRST(bb);
bucket != APR_BRIGADE_SENTINEL(bb);
@@ -596,16 +598,26 @@
}
}
-remove_empty_buckets(bb);
+remove_empty_buckets(bb, c);
return APR_SUCCESS;
}
-static void remove_empty_buckets(apr_bucket_brigade *bb)
+static void detect_error_bucket(apr_bucket *bucket, conn_rec *c)
{
+if (AP_BUCKET_IS_ERROR(bucket)
+&& (((ap_bucket_error *)(bucket->data))->status ==
HTTP_BAD_GATEWAY)) {
+/* stream aborted and we have not ended it yet */
+c->keepalive = AP_CONN_CLOSE;
+}
+}
+
+static void remove_empty_buckets(apr_bucket_brigade *bb, conn_rec *c)
+{
apr_bucket *bucket;
while (((bucket = APR_BRIGADE_FIRST(bb)) !=
APR_BRIGADE_SENTINEL(bb)) &&
(APR_BUCKET_IS_METADATA(bucket) || (bucket->length ==
0))) {
+detect_error_bucket(bucket, c);
APR_BUCKET_REMOVE(bucket);
apr_bucket_destroy(bucket);
}
@@ -678,6 +690,7 @@
for (i = offset; i < nvec; ) {
apr_bucket *bucket = APR_BRIGADE_FIRST(bb);
if (APR_BUCKET_IS_METADATA(bucket)) {
+detect_error_bucket(bucket, c);
APR_BUCKET_REMOVE(bucket);
apr_bucket_destroy(bucket);
}
I still think that having this http "specific" error mode
"hidden" within the core output filter is misguided. Instead,
a specific http output filter is, imo, a better place.
Even so, I say let's fold this into trunk as is, and
then work on abstracting it out.
Re: AW: AW: AW: 2.2 mod_http_proxy and "partial" pages
On 12/16/2005 09:41 AM, Plüm wrote:
[..cut..]
>
> Currently I am away from my development env. I hope I can post
> a complete patch with all my ideas by tomorrow.
>
I worked out a new version of the patch. It is attached. I checked
it again with my jsp and it seems to work well. If nobody has further
objections I would commit to the trunk. After that I would do the following
next steps:
- Examine mod_proxy_ajp for the same problem.
- Put the common code of mod_proxy_ajp and mod_proxy_http to signal a broken
connection in a new function in proxy_util
Regards
Rüdiger
Index: server/core_filters.c
===
--- server/core_filters.c (Revision 357328)
+++ server/core_filters.c (Arbeitskopie)
@@ -315,8 +315,10 @@
apr_size_t *bytes_written,
conn_rec *c);
-static void remove_empty_buckets(apr_bucket_brigade *bb);
+static void detect_error_bucket(apr_bucket *bucket, conn_rec *c);
+static void remove_empty_buckets(apr_bucket_brigade *bb, conn_rec *c);
+
static apr_status_t send_brigade_blocking(apr_socket_t *s,
apr_bucket_brigade *bb,
apr_size_t *bytes_written,
@@ -487,7 +489,7 @@
if (bb == NULL) {
return;
}
-remove_empty_buckets(bb);
+remove_empty_buckets(bb, c);
if (!APR_BRIGADE_EMPTY(bb)) {
c->data_in_output_filters = 1;
if (make_a_copy) {
@@ -526,7 +528,7 @@
struct iovec vec[MAX_IOVEC_TO_WRITE];
apr_size_t nvec = 0;
-remove_empty_buckets(bb);
+remove_empty_buckets(bb, c);
for (bucket = APR_BRIGADE_FIRST(bb);
bucket != APR_BRIGADE_SENTINEL(bb);
@@ -596,16 +598,26 @@
}
}
-remove_empty_buckets(bb);
+remove_empty_buckets(bb, c);
return APR_SUCCESS;
}
-static void remove_empty_buckets(apr_bucket_brigade *bb)
+static void detect_error_bucket(apr_bucket *bucket, conn_rec *c)
{
+if (AP_BUCKET_IS_ERROR(bucket)
+&& (((ap_bucket_error *)(bucket->data))->status == HTTP_BAD_GATEWAY)) {
+/* stream aborted and we have not ended it yet */
+c->keepalive = AP_CONN_CLOSE;
+}
+}
+
+static void remove_empty_buckets(apr_bucket_brigade *bb, conn_rec *c)
+{
apr_bucket *bucket;
while (((bucket = APR_BRIGADE_FIRST(bb)) != APR_BRIGADE_SENTINEL(bb)) &&
(APR_BUCKET_IS_METADATA(bucket) || (bucket->length == 0))) {
+detect_error_bucket(bucket, c);
APR_BUCKET_REMOVE(bucket);
apr_bucket_destroy(bucket);
}
@@ -678,6 +690,7 @@
for (i = offset; i < nvec; ) {
apr_bucket *bucket = APR_BRIGADE_FIRST(bb);
if (APR_BUCKET_IS_METADATA(bucket)) {
+detect_error_bucket(bucket, c);
APR_BUCKET_REMOVE(bucket);
apr_bucket_destroy(bucket);
}
Index: modules/http/chunk_filter.c
===
--- modules/http/chunk_filter.c (Revision 357328)
+++ modules/http/chunk_filter.c (Arbeitskopie)
@@ -47,6 +47,7 @@
apr_bucket_brigade *more;
apr_bucket *e;
apr_status_t rv;
+int bad_gateway_seen = 0;
for (more = NULL; b; b = more, more = NULL) {
apr_off_t bytes = 0;
@@ -67,6 +68,13 @@
eos = e;
break;
}
+if (AP_BUCKET_IS_ERROR(e)
+&& (((ap_bucket_error *)(e->data))->status
+== HTTP_BAD_GATEWAY)) {
+/* We had a broken backend. Memorize this. */
+bad_gateway_seen = 1;
+continue;
+}
if (APR_BUCKET_IS_FLUSH(e)) {
flush = e;
more = apr_brigade_split(b, APR_BUCKET_NEXT(e));
@@ -146,13 +154,19 @@
* 2) the trailer
* 3) the end-of-chunked body CRLF
*
- * If there is no EOS bucket, then do nothing.
+ * If there is no EOS bucket, or if we had seen an error bucket with
+ * status HTTP_BAD_GATEWAY then do nothing.
+ * The error bucket with status HTTP_BAD_GATEWAY indicates that the
+ * connection to the backend (mod_proxy) broke in the middle of the
+ * response. In order to signal the client that something went wrong
+ * we do not create the last-chunk marker and set c->keepalive to
+ * AP_CONN_CLOSE in the core output filter.
*
* XXX: it would be nice to combine this with the end-of-chunk
* marker above, but this is a bit more straight-forward for
* now.
*/
-if (eos != NULL) {
+if (eos && !bad_gateway_seen) {
/* XXX: (2) trailers ... does not yet exist */
e = apr_bucket_immortal_create(ASCII_ZERO ASCII_CRLF
Re: AW: AW: 2.2 mod_http_proxy and "partial" pages
Justin Erenkrantz wrote: On Thu, Dec 15, 2005 at 10:12:57PM +0100, Ruediger Pluem wrote: I think we have to simulate to the client what happened to us on the backend: A broken connection. I mostly agree. However, Roy's veto is predicated on us not doing anything that would cause a hypothetical (*duck*) Waka protocol filter from having the underlying connection closed. The point Roy's made is that Waka will have a 'response abort' indicator. HTTP/1.1 doesn't - therefore, we should abort the connection for HTTP/1.1. So, to respect that -1, we need to keep that in mind that we only force the dropped connection somehow within the HTTP/1.1 logic. Or, have a clear path for a Waka filter chain to not drop the connection - by seeing the error bucket and morphing it into a Waka 'response abort' bucket. -- justin If we teach the HTTP/1.0 - 1.1 T-E filter to not send the 0 bucket if it sees the error bucket, and the HTTP/0.9 - 1.1 protocol filter to close the connection if it sees the error bucket and close the connection, then it should be trivial to have the waka protocol filter just keep it open flagging the last request incomplete, no? So /me concurs with your assessment. Bill
Re: AW: AW: 2.2 mod_http_proxy and "partial" pages
On Dec 16, 2005, at 3:18 AM, Justin Erenkrantz wrote: So, to respect that -1, we need to keep that in mind that we only force the dropped connection somehow within the HTTP/1.1 logic. Or, have a clear path for a Waka filter chain to not drop the connection - by seeing the error bucket and morphing it into a Waka 'response abort' bucket. -- justin Some ideas: 1. Since this is in the httpd proxy code, we insert the error bucket and set c->keepalive = AP_CONN_CLOSE 2. Each protocol adds an error_output filter, which says what to do when it sees an error bucket. I think the latter would be the most general, at the expense of having another filter in the chain :/
Re: AW: AW: 2.2 mod_http_proxy and "partial" pages
On Dec 16, 2005, at 3:18 AM, Justin Erenkrantz wrote: On Thu, Dec 15, 2005 at 10:12:57PM +0100, Ruediger Pluem wrote: I think we have to simulate to the client what happened to us on the backend: A broken connection. I mostly agree. However, Roy's veto is predicated on us not doing anything that would cause a hypothetical (*duck*) Waka protocol filter from having the underlying connection closed. The point Roy's made is that Waka will have a 'response abort' indicator. HTTP/1.1 doesn't - therefore, we should abort the connection for HTTP/1.1. So, to respect that -1, we need to keep that in mind that we only force the dropped connection somehow within the HTTP/1.1 logic. Or, have a clear path for a Waka filter chain to not drop the connection - by seeing the error bucket and morphing it into a Waka 'response abort' bucket. -- justin +1. The best way I can think of is forcing a client close by AP_CONN_CLOSE when we see an error bucket.
AW: AW: AW: 2.2 mod_http_proxy and "partial" pages
> -Ursprüngliche Nachricht- > Von: Justin Erenkrantz > Gesendet: Freitag, 16. Dezember 2005 09:19 > An: dev@httpd.apache.org > Betreff: Re: AW: AW: 2.2 mod_http_proxy and "partial" pages > > > On Thu, Dec 15, 2005 at 10:12:57PM +0100, Ruediger Pluem wrote: > > I think we have to simulate to the client what happened to > us on the backend: > > A broken connection. > > I mostly agree. > > However, Roy's veto is predicated on us not doing anything > that would cause > a hypothetical (*duck*) Waka protocol filter from having the > underlying > connection closed. The point Roy's made is that Waka will I do not intend to do close the connection by myself. Currently it will be closed because c->keepalive is set to AP_CONN_CLOSE (a approach also suggested in Roys patch). The only addition I want to make is that in the chunked case the chunked filter should not sent the closing chunk to make it clear to the client that something had broken. The question that remains to me: Does it hurt that the core output filter removes the error bucket once it has seen it? Does this address this point? Currently I am away from my development env. I hope I can post a complete patch with all my ideas by tomorrow. Regards Rüdiger
Re: AW: AW: 2.2 mod_http_proxy and "partial" pages
On Thu, Dec 15, 2005 at 10:12:57PM +0100, Ruediger Pluem wrote: > I think we have to simulate to the client what happened to us on the backend: > A broken connection. I mostly agree. However, Roy's veto is predicated on us not doing anything that would cause a hypothetical (*duck*) Waka protocol filter from having the underlying connection closed. The point Roy's made is that Waka will have a 'response abort' indicator. HTTP/1.1 doesn't - therefore, we should abort the connection for HTTP/1.1. So, to respect that -1, we need to keep that in mind that we only force the dropped connection somehow within the HTTP/1.1 logic. Or, have a clear path for a Waka filter chain to not drop the connection - by seeing the error bucket and morphing it into a Waka 'response abort' bucket. -- justin
Re: AW: AW: 2.2 mod_http_proxy and "partial" pages
On 12/15/2005 09:35 PM, Brian Akins wrote: > Plüm wrote: > This would give the client the > >> impression that the response had been correct and complete (provided >> that the reponse was in chunked encoding). If the client is a proxy >> this could lead to a cache poisoning. > > > THis is why I favor closing the connection to the client. It's simple > and the client will know that something bad happen and, usually, retry. Yes, but we need to ensure that in the case of chunked encoding the client does not get the impression that everything worked well. From my perspective this can only be reached by - not sending the zero chunk - closing the connection If we send the zero chunk the client has no possibility to find out that something went wrong. I think we have to simulate to the client what happened to us on the backend: A broken connection. Regards Rüdiger
Re: AW: AW: 2.2 mod_http_proxy and "partial" pages
Plüm wrote: This would give the client the impression that the response had been correct and complete (provided that the reponse was in chunked encoding). If the client is a proxy this could lead to a cache poisoning. THis is why I favor closing the connection to the client. It's simple and the client will know that something bad happen and, usually, retry. -- Brian Akins Lead Systems Engineer CNN Internet Technologies
AW: AW: 2.2 mod_http_proxy and "partial" pages
> -Ursprüngliche Nachricht-
> Von: Jim Jagielski
> Gesendet: Donnerstag, 15. Dezember 2005 17:02
> An: dev@httpd.apache.org
> Betreff: Re: AW: 2.2 mod_http_proxy and "partial" pages
>
>
{..cut..]
> >
> > Sorry, but I think I have to disagree.
> > There is nothing that can be handled anymore since the headers had
> > been sent to the client.
> > The only part of the chain that handles error buckets so
> far is the
> > http header filter which is gone at
> > this point of time.
>
> IMO, that's the problem... The core output filter should be
> aware of this error. Not sure if "magically" noticing this
> when removing empty buckets is the right solution...
>
No problem. Let's discuss where to place this. I just placed it
into the remove_empty_buckets function as I wanted to avoid to run
a loop twice over the brigade. I think I need some kind of loop
because otherwise I might miss this bucket (e.g. in remove_empty_bucket,
if there are other meta buckets before the error bucket).
Having the check only in writev_nonblocking might lead to a miss of
this bucket.
Anyway I detected another problem that is also there with my current
patch proposal. I think we need to make the ap_http_chunk_filter aware of this
error bucket. Otherwise it will add the "closing" zero length chunk to
the response once it sees the eos bucket. This would give the client the
impression that the response had been correct and complete (provided
that the reponse was in chunked encoding). If the client is a proxy
this could lead to a cache poisoning. So my proposal is that we do
*not* insert the "closing" zero length chunk to signal the client
that the response is not complete and broke in the middle.
Regards
Rüdiger
