Re: svn commit: r1707163 - /httpd/httpd/trunk/server/util_filter.c

[Graham Leggett]

On 07 Oct 2015, at 1:54 AM, Yann Ylavic  wrote:

> I don't understand this, why using ap_save_brigade() for both cases
> (i.e. setting aside or reading the other types of buckets) would lead
> to synchronous behaviour?
> It is when r->pool gets destroyed?
> If so, how is it different from the buckets currently set aside on r->pool?
> 
> We could also possibly always use the deferred_pool (moving it to
> conn_rec if we don't want one per reinstate-able filter).

It’s because of this code here:

rv = apr_bucket_setaside(e, p);

/* If the bucket type does not implement setaside, then
 * (hopefully) morph it into a bucket type which does, and set
 * *that* aside... */
if (rv == APR_ENOTIMPL) {
const char *s;
apr_size_t n;

rv = apr_bucket_read(e, , , APR_BLOCK_READ);
if (rv == APR_SUCCESS) {
rv = apr_bucket_setaside(e, p);
}
}

This code reads morphing buckets into RAM, and in order to not overwhelm the 
server the server is forced to block to protect itself from out-of-memory.

The actual block is triggered by ap_filter_reinstate_brigade(), which sees the 
morphing bucket and marks it to be flushed, unless the bucket belongs to a 
request, in which case it can safely be buffered for the next pass without 
fiddling with the bucket at all.

The problem comes in with transient buckets that refer to memory that stops 
existing when the call eventually returns. Given that transient buckets are 
memory resident by definition, moving them to the heap is not an issue.

In order to understand how this works you need to look at all three of 
ap_filter_reinstate_brigade(), ap_filter_should_yield() and 
ap_filter_setaside_brigade() in detail, as they work together.

These three functions were implemented in the core network filter years ago and 
the concept is well tested. All this patch does is generalise this concept to 
other filters over and over the network filter.

Regards,
Graham
—

Re: svn commit: r1707163 - /httpd/httpd/trunk/server/util_filter.c

[Yann Ylavic]

On Wed, Oct 7, 2015 at 12:38 AM,   wrote:
> Author: minfrin
> Date: Tue Oct  6 22:38:28 2015
> New Revision: 1707163
>
> URL: http://svn.apache.org/viewvc?rev=1707163=rev
> Log:
> Make sure that transient buckets are morphed into real buckets before
> the filter returns.
>
> Modified:
> httpd/httpd/trunk/server/util_filter.c
>
> Modified: httpd/httpd/trunk/server/util_filter.c
> URL: 
> http://svn.apache.org/viewvc/httpd/httpd/trunk/server/util_filter.c?rev=1707163=1707162=1707163=diff
> ==
> --- httpd/httpd/trunk/server/util_filter.c (original)
> +++ httpd/httpd/trunk/server/util_filter.c Tue Oct  6 22:38:28 2015
> @@ -736,6 +736,16 @@ AP_DECLARE(apr_status_t) ap_filter_setas
>
>  /* decide what pool we setaside to, request pool or deferred pool? */
>  if (f->r) {
> +apr_bucket *e;
> +for (e = APR_BRIGADE_FIRST(bb); e != APR_BRIGADE_SENTINEL(bb); e 
> =
> +APR_BUCKET_NEXT(e)) {
> +if (APR_BUCKET_IS_TRANSIENT(e)) {
> +int rv = apr_bucket_setaside(e, f->r->pool);
> +if (rv != APR_SUCCESS) {
> +return rv;
> +}
> +}
> +}
>  pool = f->r->pool;
>  APR_BRIGADE_CONCAT(f->bb, bb);
>  }

I don't understand this, why using ap_save_brigade() for both cases
(i.e. setting aside or reading the other types of buckets) would lead
to synchronous behaviour?
It is when r->pool gets destroyed?
If so, how is it different from the buckets currently set aside on r->pool?

We could also possibly always use the deferred_pool (moving it to
conn_rec if we don't want one per reinstate-able filter).

Regards,
Yann.