On 12/08/2007 04:07 PM, Jim Jagielski wrote:
> I didn't see this patch in the commit list but did see it referred
> to in the 2.2 STATUS file... I'm reviewing the patch now but two
> things did stick out:
>
> - apr_brigade_cleanup(ctx->ctxbb);
> - APR_BUCKET_REMOVE(b);
> - APR_BRIGADE_INSERT_TAIL(passbb, b);
> - break;
> - }
> - else if (APR_BUCKET_IS_FLUSH(b)) {
> + apr_brigade_cleanup(ctx->linebb);
> APR_BUCKET_REMOVE(b);
> - APR_BRIGADE_INSERT_TAIL(passbb, b);
> - rv = ap_pass_brigade(f->next, passbb);
> - apr_brigade_cleanup(passbb);
> - if (rv != APR_SUCCESS)
> - return rv;
> + APR_BRIGADE_INSERT_TAIL(ctx->passbb, b);
> }
> + /*
> + * No need to handle FLUSH buckets separately as we call
> + * ap_pass_brigade anyway at the end of the loop.
> + */
> else if (APR_BUCKET_IS_METADATA(b)) {
> APR_BUCKET_REMOVE(b);
> - APR_BRIGADE_INSERT_TAIL(passbb, b);
> + APR_BRIGADE_INSERT_TAIL(ctx->passbb, b);
>
> The reason why the current code handles FLUSH separately
> is though, yes, the ap_pass_brigade is done at the end of
> the while loop, that is *only* done when we're done handling
> the full brigade... The intent was to honor flushes in the
> brigade "immediately" and "reset" at that point... Not sure
> why this was removed, since the old way is more efficient.
>
In fact the patch does all this as it passes the passbb brigade down
the chain after *each* processed bucket of the original brigade
(the ap_pass_brigade is at *the end* of the while loop *not* after
the while loop).
So we don't process the whole brigade before passing it down the
chain which would be indeed insane.
In fact flush buckets are handled in the same manner as before
the patch as they fall in the
if (APR_BUCKET_IS_METADATA(b))
branch and the next code that is executed after the block there
is an ap_pass_brigade at the end of the while loop
> I also see that AP_MIN_BYTES_TO_WRITE is no longer being
I saw no need for us to do *any* buffering in the filter. If the
data passed down the chain is not reasonable large for sending over
the wire the core network filter will take care of this and buffer
it until it is reasonable or a flush bucket is seen.
> used at all... I'm guessing MAX_BUCKETS is designed to
> "replace" that?? Again, the idea is to avoid extremely
> large in-process data sizes :) The MAX_BUCKETS seems
The patch does that :-).
> to be mostly for super-bad cases and not so much for
> behaving "nicely" in all cases. (mod_include.c does
> the same thing, btw)
No MAX_BUCKETS should not replace that. It is a safety measure
for super bad cases like the example I gave in my initial review
mail
(http://mail-archives.apache.org/mod_mbox/httpd-dev/200712.mbox/[EMAIL
PROTECTED]).
Regards
Rüdiger
