Re: Memory usage, core output filter, and apr_brigade_destroy

[Stefan Fritsch]

Thanks for your comments.

On Wednesday 23 September 2009, Ruediger Pluem wrote:
 --- modules/http/chunk_filter.c (Revision 818232)
 +++ modules/http/chunk_filter.c (Arbeitskopie)
 @@ -49,11 +49,11 @@
  #define ASCII_CRLF  \015\012
  #define ASCII_ZERO  \060
  conn_rec *c = f-r-connection;
 -apr_bucket_brigade *more;
 +apr_bucket_brigade *more, *tmp;
  apr_bucket *e;
  apr_status_t rv;
 
 -for (more = NULL; b; b = more, more = NULL) {
 +for (more = tmp = NULL; b; tmp = b, b = more, more = NULL) {
  apr_off_t bytes = 0;
  apr_bucket *eos = NULL;
  apr_bucket *flush = NULL;
 @@ -85,7 +85,8 @@
  if (APR_BUCKET_IS_FLUSH(e)) {
  flush = e;
  if (e != APR_BRIGADE_LAST(b)) {
 -more = apr_brigade_split(b,
  APR_BUCKET_NEXT(e)); +more =
  apr_brigade_split_ex(b, APR_BUCKET_NEXT(e), tmp); +   
  tmp = NULL;
  }
  break;
  }
 @@ -105,7 +106,8 @@
   * block so we pass down what we have so far.
   */
  bytes += len;
 -more = apr_brigade_split(b,
  APR_BUCKET_NEXT(e)); +more =
  apr_brigade_split_ex(b, APR_BUCKET_NEXT(e), tmp); +   
  tmp = NULL;
  break;
  }
  else {
 
 What is the point here? tmp is always NULL when passed to
  apr_brigade_split_ex so apr_brigade_split_ex == apr_brigade_split

You missed the tmp = b at the beginning of the loop. The tmp = NULL 
lines were actually not needed. I have changed this a bit to hopefully 
make it clearer and commited the patch together with the other changes 
you suggested.

Cheers,
Stefan

Re: Memory usage, core output filter, and apr_brigade_destroy

[Ruediger Pluem]

On 10/04/2009 09:37 AM, Stefan Fritsch wrote:
 Thanks for your comments.
 
 On Wednesday 23 September 2009, Ruediger Pluem wrote:


 What is the point here? tmp is always NULL when passed to
  apr_brigade_split_ex so apr_brigade_split_ex == apr_brigade_split
 
 You missed the tmp = b at the beginning of the loop. The tmp = NULL 
 lines were actually not needed. I have changed this a bit to hopefully 
 make it clearer and commited the patch together with the other changes 
 you suggested.

Yes, I missed that. Thanks for explaining.

Regards

Rüdiger

Re: Memory usage, core output filter, and apr_brigade_destroy

[Ruediger Pluem]

On 09/22/2009 09:19 PM, Stefan Fritsch wrote:
 On Sunday 13 September 2009, Stefan Fritsch wrote:
 On Sunday 13 September 2009, Ruediger Pluem wrote:
 On 09/13/2009 01:11 PM, Stefan Fritsch wrote:
 http://httpd.apache.org/docs/trunk/developer/output-filters.htm
 l recommends to reuse bucket brigades and to not use
 apr_brigade_destroy. However, both in 2.2 and in trunk, the
 core output filter sometimes calls apr_brigade_destroy on
 brigades that it has received down the chain from earlier
 output filters. Is this not bound to cause problems since the
 brigade's pool cleanup is then removed but the brigade is still
 reused later on?
 It could be. But the questions is if it is really reused later
 on.
 Since this is the recommended design for output filters, I am sure
  it can happen.
 
 I have attached two patches:
 
 In the first, I change (hopefully) all places to not 
 apr_brigade_destroy brigades that have been passed down the filter 
 chain. Especially the core_output_filter change needs some review.
 
 In the second, I have changed all occurences of apr_brigade_split to 
 apr_brigade_split_ex and I have made some more changes where bucket 
 brigades can be reused.
 
 The test suite shows no regressions.


Index: server/protocol.c
===
--- server/protocol.c   (Revision 818232)
+++ server/protocol.c   (Arbeitskopie)
@@ -1239,6 +1239,7 @@
  * least one bucket on to the next output filter
  * for this request
  */
+apr_bucket_brigade *tmpbb;
 };

 /* This filter computes the content length, but it also computes the number
@@ -1259,6 +1260,8 @@
 if (!ctx) {
 f-ctx = ctx = apr_palloc(r-pool, sizeof(*ctx));
 ctx-data_sent = 0;
+ctx-tmpbb = apr_brigade_create(r-connection-pool,

This should be r-pool instead. The lifetime of ctx itself is limited to r-pool

+r-connection-bucket_alloc);
 }


@@ -1433,24 +1431,38 @@
 if (f == NULL) {
 /* our filter hasn't been added yet */
 ctx = apr_pcalloc(r-pool, sizeof(*ctx));
+ctx-tmpbb = apr_brigade_create(r-pool, r-connection-bucket_alloc);
+
 ap_add_output_filter(OLD_WRITE, ctx, r, r-connection);
 f = r-output_filters;
 }

+return f;
+}
+
+static apr_status_t buffer_output(request_rec *r,
+  const char *str, apr_size_t len)
+{
+conn_rec *c = r-connection;
+ap_filter_t *f;
+old_write_filter_ctx *ctx;
+
+if (len == 0)
+return APR_SUCCESS;
+
+f = insert_old_write_filter(r);
+ctx = f-ctx;
+
 /* if the first filter is not our buffering filter, then we have to
  * deliver the content through the normal filter chain
  */
 if (f != r-output_filters) {
-apr_bucket_brigade *bb = apr_brigade_create(r-pool, c-bucket_alloc);
 apr_bucket *b = apr_bucket_transient_create(str, len, c-bucket_alloc);
-APR_BRIGADE_INSERT_TAIL(bb, b);
+APR_BRIGADE_INSERT_TAIL(ctx-tmpbb, b);

-return ap_pass_brigade(r-output_filters, bb);
+return ap_pass_brigade(r-output_filters, ctx-tmpbb);

To be on the save side I think an apr_brigade_cleanup(ctx-tmpbb) is due
before doing the return, just in case a filter did not consume the buckets
properly.

 }
@@ -1605,13 +1617,17 @@
 AP_DECLARE(int) ap_rflush(request_rec *r)
 {
 conn_rec *c = r-connection;
-apr_bucket_brigade *bb;
 apr_bucket *b;
+ap_filter_t *f;
+old_write_filter_ctx *ctx;

-bb = apr_brigade_create(r-pool, c-bucket_alloc);
+f = insert_old_write_filter(r);
+ctx = f-ctx;
+
 b = apr_bucket_flush_create(c-bucket_alloc);
-APR_BRIGADE_INSERT_TAIL(bb, b);
-if (ap_pass_brigade(r-output_filters, bb) != APR_SUCCESS)
+APR_BRIGADE_INSERT_TAIL(ctx-tmpbb, b);
+
+if (ap_pass_brigade(r-output_filters, ctx-tmpbb) != APR_SUCCESS)

Same as above: We should call apr_brigade_cleanup(ctx-tmpbb) to be on the save 
side.

 return -1;

 return 0;


--- modules/http/chunk_filter.c (Revision 818232)
+++ modules/http/chunk_filter.c (Arbeitskopie)
@@ -49,11 +49,11 @@
 #define ASCII_CRLF  \015\012
 #define ASCII_ZERO  \060
 conn_rec *c = f-r-connection;
-apr_bucket_brigade *more;
+apr_bucket_brigade *more, *tmp;
 apr_bucket *e;
 apr_status_t rv;

-for (more = NULL; b; b = more, more = NULL) {
+for (more = tmp = NULL; b; tmp = b, b = more, more = NULL) {
 apr_off_t bytes = 0;
 apr_bucket *eos = NULL;
 apr_bucket *flush = NULL;
@@ -85,7 +85,8 @@
 if (APR_BUCKET_IS_FLUSH(e)) {
 flush = e;
 if (e != APR_BRIGADE_LAST(b)) {
-more = apr_brigade_split(b, APR_BUCKET_NEXT(e));
+more = apr_brigade_split_ex(b, APR_BUCKET_NEXT(e), tmp);
+tmp = NULL;
 }
 break;

Re: Memory usage, core output filter, and apr_brigade_destroy

[Stefan Fritsch]

On Sunday 13 September 2009, Stefan Fritsch wrote:
 On Sunday 13 September 2009, Ruediger Pluem wrote:
  On 09/13/2009 01:11 PM, Stefan Fritsch wrote:
   http://httpd.apache.org/docs/trunk/developer/output-filters.htm
  l recommends to reuse bucket brigades and to not use
   apr_brigade_destroy. However, both in 2.2 and in trunk, the
   core output filter sometimes calls apr_brigade_destroy on
   brigades that it has received down the chain from earlier
   output filters. Is this not bound to cause problems since the
   brigade's pool cleanup is then removed but the brigade is still
   reused later on?
 
  It could be. But the questions is if it is really reused later
  on.
 
 Since this is the recommended design for output filters, I am sure
  it can happen.

I have attached two patches:

In the first, I change (hopefully) all places to not 
apr_brigade_destroy brigades that have been passed down the filter 
chain. Especially the core_output_filter change needs some review.

In the second, I have changed all occurences of apr_brigade_split to 
apr_brigade_split_ex and I have made some more changes where bucket 
brigades can be reused.

The test suite shows no regressions.

Cheers,
Stefan
diff --git a/modules/http/byterange_filter.c b/modules/http/byterange_filter.c
index a79b7f7..92048ab 100644
--- a/modules/http/byterange_filter.c
+++ b/modules/http/byterange_filter.c
@@ -307,7 +307,7 @@ AP_CORE_DECLARE_NONSTD(apr_status_t) ap_byterange_filter(ap_filter_t *f,
 APR_BRIGADE_INSERT_TAIL(bsend, e);
 
 /* we're done with the original content - all of our data is in bsend. */
-apr_brigade_destroy(bb);
+apr_brigade_cleanup(bb);
 
 /* send our multipart output */
 return ap_pass_brigade(f-next, bsend);
diff --git a/modules/http/http_filters.c b/modules/http/http_filters.c
index 3e96cb9..01ced24 100644
--- a/modules/http/http_filters.c
+++ b/modules/http/http_filters.c
@@ -1112,7 +1112,7 @@ AP_CORE_DECLARE_NONSTD(apr_status_t) ap_http_header_filter(ap_filter_t *f,
 ctx = f-ctx = apr_pcalloc(r-pool, sizeof(header_filter_ctx));
 }
 else if (ctx-headers_sent) {
-apr_brigade_destroy(b);
+apr_brigade_cleanup(b);
 return OK;
 }
 }
@@ -1283,7 +1283,7 @@ AP_CORE_DECLARE_NONSTD(apr_status_t) ap_http_header_filter(ap_filter_t *f,
 ap_pass_brigade(f-next, b2);
 
 if (r-header_only) {
-apr_brigade_destroy(b);
+apr_brigade_cleanup(b);
 ctx-headers_sent = 1;
 return OK;
 }
diff --git a/server/core_filters.c b/server/core_filters.c
index f073765..eb206c1 100644
--- a/server/core_filters.c
+++ b/server/core_filters.c
@@ -316,7 +316,7 @@ int ap_core_input_filter(ap_filter_t *f, apr_bucket_brigade *b,
 static void setaside_remaining_output(ap_filter_t *f,
   core_output_filter_ctx_t *ctx,
   apr_bucket_brigade *bb,
-  int make_a_copy, conn_rec *c);
+  conn_rec *c);
 
 static apr_status_t send_brigade_nonblocking(apr_socket_t *s,
  apr_bucket_brigade *bb,
@@ -392,19 +392,21 @@ apr_status_t ap_core_output_filter(ap_filter_t *f, apr_bucket_brigade *new_bb)
 }
 }
 
+if (new_bb != NULL) {
+bb = new_bb;
+}
+
 if ((ctx-buffered_bb != NULL) 
 !APR_BRIGADE_EMPTY(ctx-buffered_bb)) {
-bb = ctx-buffered_bb;
-ctx-buffered_bb = NULL;
 if (new_bb != NULL) {
-APR_BRIGADE_CONCAT(bb, new_bb);
+APR_BRIGADE_PREPEND(bb, ctx-buffered_bb);
+}
+else {
+bb = ctx-buffered_bb;
 }
 c-data_in_output_filters = 0;
 }
-else if (new_bb != NULL) {
-bb = new_bb;
-}
-else {
+else if (new_bb == NULL) {
 return APR_SUCCESS;
 }
 
@@ -444,7 +446,7 @@ apr_status_t ap_core_output_filter(ap_filter_t *f, apr_bucket_brigade *new_bb)
 /* The client has aborted the connection */
 c-aborted = 1;
 }
-setaside_remaining_output(f, ctx, bb, 0, c);
+setaside_remaining_output(f, ctx, bb, c);
 return rv;
 }
 
@@ -508,14 +519,14 @@ apr_status_t ap_core_output_filter(ap_filter_t *f, apr_bucket_brigade *new_bb)
 }
 }
 
-setaside_remaining_output(f, ctx, bb, 1, c);
+setaside_remaining_output(f, ctx, bb, c);
 return APR_SUCCESS;
 }
 
 static void setaside_remaining_output(ap_filter_t *f,
   core_output_filter_ctx_t *ctx,
   apr_bucket_brigade *bb,
-  int make_a_copy, conn_rec *c)
+  conn_rec *c)
 {
 if (bb == NULL) {
 return;
@@ -523,20 +534,14 @@ static void setaside_remaining_output(ap_filter_t *f,
 remove_empty_buckets(bb);
 if 

Re: Memory usage, core output filter, and apr_brigade_destroy

[Stefan Fritsch]

On Sun, 13 Sep 2009, Ruediger Pluem wrote:

But your patch is causing core dumps during the proxy tests when
running the test suite :-(.
I currently don't understand why.



Hmmm... either ctx-tmp_flush_bb is NULL or, since it was added in the
middle of the struct, you didn't do a make distclean 1st


That is not the problem. I did a slightly modified patch that added it to the
end. I suppose it has something to do with not matching pools or bucket
allocators between bb and ctx-tmp_flush_bb.
It fails on in the proxy case and in the proxy case we have some mixtures going
on there regarding pools and bucket allocators caused by the pooled backend
connections.


Yes, the lifetime of the brigade was wrong. The attached patch works 
without segfaults.


Cheers,
Stefandiff --git a/include/httpd.h b/include/httpd.h
index a7a7025..cd47b11 100644
--- a/include/httpd.h
+++ b/include/httpd.h
@@ -1241,6 +1241,7 @@ typedef struct core_output_filter_ctx {
 apr_bucket_brigade *buffered_bb;
 apr_size_t bytes_in;
 apr_size_t bytes_written;
+apr_bucket_brigade *tmp_flush_bb;
 } core_output_filter_ctx_t;
  
 typedef struct core_filter_ctx {
diff --git a/server/core_filters.c b/server/core_filters.c
index d9b6eb0..2f60d04 100644
--- a/server/core_filters.c
+++ b/server/core_filters.c
@@ -468,7 +468,15 @@ apr_status_t ap_core_output_filter(ap_filter_t *f, apr_bucket_brigade *new_bb)
  bucket = next) {
 next = APR_BUCKET_NEXT(bucket);
 if (APR_BUCKET_IS_FLUSH(bucket)) {
-apr_bucket_brigade *remainder = apr_brigade_split(bb, next);
+if (!ctx-tmp_flush_bb) {
+/*
+ * Need to create tmp brigade with correct lifetime. Passing
+ * NULL to apr_brigade_split_ex would result in a brigade
+ * allocated from a different pool.
+ */
+ctx-tmp_flush_bb = apr_brigade_create(c-pool, c-bucket_alloc);
+}
+ctx-tmp_flush_bb = apr_brigade_split_ex(bb, next, ctx-tmp_flush_bb);
 apr_status_t rv = send_brigade_blocking(net-client_socket, bb,
 (ctx-bytes_written), c);
 if (rv != APR_SUCCESS) {
@@ -476,7 +484,7 @@ apr_status_t ap_core_output_filter(ap_filter_t *f, apr_bucket_brigade *new_bb)
 c-aborted = 1;
 return rv;
 }
-bb = remainder;
+APR_BRIGADE_CONCAT(bb, ctx-tmp_flush_bb);
 next = APR_BRIGADE_FIRST(bb);
 bytes_in_brigade = 0;
 non_file_bytes_in_brigade = 0;

Re: Memory usage, core output filter, and apr_brigade_destroy

[Ruediger Pluem]

On 09/14/2009 07:32 PM, Stefan Fritsch wrote:
 On Sun, 13 Sep 2009, Ruediger Pluem wrote:
 But your patch is causing core dumps during the proxy tests when
 running the test suite :-(.
 I currently don't understand why.


 Hmmm... either ctx-tmp_flush_bb is NULL or, since it was added in the
 middle of the struct, you didn't do a make distclean 1st

 That is not the problem. I did a slightly modified patch that added it
 to the
 end. I suppose it has something to do with not matching pools or bucket
 allocators between bb and ctx-tmp_flush_bb.
 It fails on in the proxy case and in the proxy case we have some
 mixtures going
 on there regarding pools and bucket allocators caused by the pooled
 backend
 connections.
 
 Yes, the lifetime of the brigade was wrong. The attached patch works
 without segfaults.

Thanks for the update. I committed a slightly modified version as
r814807. It avoids the constant if check in the flush bucket case
at the expense of always creating the brigade when setting up the
context.

Regards

Rüdiger

Re: Memory usage, core output filter, and apr_brigade_destroy

[Paul Querna]

On Mon, Sep 14, 2009 at 12:48 PM, Ruediger Pluem rpl...@apache.org wrote:


 On 09/14/2009 07:32 PM, Stefan Fritsch wrote:
 On Sun, 13 Sep 2009, Ruediger Pluem wrote:
 But your patch is causing core dumps during the proxy tests when
 running the test suite :-(.
 I currently don't understand why.


 Hmmm... either ctx-tmp_flush_bb is NULL or, since it was added in the
 middle of the struct, you didn't do a make distclean 1st

 That is not the problem. I did a slightly modified patch that added it
 to the
 end. I suppose it has something to do with not matching pools or bucket
 allocators between bb and ctx-tmp_flush_bb.
 It fails on in the proxy case and in the proxy case we have some
 mixtures going
 on there regarding pools and bucket allocators caused by the pooled
 backend
 connections.

 Yes, the lifetime of the brigade was wrong. The attached patch works
 without segfaults.

 Thanks for the update. I committed a slightly modified version as
 r814807. It avoids the constant if check in the flush bucket case
 at the expense of always creating the brigade when setting up the
 context.


regarding r814807, if you look in core_fitlers.c, there is a
brigade_move function that pre-dated apr_brigade_split_ex existing, I
think brigade_move could be converted to  apr_brigade_split_ex...

Re: Memory usage, core output filter, and apr_brigade_destroy

[Ruediger Pluem]

On 09/14/2009 10:29 PM, Paul Querna wrote:
 On Mon, Sep 14, 2009 at 12:48 PM, Ruediger Pluem rpl...@apache.org wrote:

 On 09/14/2009 07:32 PM, Stefan Fritsch wrote:
 On Sun, 13 Sep 2009, Ruediger Pluem wrote:
 But your patch is causing core dumps during the proxy tests when
 running the test suite :-(.
 I currently don't understand why.

 Hmmm... either ctx-tmp_flush_bb is NULL or, since it was added in the
 middle of the struct, you didn't do a make distclean 1st
 That is not the problem. I did a slightly modified patch that added it
 to the
 end. I suppose it has something to do with not matching pools or bucket
 allocators between bb and ctx-tmp_flush_bb.
 It fails on in the proxy case and in the proxy case we have some
 mixtures going
 on there regarding pools and bucket allocators caused by the pooled
 backend
 connections.
 Yes, the lifetime of the brigade was wrong. The attached patch works
 without segfaults.
 Thanks for the update. I committed a slightly modified version as
 r814807. It avoids the constant if check in the flush bucket case
 at the expense of always creating the brigade when setting up the
 context.

 
 regarding r814807, if you look in core_fitlers.c, there is a
 brigade_move function that pre-dated apr_brigade_split_ex existing, I
 think brigade_move could be converted to  apr_brigade_split_ex...

Good catch. They are nearly the same with the exception that
apr_brigrade_split_ex cleans up brigade a where the tail ends if it
is not empty at the beginning, but this is IHMO a benefit.

Regards

Rüdiger

Re: Memory usage, core output filter, and apr_brigade_destroy

[Ruediger Pluem]

On 09/13/2009 01:11 PM, Stefan Fritsch wrote:
 Hi,
 
 http://httpd.apache.org/docs/trunk/developer/output-filters.html
 recommends to reuse bucket brigades and to not use apr_brigade_destroy.
 However, both in 2.2 and in trunk, the core output filter sometimes
 calls apr_brigade_destroy on brigades that it has received down the
 chain from earlier output filters. Is this not bound to cause problems
 since the brigade's pool cleanup is then removed but the brigade is
 still reused later on?

It could be. But the questions is if it is really reused later on.

 
 Also, the core output filter often creates new brigades instead of
 reusing an existing brigade. This should also be changed to reduce
 memory usage, shouldn't it?

Yes. That was the reason for adding apr_brigade_split_ex to apr-util 1.3.
But so far nobody has found time to get through the current httpd code
and improve the needed sections by using it.

 
 For trunk, the attached patch at least keeps the temporary brigade for
 flush buckets around. Do the versioning rules allow to add elements to
 core_output_filter_ctx for 2.2.x, too? It's defined in httpd.h.

Yes, if you add it to the end of the struct. Then only a minor bump is
required which is allowed. The other possibly more formal problem is
that we currently do not require apr / apr-util 1.3 for httpd 2.2.x but only
1.2. But after shipping 2.2.x for more then one year with apr / apr-util 1.3
without problems I guess we can change this and require apr / apr-util 1.3
for the next 2.2.x release.

But your patch is causing core dumps during the proxy tests when
running the test suite :-(.
I currently don't understand why.

Regards

Rüdiger

Re: Memory usage, core output filter, and apr_brigade_destroy

[Jim Jagielski]

On Sep 13, 2009, at 8:48 AM, Ruediger Pluem wrote:




On 09/13/2009 01:11 PM, Stefan Fritsch wrote:

Hi,

http://httpd.apache.org/docs/trunk/developer/output-filters.html
recommends to reuse bucket brigades and to not use  
apr_brigade_destroy.

However, both in 2.2 and in trunk, the core output filter sometimes
calls apr_brigade_destroy on brigades that it has received down the
chain from earlier output filters. Is this not bound to cause  
problems

since the brigade's pool cleanup is then removed but the brigade is
still reused later on?


It could be. But the questions is if it is really reused later on.



Also, the core output filter often creates new brigades instead of
reusing an existing brigade. This should also be changed to reduce
memory usage, shouldn't it?


Yes. That was the reason for adding apr_brigade_split_ex to apr-util  
1.3.

But so far nobody has found time to get through the current httpd code
and improve the needed sections by using it.



For trunk, the attached patch at least keeps the temporary brigade  
for
flush buckets around. Do the versioning rules allow to add elements  
to

core_output_filter_ctx for 2.2.x, too? It's defined in httpd.h.


Yes, if you add it to the end of the struct. Then only a minor bump is
required which is allowed. The other possibly more formal problem is
that we currently do not require apr / apr-util 1.3 for httpd 2.2.x  
but only
1.2. But after shipping 2.2.x for more then one year with apr / apr- 
util 1.3
without problems I guess we can change this and require apr / apr- 
util 1.3

for the next 2.2.x release.

But your patch is causing core dumps during the proxy tests when
running the test suite :-(.
I currently don't understand why.



Hmmm... either ctx-tmp_flush_bb is NULL or, since it was added in the
middle of the struct, you didn't do a make distclean 1st

Re: Memory usage, core output filter, and apr_brigade_destroy

[Ruediger Pluem]

On 09/13/2009 05:12 PM, Jim Jagielski wrote:
 
 On Sep 13, 2009, at 8:48 AM, Ruediger Pluem wrote:
 


 On 09/13/2009 01:11 PM, Stefan Fritsch wrote:
 Hi,

 http://httpd.apache.org/docs/trunk/developer/output-filters.html
 recommends to reuse bucket brigades and to not use apr_brigade_destroy.
 However, both in 2.2 and in trunk, the core output filter sometimes
 calls apr_brigade_destroy on brigades that it has received down the
 chain from earlier output filters. Is this not bound to cause problems
 since the brigade's pool cleanup is then removed but the brigade is
 still reused later on?

 It could be. But the questions is if it is really reused later on.


 Also, the core output filter often creates new brigades instead of
 reusing an existing brigade. This should also be changed to reduce
 memory usage, shouldn't it?

 Yes. That was the reason for adding apr_brigade_split_ex to apr-util 1.3.
 But so far nobody has found time to get through the current httpd code
 and improve the needed sections by using it.


 For trunk, the attached patch at least keeps the temporary brigade for
 flush buckets around. Do the versioning rules allow to add elements to
 core_output_filter_ctx for 2.2.x, too? It's defined in httpd.h.

 Yes, if you add it to the end of the struct. Then only a minor bump is
 required which is allowed. The other possibly more formal problem is
 that we currently do not require apr / apr-util 1.3 for httpd 2.2.x
 but only
 1.2. But after shipping 2.2.x for more then one year with apr /
 apr-util 1.3
 without problems I guess we can change this and require apr / apr-util
 1.3
 for the next 2.2.x release.

 But your patch is causing core dumps during the proxy tests when
 running the test suite :-(.
 I currently don't understand why.

 
 Hmmm... either ctx-tmp_flush_bb is NULL or, since it was added in the
 middle of the struct, you didn't do a make distclean 1st

That is not the problem. I did a slightly modified patch that added it to the
end. I suppose it has something to do with not matching pools or bucket
allocators between bb and ctx-tmp_flush_bb.
It fails on in the proxy case and in the proxy case we have some mixtures going
on there regarding pools and bucket allocators caused by the pooled backend
connections.


Regards

Rüdiger

Re: Memory usage, core output filter, and apr_brigade_destroy

[Stefan Fritsch]

Hi Rüdiger,

thanks for the response.

On Sunday 13 September 2009, Ruediger Pluem wrote:
 On 09/13/2009 01:11 PM, Stefan Fritsch wrote:
  http://httpd.apache.org/docs/trunk/developer/output-filters.html
  recommends to reuse bucket brigades and to not use
  apr_brigade_destroy. However, both in 2.2 and in trunk, the core
  output filter sometimes calls apr_brigade_destroy on brigades
  that it has received down the chain from earlier output filters.
  Is this not bound to cause problems since the brigade's pool
  cleanup is then removed but the brigade is still reused later on?
 
 It could be. But the questions is if it is really reused later on.

Since this is the recommended design for output filters, I am sure it 
can happen.

 But your patch is causing core dumps during the proxy tests when
 running the test suite :-(.

It seems I should install the test suite, too. I will look at it when 
I have some free cycles again.

Cheers,
Stefan