AW: AW: AW: 2.2 mod_http_proxy and partial pages

[Plüm , Rüdiger , VIS]

 
 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.]

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.

Re: AW: AW: AW: 2.2 mod_http_proxy and partial pages

[Ruediger Pluem]

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

[William A. Rowe, Jr.]

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

[William A. Rowe, Jr.]

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

[Graham Leggett]

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

[Ruediger Pluem]

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 filter
+ * context.

Re: AW: AW: AW: 2.2 mod_http_proxy and partial pages

[Jim Jagielski]

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

[Jim Jagielski]

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

[Ruediger Pluem]

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

[Ruediger Pluem]

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

[Jim Jagielski]

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

[Ruediger Pluem]

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
/* trailers */

Re: AW: AW: AW: 2.2 mod_http_proxy and partial pages

[Joe Orton]

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

[Ruediger Pluem]

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

[Nick Kew]

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

[Ruediger Pluem]

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, APR_BUCKET_NEXT(e));
@@ 

Re: AW: AW: AW: 2.2 mod_http_proxy and partial pages

[Ruediger Pluem]

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

[Justin Erenkrantz]

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

[Jim Jagielski]

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

[Ruediger Pluem]

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

[Justin Erenkrantz]

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

[Brian Akins]

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

[Ruediger Pluem]

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

[Jim Jagielski]

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

[Ruediger Pluem]

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_connection_create(const char *proxy_function,
   

Re: AW: AW: AW: 2.2 mod_http_proxy and partial pages

[Ruediger Pluem]

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

[Ruediger Pluem]

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);
+ e != 

Re: AW: AW: AW: 2.2 mod_http_proxy and partial pages

[Jim Jagielski]

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

[Ruediger Pluem]

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

[Ruediger Pluem]

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: AW: 2.2 mod_http_proxy and partial pages

[Jim Jagielski]

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

[Ruediger Pluem]

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: 2.2 mod_http_proxy and partial pages

[Justin Erenkrantz]

On Thu, Dec 15, 2005 at 05:57:24PM +0100, Plm, Rdiger, VIS wrote:
 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.

If we can do it, sure.  Otherwise, I won't lose any sleep.  -- justin

Re: AW: AW: 2.2 mod_http_proxy and partial pages

[Justin Erenkrantz]

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

AW: AW: AW: 2.2 mod_http_proxy and partial pages

[Plüm , Rüdiger , VIS]

 -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: 2.2 mod_http_proxy and partial pages

[Roy T. Fielding]

On Dec 16, 2005, at 12:41 AM, Plüm, Rüdiger, VIS wrote:

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).


Right, the important bit is that the code managing the client
connection is what should close the client connection, not the
code that is managing the outbound (server) connection.  For all
we know, the client connection might be an SMTP notifier.

If we wanted to get really fancy, we could check to see if the
response code has been sent yet and change the whole response to
a 502 bad gateway, but that probably isn't worth the effort.


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.


I agree with that change.


The question that remains to me: Does it hurt that the core output
filter removes the error bucket once it has seen it?


I have no idea. I looked at the filtering code and the way it uses
error buckets (in fact, the only place that uses error buckets)
and I don't understand why it was written this way at all.  It is
more efficient to just use the EOS bucket data to indicate an
error in all of those cases, since they all result in an EOS
anyway and we just waste memory/time with the special bucket type.

Roy

Re: AW: AW: 2.2 mod_http_proxy and partial pages

[Jim Jagielski]

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.

Re: AW: AW: 2.2 mod_http_proxy and partial pages

[Jim Jagielski]

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

[William A. Rowe, Jr.]

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: 2.2 mod_http_proxy and partial pages

[Jim Jagielski]

On Dec 13, 2005, at 7:32 PM, Ruediger Pluem wrote:




On 12/14/2005 12:59 AM, Jim Jagielski wrote:


On Dec 13, 2005, at 1:28 PM, Ruediger Pluem wrote:


[..cut..]



The reason the other patch didn't do this is that,
upon reflection, closing the client connection at
this point does not seem quite right. Closing
it simply because the gateway died just doesn't
feel right, and seems almost overkill. I think
simply setting the error bucket to allow the rest
of the chain to correctly handle it, is the best
we could (and should) do :/


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... 

AW: AW: 2.2 mod_http_proxy and partial pages

[Plüm , Rüdiger , VIS]

 -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

Re: AW: AW: 2.2 mod_http_proxy and partial pages

[Brian Akins]

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

Re: AW: AW: 2.2 mod_http_proxy and partial pages

[Ruediger Pluem]

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: 2.2 mod_http_proxy and partial pages

[Jim Jagielski]

Eyes please... The coffee is VERY week this morning :)

Index: modules/proxy/mod_proxy_http.c
===
--- modules/proxy/mod_proxy_http.c  (revision 356419)
+++ modules/proxy/mod_proxy_http.c  (working copy)
@@ -1481,12 +1481,19 @@
 }
 else if (rv != APR_SUCCESS) {
 /* In this case, we are in real trouble  
because

- * our backend bailed on us, so abort our
- * connection to our user too.
+ * our backend bailed on us. Pass along a  
502 error

+ * bucket
  */
 ap_log_cerror(APLOG_MARK, APLOG_ERR, rv, c,
   proxy: error reading  
response);

-c-aborted = 1;
+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(f-c-bucket_alloc);
+APR_BRIGADE_INSERT_TAIL(bb, e);
+ap_pass_brigade(r-output_filters, bb);
+backend-close = 1;
 break;
 }
 /* next time try a non-blocking read */
Index: modules/cache/mod_disk_cache.c
===
--- modules/cache/mod_disk_cache.c  (revision 356419)
+++ modules/cache/mod_disk_cache.c  (working copy)
@@ -1010,7 +1010,7 @@
  * sanity checks.
  */
 if (APR_BUCKET_IS_EOS(APR_BRIGADE_LAST(bb))) {
-if (r-connection-aborted) {
+if (r-connection-aborted || r-no_cache) {
 ap_log_error(APLOG_MARK, APLOG_INFO, 0, r-server,
  disk_cache: Discarding body for URL %s 
  because connection has been aborted.,

Re: AW: 2.2 mod_http_proxy and partial pages

[Graham Leggett]

Jim Jagielski wrote:


Eyes please... The coffee is VERY week this morning :)


Despite having overindulged upon the sushi, I shall give it a go :)


Index: modules/cache/mod_disk_cache.c
===
--- modules/cache/mod_disk_cache.c  (revision 356419)
+++ modules/cache/mod_disk_cache.c  (working copy)
@@ -1010,7 +1010,7 @@
  * sanity checks.
  */
 if (APR_BUCKET_IS_EOS(APR_BRIGADE_LAST(bb))) {
-if (r-connection-aborted) {
+if (r-connection-aborted || r-no_cache) {
 ap_log_error(APLOG_MARK, APLOG_INFO, 0, r-server,
  disk_cache: Discarding body for URL %s 
  because connection has been aborted.,



Would mem_cache have to do the same thing? Is there not a way to make 
this generic?


Otherwise +1.

Regards,
Graham
--


smime.p7s
Description: S/MIME Cryptographic Signature

Re: AW: 2.2 mod_http_proxy and partial pages

[Brian Akins]

Graham Leggett wrote:

Would mem_cache have to do the same thing? Is there not a way to make 
this generic?


Each cache provider traverses the brigade itself, so at this time, each 
provider must handle this itself.


--
Brian Akins
Lead Systems Engineer
CNN Internet Technologies

Re: AW: 2.2 mod_http_proxy and partial pages

[Ruediger Pluem]

On 12/13/2005 05:55 PM, Jim Jagielski wrote:
 Eyes please... The coffee is VERY week this morning :)

Yes, I guess it was very weAk ;-). Just teasing. :)
Patch looks good to me. As it is only half of the way I assembled it with the 
other
pieces we had and I had sent. The attached patch passed the test with my jsp I 
described
earlier (and it really closes the connection to the client :-)).

So some eyes on this one please.

Regards

Rüdiger

P.S: Ignore the garbage signs in the mail version of the patch. Its just a copy 
and paste
from my vi.

Index: server/core_filters.c
===
--- server/core_filters.c»··(Revision 356370)
+++ 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,25 @@
 }
 }
·
-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 ((bucket-type == ap_bucket_type_error)  (((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 +689,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/proxy/mod_proxy_http.c
===
--- modules/proxy/mod_proxy_http.c»·(Revision 356370)
+++ modules/proxy/mod_proxy_http.c»·(Arbeitskopie)
@@ -1481,12 +1481,18 @@
 }
 else if (rv != APR_SUCCESS) {
 /* In this case, we are in real trouble because
- * our backend bailed on us, so abort our
- * connection to our user too.
+ * our backend bailed on us.
  */
 ap_log_cerror(APLOG_MARK, APLOG_ERR, rv, c,
   proxy: error reading response);
-c-aborted = 1;
+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);
+backend-close = 1;
 break;
 }
 /* next time try a non-blocking read */
Index: modules/cache/mod_disk_cache.c
===
--- modules/cache/mod_disk_cache.c»·(Revision 356370)
+++ modules/cache/mod_disk_cache.c»·(Arbeitskopie)
@@ -1010,7 +1010,7 @@
  * sanity checks.
  */
 if (APR_BUCKET_IS_EOS(APR_BRIGADE_LAST(bb))) {
-if (r-connection-aborted) {
+if (r-connection-aborted || r-no_cache) {
 ap_log_error(APLOG_MARK, APLOG_INFO, 0, r-server,
  disk_cache: Discarding body for URL %s 
  because connection has been aborted.,
Index: 

Re: AW: 2.2 mod_http_proxy and partial pages

[Jim Jagielski]

Graham Leggett wrote:
 
  Index: modules/cache/mod_disk_cache.c
  ===
  --- modules/cache/mod_disk_cache.c  (revision 356419)
  +++ modules/cache/mod_disk_cache.c  (working copy)
  @@ -1010,7 +1010,7 @@
* sanity checks.
*/
   if (APR_BUCKET_IS_EOS(APR_BRIGADE_LAST(bb))) {
  -if (r-connection-aborted) {
  +if (r-connection-aborted || r-no_cache) {
   ap_log_error(APLOG_MARK, APLOG_INFO, 0, r-server,
disk_cache: Discarding body for URL %s 
because connection has been aborted.,
  
 
 Would mem_cache have to do the same thing? Is there not a way to make 
 this generic?
 
 Otherwise +1.
 

Not really, at least as far as I can see (yet! :) ). The reason is
that what we use to trigger a error happened, don't cache is
the no_cache flag, and so each would need to check that (which
they should be doing anyway, imo ;) )
-- 
===
 Jim Jagielski   [|]   [EMAIL PROTECTED]   [|]   http://www.jaguNET.com/
   If you can dodge a wrench, you can dodge a ball.

Re: AW: 2.2 mod_http_proxy and partial pages

[Jim Jagielski]

On Dec 13, 2005, at 1:28 PM, Ruediger Pluem wrote:


Index: server/core_filters.c
===
--- server/core_filters.c»··(Revision 356370)
+++ 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);

+


The reason the other patch didn't do this is that,
upon reflection, closing the client connection at
this point does not seem quite right. Closing
it simply because the gateway died just doesn't
feel right, and seems almost overkill. I think
simply setting the error bucket to allow the rest
of the chain to correctly handle it, is the best
we could (and should) do :/

Re: AW: 2.2 mod_http_proxy and partial pages

[Ruediger Pluem]

On 12/14/2005 12:59 AM, Jim Jagielski wrote:
 
 On Dec 13, 2005, at 1:28 PM, Ruediger Pluem wrote:

[..cut..]

 
 The reason the other patch didn't do this is that,
 upon reflection, closing the client connection at
 this point does not seem quite right. Closing
 it simply because the gateway died just doesn't
 feel right, and seems almost overkill. I think
 simply setting the error bucket to allow the rest
 of the chain to correctly handle it, is the best
 we could (and should) do :/

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.
So the client will be most likely waiting for more data on this connection if 
the content-length was bigger
than what was sent so far. So from my point of view closing the connection is 
the only possibility to
signal the client that something got irrevocably wrong.


Regards

Rüdiger

Re: AW: 2.2 mod_http_proxy and partial pages

[Jim Jagielski]

On Dec 8, 2005, at 11:13 PM, Roy T. Fielding wrote:




I would extend the EOS bucket data to be an errno and then have
mod_cache check for that data != 0 when it does its EOS check.


For httpd's filters, an EOS bucket data doesn't attempt a close of  
the
stream: in fact, EOS doesn't do anything to the socket.  By the  
time we
start writing the body, all of the filters that know HTTP are long  
gone.
(The only thing that might be left is a chunking output filter.)   
Plus,
with apr's bucket types, there is no mechanism to associate that  
type of

error condition.


No, but we can graft it onto the currently unused EOS bucket data.



Hmmm... or use the error bucket type, with some intelligent
checking at the correct time.

Re: AW: 2.2 mod_http_proxy and partial pages

[William A. Rowe, Jr.]

Jim Jagielski wrote:


On Dec 8, 2005, at 11:13 PM, Roy T. Fielding wrote:




I would extend the EOS bucket data to be an errno and then have
mod_cache check for that data != 0 when it does its EOS check.


Ugh, EOS means exactly what it says...


No, but we can graft it onto the currently unused EOS bucket data.


There's really no need to refactor...


Hmmm... or use the error bucket type, with some intelligent
checking at the correct time.


++1.  We have the technology, we don't need to extend EOS buckets.

If anything gets in the way of the error bucket making it to the right
place, that behavior would be a bug.

Bill

Re: AW: 2.2 mod_http_proxy and partial pages

[Graham Leggett]

William A. Rowe, Jr. wrote:


Hmmm... or use the error bucket type, with some intelligent
checking at the correct time.


++1.  We have the technology, we don't need to extend EOS buckets.


+1 as well.

Regards,
Graham
--


smime.p7s
Description: S/MIME Cryptographic Signature

Re: 2.2 mod_http_proxy and partial pages

[Plüm , Rüdiger , VIS]

 -Ursprüngliche Nachricht-
 Von: Justin Erenkrantz 
 Gesendet: Freitag, 9. Dezember 2005 06:22
 An: dev@httpd.apache.org
 Betreff: Re: AW: 2.2 mod_http_proxy and partial pages
 

[..cut..]

 Even with an EOS bucket, how will we indicate that the 
 connection should be aborted?  (i.e. don't try to do any 
 further writes?)
 
 (See below as to why an EOS doesn't do anything.)
 
  If you have another idea how to specify via the handler that the
  connection
  needs to be dropped, I'm all ears.  But, I couldn't see one.  --  
  justin
  
  I would extend the EOS bucket data to be an errno and then have 
  mod_cache check for that data != 0 when it does its EOS check.
 
 For httpd's filters, an EOS bucket data doesn't attempt a close of the
 stream: in fact, EOS doesn't do anything to the socket.  By 
 the time we start writing the body, all of the filters that 

Sorry for possibly getting back to a dead horse, but what if we set
c-keepalive to AP_CONN_CLOSE and sent an EOS bucket?
Of course currently this would only cause the connection to the client to be 
closed.
For the notification of the filters on the stack that something has gone
wrong we need additional measures as the error code addition to the
EOS bucket.

[..cut..]

Regards

Rüdiger

Re: AW: 2.2 mod_http_proxy and partial pages

[Brian Akins]

Roy T. Fielding wrote:


Me too -- I'll see what I can come up with during the hackathon.



So I'm assuming we will have no consensus until after apachecon?



When I looked at it earlier, a fix seemed feasible with only two
lines of code plus whatever log messages are desired.


That's the fix you vetoed, isn't it?


--
Brian Akins
Lead Systems Engineer
CNN Internet Technologies

Re: AW: 2.2 mod_http_proxy and partial pages

[Graham Leggett]

Roy T. Fielding wrote:


No, you understood its purpose correctly.  I have no idea what Justin
is talking about -- maybe the proxy's connection to the outbound server?
c-aborted is only set on the inbound connection when a previous write
to that connection has failed.

Setting the inbound c-aborted within a filter just to indicate
that the outbound connection has died is wrong -- it prevents
other parts of the server (that are working fine) from correctly
handling the bad gateway response.


I don't fully understand - the problem in question (as I understand it) 
happens when the backend drops the connection unexpectedly half way 
through the response.


At this point the frontend connection has already sent a 200 Ok, it's 
already sent headers like Content-Length, etc, at this point there is no 
graceful way of handling the error or sending bad gateway.


The only sane thing to do at this point (as I can see, unless I am 
missing something) is to abort the frontend connection, mirroring the 
same behaviour that the backend just did to us. At the same time, 
modules like Cache needs to be informed things have gone pear shaped, so 
the cached entry is invalidated.


Or am I missing something?

Regards,
Graham
--


smime.p7s
Description: S/MIME Cryptographic Signature

Re: 2.2 mod_http_proxy and partial pages

[Brian Akins]

Plüm wrote:


Stupid question: Can't we enforce at least this by setting
c-keepalive to AP_CONN_CLOSE. Of course this does not solve the problem
to make the remaining parts of the code aware of the bad gateway situation.


Probably, but other modules already check c-aborted. So it would be 
best to be consistent.


--
Brian Akins
Lead Systems Engineer
CNN Internet Technologies

Re: AW: 2.2 mod_http_proxy and partial pages

[Brian Akins]

Graham Leggett wrote:

At this point the frontend connection has already sent a 200 Ok, it's 
already sent headers like Content-Length, etc, at this point there is no 
graceful way of handling the error or sending bad gateway.


I agree.  There is no way we can inform the client of the problem. 
Currently,


- we just cease sending the client data.
- The client is rather confused
- other modules think the request completed successfully.


If we c-aborted:
- we just cease sending the client data.
- we close connection to client
- The client is rather confused
- other modules know that the request was aborted and can react


So, currently, we would have to change multiple modules to notice some 
other error notification.  Meanwhile, the client gains nothing.  So, I 
say, just use c-aborted



Another option, possibly even more evil, is to have another field in 
request_rec (or somewhere easy) that notes that the server (us or the 
backend) is causing the termination and not the client. Or make 
r-aborted have multiple values like c-keepalive:


#define AP_NOT_ABORTED 0
#define AP_CLIENT_ABORTED 1
#define AP_ORIGIN_ABORTED 2
#define AP_INTERNAL_ABORTED  3


most modules could still just check for r-aborted.  You could call that 
legacy support.  New modules, if they desired, could react differently 
depending on actual value.


Thoughts?
--
Brian Akins
Lead Systems Engineer
CNN Internet Technologies

Re: AW: 2.2 mod_http_proxy and partial pages

[Brian Akins]

Brian Akins wrote:


r-aborted have multiple values like c-keepalive:


of course, I meant c-aborted.



--
Brian Akins
Lead Systems Engineer
CNN Internet Technologies

Re: AW: 2.2 mod_http_proxy and partial pages

[Justin Erenkrantz]

On Wed, Dec 07, 2005 at 06:16:42PM -0800, Roy Fielding wrote:
 Setting the inbound c-aborted within a filter just to indicate
 that the outbound connection has died is wrong -- it prevents
 other parts of the server (that are working fine) from correctly
 handling the bad gateway response.

FWIW, it's not a filter - it's the handler itself that is setting
c-aborted.

If you have another idea how to specify via the handler that the connection
needs to be dropped, I'm all ears.  But, I couldn't see one.  -- justin

Re: AW: 2.2 mod_http_proxy and partial pages

[Jim Jagielski]

On Dec 8, 2005, at 8:31 AM, Brian Akins wrote:


Graham Leggett wrote:

At this point the frontend connection has already sent a 200 Ok,  
it's already sent headers like Content-Length, etc, at this point  
there is no graceful way of handling the error or sending bad  
gateway.


I agree.  There is no way we can inform the client of the problem.  
Currently,


- we just cease sending the client data.
- The client is rather confused
- other modules think the request completed successfully.


If we c-aborted:
- we just cease sending the client data.
- we close connection to client
- The client is rather confused
- other modules know that the request was aborted and can react


So, currently, we would have to change multiple modules to notice  
some other error notification.  Meanwhile, the client gains  
nothing.  So, I say, just use c-aborted




+1

Re: AW: 2.2 mod_http_proxy and partial pages

[Roy T. Fielding]

On Dec 8, 2005, at 6:58 AM, Justin Erenkrantz wrote:


On Wed, Dec 07, 2005 at 06:16:42PM -0800, Roy Fielding wrote:

Setting the inbound c-aborted within a filter just to indicate
that the outbound connection has died is wrong -- it prevents
other parts of the server (that are working fine) from correctly
handling the bad gateway response.


FWIW, it's not a filter - it's the handler itself that is setting
c-aborted.


The handler is just the top of the filter stack.  The point is that
it doesn't know anything about the other end (the c connection) and
thus can't know if it should be aborted.  waka, for example, has an
abort mechanism on the wire and thus would be horribly abused by
such a workaround.

If you have another idea how to specify via the handler that the  
connection
needs to be dropped, I'm all ears.  But, I couldn't see one.  --  
justin


I would extend the EOS bucket data to be an errno and then have
mod_cache check for that data != 0 when it does its EOS check.

I am surprised it wasn't implemented that way in the first place.

Roy

Re: AW: 2.2 mod_http_proxy and partial pages

[Justin Erenkrantz]

On Thu, Dec 08, 2005 at 08:38:53PM -0800, Roy Fielding wrote:
 On Dec 8, 2005, at 6:58 AM, Justin Erenkrantz wrote:
 
 On Wed, Dec 07, 2005 at 06:16:42PM -0800, Roy Fielding wrote:
 Setting the inbound c-aborted within a filter just to indicate
 that the outbound connection has died is wrong -- it prevents
 other parts of the server (that are working fine) from correctly
 handling the bad gateway response.
 
 FWIW, it's not a filter - it's the handler itself that is setting
 c-aborted.
 
 The handler is just the top of the filter stack.  The point is that
 it doesn't know anything about the other end (the c connection) and
 thus can't know if it should be aborted.  waka, for example, has an
 abort mechanism on the wire and thus would be horribly abused by
 such a workaround.

Even with an EOS bucket, how will we indicate that the connection should be
aborted?  (i.e. don't try to do any further writes?)

(See below as to why an EOS doesn't do anything.)

 If you have another idea how to specify via the handler that the  
 connection
 needs to be dropped, I'm all ears.  But, I couldn't see one.  --  
 justin
 
 I would extend the EOS bucket data to be an errno and then have
 mod_cache check for that data != 0 when it does its EOS check.

For httpd's filters, an EOS bucket data doesn't attempt a close of the
stream: in fact, EOS doesn't do anything to the socket.  By the time we
start writing the body, all of the filters that know HTTP are long gone.
(The only thing that might be left is a chunking output filter.)  Plus,
with apr's bucket types, there is no mechanism to associate that type of
error condition.

 I am surprised it wasn't implemented that way in the first place.

I am too, but I'd like to be able to resolve this veto without radically
having to redesign filters in httpd.  =)

FWIW, serf doesn't have these sorts of problems; that's one of the (many)
things we corrected with serf's filter design.  -- justin

Re: AW: 2.2 mod_http_proxy and partial pages

[Roy T. Fielding]

On Dec 8, 2005, at 9:21 PM, Justin Erenkrantz wrote:
Even with an EOS bucket, how will we indicate that the connection  
should be

aborted?  (i.e. don't try to do any further writes?)


The inbound connection hasn't been aborted -- only the outbound.
We don't even want to abort the inbound connection until the client
has received as much of the response as we received, since that
will help the client diagnose the error.  The only thing we need
to do is invalidate the cache entry, which would be done when
mod_*_cache checks the EOS bucket (which it already does)
for an error (the new feature).


I would extend the EOS bucket data to be an errno and then have
mod_cache check for that data != 0 when it does its EOS check.


For httpd's filters, an EOS bucket data doesn't attempt a close of the
stream: in fact, EOS doesn't do anything to the socket.  By the  
time we
start writing the body, all of the filters that know HTTP are long  
gone.
(The only thing that might be left is a chunking output filter.)   
Plus,
with apr's bucket types, there is no mechanism to associate that  
type of

error condition.


No, but we can graft it onto the currently unused EOS bucket data.


I am surprised it wasn't implemented that way in the first place.


I am too, but I'd like to be able to resolve this veto without  
radically

having to redesign filters in httpd.  =)


Me too -- I'll see what I can come up with during the hackathon.
When I looked at it earlier, a fix seemed feasible with only two
lines of code plus whatever log messages are desired.

FWIW, serf doesn't have these sorts of problems; that's one of the  
(many)

things we corrected with serf's filter design.  -- justin


Yes, but will it be done before waka? ;-)

Roy

Re: 2.2 mod_http_proxy and partial pages

[Plüm , Rüdiger , VIS]

 -On December 7, 2005 2:00:19 AM +0100 Ruediger Pluem [EMAIL PROTECTED] 
 wrote:

 The patches to mod_proxy_http we identified here on list do indeed work
 and are in as r354628.

 Sorry for stepping in that late into the discussion, but wouldn't it be
 better to fix that after the return from proxy_run_scheme_handler in
 mod_proxy?

 The error has to be flagged inside the HTTP scheme before the error is 
 lost.  Without this patch, mod_proxy_http returns 'success' 
 unconditionally.  That is clearly wrong and that's what I changed.

Yes, of course the scheme handler must signal the proxy handler that the backend
broke. Just returning 'success' in this case is of course plain wrong.


 I fear that mod_proxy_ajp is affected by the same problem that
 is now fixed in mod_proxy_http. This means we put the burden of handling
 this in a unified way on each proxy backend module. How about letting the
 schema_handler simply return a specific value (DONE or whatever) to
 signal that the backend broke in the middle of sending the response and
 let mod_proxy handle the dirty work.

 That's what it does right now.  What would you like to change?

I would like to set the c-aborted in mod_proxy's proxy_handler after the
run_scheme_handler.

Reason:

1. We can define a clear interface for the scheme handlers here:
   If the backend broke before you sent headers just return BAD_GATEWAY
   and send nothing, if it broke afterwards just return BROKEN_BACKEND
   (or whatever you like that should be defined for this case).
   The proxy handler would handle this BROKEN_BACKEND return code and
   do the 'right' thing (currently setting c-aborted).
   Thus we do not have the need to put the burden of the details on
   the schema handler (why I regard it as a burden see 2.)

2. I am not 100% percent happy with the c-aborted approach as the original
   intention of c-aborted was another one (The connection to the *client* broke
   not to the *backend*). I admit that I do not see any other approach
   currently, so we should stick with this, but if we decide to change this
   later on and we follow 1. then it is much easier to change as we have this
   code only in *one* location and not in every scheme handler.

[..cut..]

 An error bucket is already sent down the chain when the specific connection 
 error I hit with the chunked line occurs through HTTP_IN, but that 
 accomplishes little because the HTTP filters which understand the error 
 buckets have already gone as the headers have been sent.

 FWIW, an error bucket, by itself, would not be enough; the connection close 
 logic is only implemented well outside of the filter logic.  At best, it 
 has to be an error bucket combined with a returned status code that can be 
 returned all the way up.  -- justin

Ahh, ok. Thanks for clarification.

Regards

Rüdiger

Re: 2.2 mod_http_proxy and partial pages

[Justin Erenkrantz]

On Wed, Dec 07, 2005 at 10:21:10AM +0100, Plm, Rdiger, VIS wrote:
 I would like to set the c-aborted in mod_proxy's proxy_handler after the
 run_scheme_handler.
 
 Reason:
 
 1. We can define a clear interface for the scheme handlers here:
If the backend broke before you sent headers just return BAD_GATEWAY
and send nothing, if it broke afterwards just return BROKEN_BACKEND
(or whatever you like that should be defined for this case).
The proxy handler would handle this BROKEN_BACKEND return code and
do the 'right' thing (currently setting c-aborted).
Thus we do not have the need to put the burden of the details on
the schema handler (why I regard it as a burden see 2.)

Feel free to commit a patch.  =)

 2. I am not 100% percent happy with the c-aborted approach as the original
intention of c-aborted was another one (The connection to the *client* 
 broke
not to the *backend*). I admit that I do not see any other approach
currently, so we should stick with this, but if we decide to change this
later on and we follow 1. then it is much easier to change as we have this
code only in *one* location and not in every scheme handler.

But, that's exactly what we want: we must abort the connection to the
client because we can't complete the response.  -- justin

RE: 2.2 mod_http_proxy and partial pages

[Plüm , Rüdiger , VIS]

 -Ursprüngliche Nachricht-
 Von: Justin Erenkrantz 
 Gesendet: Mittwoch, 7. Dezember 2005 17:08
 An: dev@httpd.apache.org
 Betreff: Re: 2.2 mod_http_proxy and partial pages
 

[..cut..]

 
 Feel free to commit a patch.  =)

I will do so :).

 
  2. I am not 100% percent happy with the c-aborted approach 
 as the original
 intention of c-aborted was another one (The connection 
 to the *client* broke
 not to the *backend*). I admit that I do not see any 
 other approach
 currently, so we should stick with this, but if we 
 decide to change this
 later on and we follow 1. then it is much easier to 
 change as we have this
 code only in *one* location and not in every scheme handler.
 
 But, that's exactly what we want: we must abort the 
 connection to the client because we can't complete the 
 response.  -- justin

Yes, I know. Maybe this is nitpicking, but my original understanding is that
c-aborted is set if the connection to the client has broken for whatever 
external
reason on the network route between client and server, not if we decide that we
need to / should break this connection to the client because of something has 
gone
wrong on the backend. But as said, this is possibly just nitpicking :-).

Regards

Rüdiger

 

Re: 2.2 mod_http_proxy and partial pages

[Justin Erenkrantz]

On Wed, Dec 07, 2005 at 05:24:46PM +0100, Plm, Rdiger, VIS wrote:
 Yes, I know. Maybe this is nitpicking, but my original understanding is that
 c-aborted is set if the connection to the client has broken for whatever 
 external
 reason on the network route between client and server, not if we decide that 
 we
 need to / should break this connection to the client because of something has 
 gone
 wrong on the backend. But as said, this is possibly just nitpicking :-).

Nope, that's the flag we set when we want the core to drop the connection.
I thought that it would be set by the filters when a connection was
dropped, but, as I said earlier in this thread, I'm wrong.  The filters
will never ever set it.  -- justin

AW: 2.2 mod_http_proxy and partial pages

[Plüm , Rüdiger , VIS]

 -Ursprüngliche Nachricht-
 Von: Justin Erenkrantz 
 Gesendet: Mittwoch, 7. Dezember 2005 17:30
 An: dev@httpd.apache.org
 Betreff: Re: 2.2 mod_http_proxy and partial pages
 
 
 On Wed, Dec 07, 2005 at 05:24:46PM +0100, Plm, Rdiger, VIS wrote:

[..cut..]

 
 Nope, that's the flag we set when we want the core to drop 
 the connection. I thought that it would be set by the filters 
 when a connection was dropped, but, as I said earlier in this 
 thread, I'm wrong.  The filters will never ever set it.  -- justin
 

Ok. Then I withdraw my objections against the setting of c-aborted.
I just understood its purpose wrong. Thanks for clarification.
Regarding the question where to set this (in scheme_handler vs after
run_scheme_handler hook in proxy handler) I will redecide once I had
a more closer look on mod_proxy_ajp.

Regards

Rüdiger

Re: AW: 2.2 mod_http_proxy and partial pages

[Brian Akins]

Okay, so what is the status of this?  Are the two patches submitted 
good enough for all those concerned.


AFAIK, they seem to fix my issue.  I would like some idea of how 2.2.1 
will handle it, so I do not wind up having to patch and re-patch here.


TIA

--
Brian Akins
Lead Systems Engineer
CNN Internet Technologies

Re: AW: 2.2 mod_http_proxy and partial pages

[Roy T. Fielding]

On Dec 7, 2005, at 8:44 AM, Plüm, Rüdiger, VIS wrote:

Nope, that's the flag we set when we want the core to drop
the connection. I thought that it would be set by the filters
when a connection was dropped, but, as I said earlier in this
thread, I'm wrong.  The filters will never ever set it.  -- justin


Ok. Then I withdraw my objections against the setting of c-aborted.
I just understood its purpose wrong. Thanks for clarification.


No, you understood its purpose correctly.  I have no idea what Justin
is talking about -- maybe the proxy's connection to the outbound server?
c-aborted is only set on the inbound connection when a previous write
to that connection has failed.

Setting the inbound c-aborted within a filter just to indicate
that the outbound connection has died is wrong -- it prevents
other parts of the server (that are working fine) from correctly
handling the bad gateway response.

-1 VETO

Find another solution before the next release.  I have no objection
to such a workaround as a patch, but we don't introduce new spaghetti
code just to fix a trivial problem.

Roy

Re: 2.2 mod_http_proxy and partial pages

[Plüm , Rüdiger , VIS]

 -Ursprüngliche Nachricht-
 Von: Roy T. Fielding 
 Gesendet: Donnerstag, 8. Dezember 2005 03:17

[..cut..]

  Ok. Then I withdraw my objections against the setting of 
 c-aborted. I 
  just understood its purpose wrong. Thanks for clarification.
 
 No, you understood its purpose correctly.  I have no idea 
 what Justin is talking about -- maybe the proxy's connection 
 to the outbound server?
 c-aborted is only set on the inbound connection when a previous write
 to that connection has failed.

Ok. Any further opinions on the purpose of c-aborted that support either
Roys or Justins point of view on this?

 
 Setting the inbound c-aborted within a filter just to 
 indicate that the outbound connection has died is wrong -- it 
 prevents other parts of the server (that are working fine) 
 from correctly handling the bad gateway response.

Just to ensure that we are talking about the same thing here:
The client will never get aware of the bad gateway as it already
has received the headers. So we are only talking about handling
this bad gateway situation correctly internally, so that for example
mod_cache does not cache an incomplete and thus broken response.
The client is doomed anyway.
As far as I can see everybody agrees that the connection to the
client must be closed in this situation.
Stupid question: Can't we enforce at least this by setting
c-keepalive to AP_CONN_CLOSE. Of course this does not solve the problem
to make the remaining parts of the code aware of the bad gateway situation.

Regards

Rüdiger

[..cut..]

2.2 mod_http_proxy and partial pages

[Brian Akins]

I have a serious issue.  It seems that if something happens during a 
proxy request after mod_http_proxy starts reading from the backend 
server, no error is reported. (IE, see what happens when ap_pass_brigade 
 returns non success).  The problem is that this partial page may be 
cached because r-status is a cacheable code, but the response is 
actually broken.  The result is that the broken version may be cached 
and served again and again.


Should we break RFC (maybe) and change r-status to 503 (for example) so 
cache can check letter.


Note: this is using our internal mod_cache, but looks like the issue 
would be the same in stock mod_cache.



--
Brian Akins
Lead Systems Engineer
CNN Internet Technologies

Re: 2.2 mod_http_proxy and partial pages

[Jim Jagielski]

On Dec 6, 2005, at 9:29 AM, Brian Akins wrote:

I have a serious issue.  It seems that if something happens during  
a proxy request after mod_http_proxy starts reading from the  
backend server, no error is reported. (IE, see what happens when  
ap_pass_brigade  returns non success).  The problem is that this  
partial page may be cached because r-status is a cacheable code,  
but the response is actually broken.  The result is that the  
broken version may be cached and served again and again.


Should we break RFC (maybe) and change r-status to 503 (for  
example) so cache can check letter.


Note: this is using our internal mod_cache, but looks like the  
issue would be the same in stock mod_cache.




Hmmm. I haven't taken a look yet, but is seems to me that
only complete responses should be cached, not partial, and
as such we need some better mechanism in place for that
Not Cache-able, Could be Cache-able and To-Be-Cached
state tree.

Re: 2.2 mod_http_proxy and partial pages

[Brian Akins]

Jim Jagielski wrote:


Hmmm. I haven't taken a look yet, but is seems to me that
only complete responses should be cached, not partial, and
as such we need some better mechanism in place for that
Not Cache-able, Could be Cache-able and To-Be-Cached
state tree.



From the best I can tell, the issue is in the proxy code.  When a 
response gets truncated for whatever reason, it doesn't pass an error 
along, so the filters never know that something bad happened.



--
Brian Akins
Lead Systems Engineer
CNN Internet Technologies

Re: 2.2 mod_http_proxy and partial pages

[Paul Querna]

Brian Akins wrote:
I have a serious issue.  It seems that if something happens during a 
proxy request after mod_http_proxy starts reading from the backend 
server, no error is reported. (IE, see what happens when 
ap_pass_brigade  returns non success).  The problem is that this 
partial page may be cached because r-status is a cacheable code, 
but the response is actually broken.  The result is that the broken 
version may be cached and served again and again.


Should we break RFC (maybe) and change r-status to 503 (for example) 
so cache can check letter.


Note: this is using our internal mod_cache, but looks like the issue 
would be the same in stock mod_cache.




Related issue:

http://issues.apache.org/bugzilla/show_bug.cgi?id=15866

I don't think its breaking the RFC to not-cache partial pages.

-Paul

Re: 2.2 mod_http_proxy and partial pages

[Brian Akins]

Brian Akins wrote:

 From the best I can tell, the issue is in the proxy code.  When a 
response gets truncated for whatever reason, it doesn't pass an error 
along, so the filters never know that something bad happened.





From mod_proxy_http.c

in the function ap_proxy_http_process_response:

   /* try send what we read */
if (ap_pass_brigade(r-output_filters, bb) != 
APR_SUCCESS

|| c-aborted) {
/* Ack! Phbtt! Die! User aborted! */
backend-close = 1;  /* this causes socket 
close below */

finish = TRUE;
}




It seems this will allow a partial response to be passed down the 
filter chain with nothing noting that it was an error condition.  This 
exact situation seems to be what I am seeing.



Also, we are still using AP_SERVER_BASEVERSION rather than 
ap_get_server_version() for Via.



--
Brian Akins
Lead Systems Engineer
CNN Internet Technologies

Re: 2.2 mod_http_proxy and partial pages

[Brian Akins]

Paul Querna wrote:


Related issue:

http://issues.apache.org/bugzilla/show_bug.cgi?id=15866

I don't think its breaking the RFC to not-cache partial pages.


Yep. That's my issue:

This one doesn't have an easy solution.  The problem is that mod_proxy 
currently
has no way to tell mod_cache if a response terminated abnormally.  We 
could add
some code in mod_cache, to make sure Content-Length matches that actual 
length,

and invalidate the cache at that point.


I would really like to use the stock 2.2 proxy modules, but I cannot 
with this bug.


As a quick fix, could we not have proxy set r-status = HTTP_BAD_GATEWAY 
or something and re-check in the cache before finalizing the store?



--
Brian Akins
Lead Systems Engineer
CNN Internet Technologies

Re: 2.2 mod_http_proxy and partial pages

[Jim Jagielski]

Brian Akins wrote:
 
 Jim Jagielski wrote:
 
  Hmmm. I haven't taken a look yet, but is seems to me that
  only complete responses should be cached, not partial, and
  as such we need some better mechanism in place for that
  Not Cache-able, Could be Cache-able and To-Be-Cached
  state tree.
  
 
  From the best I can tell, the issue is in the proxy code.  When a 
 response gets truncated for whatever reason, it doesn't pass an error 
 along, so the filters never know that something bad happened.
 

Yep... a prelim look indicates that any error within the
proxy isn't bubbled down (or is it up? :) ) to the cache
(or almost anthing else ;) ).

-- 
===
 Jim Jagielski   [|]   [EMAIL PROTECTED]   [|]   http://www.jaguNET.com/
   If you can dodge a wrench, you can dodge a ball.

Re: 2.2 mod_http_proxy and partial pages

[Paul Querna]

Brian Akins wrote:

Paul Querna wrote:


Related issue:

http://issues.apache.org/bugzilla/show_bug.cgi?id=15866

I don't think its breaking the RFC to not-cache partial pages.


Yep. That's my issue:

This one doesn't have an easy solution.  The problem is that mod_proxy 
currently
has no way to tell mod_cache if a response terminated abnormally.  We 
could add
some code in mod_cache, to make sure Content-Length matches that 
actual length,

and invalidate the cache at that point.


I would really like to use the stock 2.2 proxy modules, but I cannot 
with this bug.


As a quick fix, could we not have proxy set r-status = 
HTTP_BAD_GATEWAY or something and re-check in the cache before 
finalizing the store?




Yes, and then remove the problem content and header file.

-Paul

Re: 2.2 mod_http_proxy and partial pages

[Brian Akins]

Brian Akins wrote:

As a quick fix, could we not have proxy set r-status = HTTP_BAD_GATEWAY 
or something and re-check in the cache before finalizing the store?


pseudo code:

in proxy_http

if(some proxy error) {
error_log(error during transit. forcing status change);
r-status = HTTP_GATEWAY_TIME_OUT;  /*(or something not cahceable)*/
}


in mod_cache in store_body check r-status on every bucket?  This may 
need to be in providers for now???






--
Brian Akins
Lead Systems Engineer
CNN Internet Technologies

Re: 2.2 mod_http_proxy and partial pages

[Justin Erenkrantz]

On Tue, Dec 06, 2005 at 09:29:42AM -0500, Brian Akins wrote:
 I have a serious issue.  It seems that if something happens during a 
 proxy request after mod_http_proxy starts reading from the backend 
 server, no error is reported. (IE, see what happens when ap_pass_brigade 
  returns non success).  The problem is that this partial page may be 
 cached because r-status is a cacheable code, but the response is 
 actually broken.  The result is that the broken version may be cached 
 and served again and again.
 
 Should we break RFC (maybe) and change r-status to 503 (for example) so 
 cache can check letter.
 
 Note: this is using our internal mod_cache, but looks like the issue 
 would be the same in stock mod_cache.

Our mod_cache will abort the response if the connection to the original
client is aborted for whatever reason.  So, I'm doubtful the scenario you
describe would happen to our mod_cache.  (See mod_disk_cache.c:1013.)

There *might* be a breakage if the server aborted it's half of the
connection partway through the response.  I don't have the time to fully
look at the code, but there might be a code path that does so.  -- justin

Re: 2.2 mod_http_proxy and partial pages

[Justin Erenkrantz]

On Tue, Dec 06, 2005 at 12:07:32PM -0500, Brian Akins wrote:
 in mod_cache in store_body check r-status on every bucket?  This may 
 need to be in providers for now???

No.  Changing the status after the first write will not matter.  -- justin

Re: 2.2 mod_http_proxy and partial pages

[Brian Akins]

Paul Querna wrote:

As a quick fix, could we not have proxy set r-status = 
HTTP_BAD_GATEWAY or something and re-check in the cache before 
finalizing the store?




Yes, and then remove the problem content and header file.



Since I do not use the stock mod_cache, I cannot really test it. 
However, I can try to get together a patch that changes r-status in 
these cases.  Is that acceptable?  Will this screw up proxy_balancer or 
is it out of the picture by this point?




--
Brian Akins
Lead Systems Engineer
CNN Internet Technologies

Re: 2.2 mod_http_proxy and partial pages

[Justin Erenkrantz]

On Tue, Dec 06, 2005 at 12:10:44PM -0500, Brian Akins wrote:
 Since I do not use the stock mod_cache, I cannot really test it. 
 However, I can try to get together a patch that changes r-status in 
 these cases.  Is that acceptable?  Will this screw up proxy_balancer or 
 is it out of the picture by this point?

Modifying r-status is not an option.  The right thing to do if there is a
backend error reading the response from upstream is for mod_proxy to return
an error as the handler.  Then, mod_cache's error handling code should come
into play and that should trigger the data file never being put into the
correct place.

The code around mod_proxy_http.c:1550 appears to be bogus.  -- justin

Re: 2.2 mod_http_proxy and partial pages

[Brian Akins]

Justin Erenkrantz wrote:


Our mod_cache will abort the response if the connection to the original
client is aborted for whatever reason.  So, I'm doubtful the scenario you
describe would happen to our mod_cache.  (See mod_disk_cache.c:1013.)


Cool. yep that would help me.



There *might* be a breakage if the server aborted it's half of the
connection partway through the response.  I don't have the time to fully
look at the code, but there might be a code path that does so.  -- justin


From what I can tell, stock proxy and cache is vulnerable to that.

On a side note, I would like to aggressively attach the performance 
issues I have with stock mod_cache.  I will hopefully give more details 
after I figure out what to do about this situation.



--
Brian Akins
Lead Systems Engineer
CNN Internet Technologies

Re: 2.2 mod_http_proxy and partial pages

[Justin Erenkrantz]

On Tue, Dec 06, 2005 at 12:22:18PM -0500, Brian Akins wrote:
 There *might* be a breakage if the server aborted it's half of the
 connection partway through the response.  I don't have the time to fully
 look at the code, but there might be a code path that does so.  -- justin
 
 From what I can tell, stock proxy and cache is vulnerable to that.

After a bit more of thinking, the right thing to do would be to have
mod_proxy force a dropped connection to the client.  Since the backend
bailed on us, there's no way for us to finish the response - we've likely
already guaranteed the headers, but have no way to keep the connection
'correct' with respect to the metadata we've already sent.  We must bail.

If mod_proxy forces the dropped connection, this should, in turn, cause
mod_cache to not be able to 'finish' the response.  Therefore, there should
be no cachable partial response to serve.  -- justin

Re: 2.2 mod_http_proxy and partial pages

[Brian Akins]

Justin Erenkrantz wrote:

On Tue, Dec 06, 2005 at 12:22:18PM -0500, Brian Akins wrote:


There *might* be a breakage if the server aborted it's half of the
connection partway through the response.  I don't have the time to fully
look at the code, but there might be a code path that does so.  -- justin


From what I can tell, stock proxy and cache is vulnerable to that.



After a bit more of thinking, the right thing to do would be to have
mod_proxy force a dropped connection to the client.  Since the backend
bailed on us, there's no way for us to finish the response - we've likely
already guaranteed the headers, but have no way to keep the connection
'correct' with respect to the metadata we've already sent.  We must bail.

If mod_proxy forces the dropped connection, this should, in turn, cause
mod_cache to not be able to 'finish' the response.  Therefore, there should
be no cachable partial response to serve.  -- justin


+1

The client is already screwed in this case anyway...

--
Brian Akins
Lead Systems Engineer
CNN Internet Technologies

Re: 2.2 mod_http_proxy and partial pages

[Brian Akins]

Justin Erenkrantz wrote:



After a bit more of thinking, the right thing to do would be to have
mod_proxy force a dropped connection to the client. 


So, do we just need to set r-connection-aborted = 1 and core will take 
 care of it?  If so, a patch should be trivial.


--
Brian Akins
Lead Systems Engineer
CNN Internet Technologies

Re: 2.2 mod_http_proxy and partial pages

[Justin Erenkrantz]

--On December 6, 2005 2:02:02 PM -0500 Brian Akins [EMAIL PROTECTED] 
wrote:



So, do we just need to set r-connection-aborted = 1 and core will take
care of it?  If so, a patch should be trivial.


I think that's a side effect of being aborted.  I think we need to first 
abort the connection and then set that field.  =)  -- justin

Re: 2.2 mod_http_proxy and partial pages

[Brian Akins]

Justin Erenkrantz wrote:

I think that's a side effect of being aborted.  I think we need to first 
abort the connection and then set that field.  =)  -- justin




:) That's what I figured, just greping on aborted isn't showing 
anything obvious...



--
Brian Akins
Lead Systems Engineer
CNN Internet Technologies

Re: 2.2 mod_http_proxy and partial pages

[Brian Akins]

Brian Akins wrote:

Justin Erenkrantz wrote:

I think that's a side effect of being aborted.  I think we need to 
first abort the connection and then set that field.  =)  -- justin




:) That's what I figured, just greping on aborted isn't showing 
anything obvious...





Apparently that's how mod_dav does it.  (see mod_dav.c:4076).  From what 
I can tell some of the core stuff picks up on this.  It can get set in 
server/core_filters.c and http_core.c does something with it


--
Brian Akins
Lead Systems Engineer
CNN Internet Technologies

Re: 2.2 mod_http_proxy and partial pages

[Brian Akins]

Brian Akins wrote:

Justin Erenkrantz wrote:



After a bit more of thinking, the right thing to do would be to have
mod_proxy force a dropped connection to the client. 



So, do we just need to set r-connection-aborted = 1 and core will take 
 care of it?  If so, a patch should be trivial.




After a quick glance through mod_http_proxy, this look to be correct:


--- mod_proxy_http.c.orig   2005-12-06 12:11:19.0 -0500
+++ mod_proxy_http.c2005-12-06 14:06:00.0 -0500
@@ -1482,6 +1482,7 @@
 else if (rv != APR_SUCCESS) {
 ap_log_cerror(APLOG_MARK, APLOG_ERR, rv, c,
   proxy: error reading response);
+c-aborted = 1;
 break;
 }
 /* next time try a non-blocking read */

--
Brian Akins
Lead Systems Engineer
CNN Internet Technologies

Re: 2.2 mod_http_proxy and partial pages

[Justin Erenkrantz]

--On December 6, 2005 2:19:58 PM -0500 Brian Akins [EMAIL PROTECTED] 
wrote:



Brian Akins wrote:

Justin Erenkrantz wrote:



After a bit more of thinking, the right thing to do would be to have
mod_proxy force a dropped connection to the client.



So, do we just need to set r-connection-aborted = 1 and core will take
 care of it?  If so, a patch should be trivial.



After a quick glance through mod_http_proxy, this look to be correct:


Hmm.  Maybe I'm wrong.  That does appear how to set it.  Okie doke.


--- mod_proxy_http.c.orig   2005-12-06 12:11:19.0 -0500
+++ mod_proxy_http.c2005-12-06 14:06:00.0 -0500
@@ -1482,6 +1482,7 @@
  else if (rv != APR_SUCCESS) {
  ap_log_cerror(APLOG_MARK, APLOG_ERR, rv, c,
proxy: error reading response);
+c-aborted = 1;
  break;
  }
  /* next time try a non-blocking read */


I do think we need to fix mod_proxy_http to return an error.

Like so.  -- justin

Index: modules/proxy/mod_proxy_http.c
===
--- modules/proxy/mod_proxy_http.c  (revision 354485)
+++ modules/proxy/mod_proxy_http.c  (working copy)
@@ -1565,8 +1565,14 @@
   }
return status;
}
-} else
-return OK;
+} else {
+if (c-aborted) {
+return DONE;
+}
+else {
+return OK;
+}
+}
}

static

Re: 2.2 mod_http_proxy and partial pages

[Jim Jagielski]

Justin Erenkrantz wrote:
 
 --On December 6, 2005 2:02:02 PM -0500 Brian Akins [EMAIL PROTECTED] 
 wrote:
 
  So, do we just need to set r-connection-aborted = 1 and core will take
  care of it?  If so, a patch should be trivial.
 
 I think that's a side effect of being aborted.  I think we need to first 
 abort the connection and then set that field.  =)  -- justin
 

+1 to setting c-aborted to 1 in this case.

-- 
===
 Jim Jagielski   [|]   [EMAIL PROTECTED]   [|]   http://www.jaguNET.com/
   If you can dodge a wrench, you can dodge a ball.

Re: 2.2 mod_http_proxy and partial pages

[Brian Akins]

Justin Erenkrantz wrote:



I do think we need to fix mod_proxy_http to return an error.



So we need my patch and your patch, right?  I'm a little medicated 
today...



--
Brian Akins
Lead Systems Engineer
CNN Internet Technologies

Re: 2.2 mod_http_proxy and partial pages

[Justin Erenkrantz]

--On December 6, 2005 3:01:28 PM -0500 Brian Akins [EMAIL PROTECTED] 
wrote:



So we need my patch and your patch, right?  I'm a little medicated
today...


I think so, yes.  -- justin

Re: 2.2 mod_http_proxy and partial pages

[Jim Jagielski]

Brian Akins wrote:
 
 Justin Erenkrantz wrote:
 
  
  I do think we need to fix mod_proxy_http to return an error.
 
 
 So we need my patch and your patch, right?  I'm a little medicated 
 today...
 

Yes, both.

-- 
===
 Jim Jagielski   [|]   [EMAIL PROTECTED]   [|]   http://www.jaguNET.com/
   If you can dodge a wrench, you can dodge a ball.

Re: 2.2 mod_http_proxy and partial pages

[Brian Akins]

Jim Jagielski wrote:

So we need my patch and your patch, right?  I'm a little medicated 
today...





Yes, both.



Can we vote on this?  I guess we do it for HEAD then for 2.2.1 (or 
something like that)?


Just want to make sure this will make into stock code.  In the 
meantime, I'll do a local patch and test.


Thanks.


--
Brian Akins
Lead Systems Engineer
CNN Internet Technologies