On Mon, Jul 19, 2004 at 03:47:45PM +0100, Nick Kew wrote: > On Mon, 19 Jul 2004, Joe Orton wrote: > > > On Sat, Jul 17, 2004 at 03:22:35PM -0000, [EMAIL PROTECTED] wrote: > > > niq 2004/07/17 08:22:35 > > > > > > Modified: modules/filters mod_deflate.c > > > Log: > > > Fix previous patch to deal correctly with multiple empty brigades before > > > we know if there's any content, and not re-process the headers. > > > > Is there no simpler fix for this e.g. first thing the filter does is "if > > (APR_BRIGADE_EMPTY(bb)) return APR_SUCCESS;". And to avoid the > > Yes, that should work (but leaves us to reprocess the whole thing next > time round). Is that better or worse? Or come to think of it, we can > both return APR_SUCCESS and set a flag.
Why set a flag at all - if asked to filter nothing what interesting state has changed since the last invocation? > > re-process issue just ap_remove_output_filter(f) if finding an EOS-only > > brigade? > > Do you recollect the discussion around when that patch went in? > I don't in full, but I had a nagging recollection of someone having > proposed a simpler solution but found it didn't work. Just enough > to persuade me to preserve the loop-over-buckets test. Nothing like that was posted to the list, at least. Patch below is still sufficient to fix the proxy+304 case; does it work for you too? --- mod_deflate.c 18 Jul 2004 01:18:58 -0000 1.56 +++ mod_deflate.c 19 Jul 2004 15:29:02 -0000 @@ -247,7 +247,6 @@ apr_bucket_brigade *bb, *proc_bb; } deflate_ctx; -static void* const deflate_yes = (void*)"YES"; static apr_status_t deflate_out_filter(ap_filter_t *f, apr_bucket_brigade *bb) { @@ -255,14 +254,14 @@ request_rec *r = f->r; deflate_ctx *ctx = f->ctx; int zRC; - char* buf; - int eos_only = 1; - apr_bucket *bkt; - char *token; - const char *encoding = NULL; deflate_filter_config *c = ap_get_module_config(r->server->module_config, &deflate_module); + /* Do nothing if asked to filter nothing. */ + if (APR_BRIGADE_EMPTY(bb)) { + return APR_SUCCESS; + } + /* If we don't have a context, we need to ensure that it is okay to send * the deflated content. If we have a context, that means we've done * this before and we liked it. @@ -270,6 +269,8 @@ * we're in better shape. */ if (!ctx) { + char *buf, *token; + const char *encoding; /* only work on main request/no subrequests */ if (r->main) { @@ -349,7 +350,13 @@ */ apr_table_setn(r->headers_out, "Vary", "Accept-Encoding"); - + /* Deflating a zero-length response would make it longer; the + * proxy may pass through an empty response for a 304 too. */ + if (APR_BUCKET_IS_EOS(APR_BRIGADE_FIRST(bb))) { + ap_remove_output_filter(f); + return ap_pass_brigade(f->next, bb); + } + /* force-gzip will just force it out regardless if the browser * can actually do anything with it. */ @@ -383,41 +390,6 @@ return ap_pass_brigade(f->next, bb); } } - - /* don't deflate responses with zero length e.g. proxied 304's but - * we do set the header on eos_only at this point for headers_filter - * - * if we get eos_only and come round again, we want to avoid redoing - * what we've already done, so set f->ctx to a flag here - */ - f->ctx = ctx = deflate_yes; - } - if (ctx == deflate_yes) { - /* deal with the pathological case of lots of empty brigades and - * no knowledge of whether content will follow - */ - for (bkt = APR_BRIGADE_FIRST(bb); - bkt != APR_BRIGADE_SENTINEL(bb); - bkt = APR_BUCKET_NEXT(bkt)) - { - if (!APR_BUCKET_IS_EOS(bkt)) { - eos_only = 0; - break; - } - } - if (eos_only) { - if (!encoding || !strcasecmp(encoding, "identity")) { - apr_table_set(r->headers_out, "Content-Encoding", "gzip"); - } - else { - apr_table_merge(r->headers_out, "Content-Encoding", "gzip"); - } - apr_table_unset(r->headers_out, "Content-Length"); - return ap_pass_brigade(f->next, bb); - } - } - if (!ctx || (ctx==deflate_yes)) { - /* We're cool with filtering this. */ ctx = f->ctx = apr_pcalloc(r->pool, sizeof(*ctx)); ctx->bb = apr_brigade_create(r->pool, f->c->bucket_alloc);