Re: cvs commit: httpd-2.0/modules/filters mod_deflate.c
On Mon, 19 Jul 2004, Joe Orton wrote: 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? Yes, mostly (it fixes the important bug that was previously a showstopper). And it's an improvement on my hack by virtue of simplicity. But it should still set the Content-Encoding header on a HEAD request that would normally be deflated (and unset content-length if present). So your: +/* 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); +} + should move after the the if ( ! force-gzip ) block, and if then if we reach the EOS-only test we should fix up the headers. That test also seems to lose the pathological case of a brigade with no data but one or more FLUSH buckets followed by EOS. Could that ever happen in a HEAD or a 204/304? Investigating this has revealed a similar bug with HEAD requests in inflate_out_filter, which I shall now have to fix:-( -- Nick Kew
Re: cvs commit: httpd-2.0/modules/filters mod_deflate.c
On Thu, 22 Jul 2004, Nick Kew wrote: On Mon, 19 Jul 2004, Joe Orton wrote: 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? Yes, mostly (it fixes the important bug that was previously a showstopper). I attach a new patch based on yours. it fixes my testcases including headers for HEAD requests. Look OK to you? -- Nick Kew--- mod_deflate.c.bak 2004-07-22 11:12:53.0 +0100 +++ mod_deflate.c 2004-07-22 12:17:13.0 +0100 @@ -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,6 @@ */ apr_table_setn(r-headers_out, Vary, Accept-Encoding); - /* force-gzip will just force it out regardless if the browser * can actually do anything with it. */ @@ -384,39 +384,22 @@ } } -/* 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 +/* Deflating a zero-length response would make it longer; the + * proxy may pass through an empty response for a 304 too. + * So we just need to fix up the headers as if we had a body. */ -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)) { +if (APR_BUCKET_IS_EOS(APR_BRIGADE_FIRST(bb))) { + 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); + +ap_remove_output_filter(f); 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)); @@ -912,6 +895,11 @@ apr_status_t rv; deflate_filter_config *c; +/* Do nothing if asked to filter nothing. */ +if (APR_BRIGADE_EMPTY(bb)) { +return APR_SUCCESS; +} + c = ap_get_module_config(r-server-module_config, deflate_module); if (!ctx) { @@ -950,6 +938,13 @@ } apr_table_unset(r-headers_out, Content-Encoding); +/* No need to inflate HEAD or 204/304 */ +if (APR_BUCKET_IS_EOS(APR_BRIGADE_FIRST(bb))) { +ap_remove_output_filter(f); +return ap_pass_brigade(f-next, bb); +} + + f-ctx = ctx = apr_pcalloc(f-r-pool, sizeof(*ctx)); ctx-proc_bb = apr_brigade_create(r-pool, f-c-bucket_alloc); ctx-buffer = apr_palloc(r-pool, c-bufferSize); @@ -983,9 +978,10 @@ apr_size_t len; /* If we actually see the EOS, that means we screwed up! */ +/* no it doesn't - not in a HEAD or 204/304 */ if (APR_BUCKET_IS_EOS(bkt)) { inflateEnd(ctx-stream); -return APR_EGENERAL; +return ap_pass_brigade(f-next, bb); } if (APR_BUCKET_IS_FLUSH(bkt)) {
Re: cvs commit: httpd-2.0/modules/filters mod_deflate.c
On Thu, Jul 22, 2004 at 12:20:33PM +0100, Nick Kew wrote: On Thu, 22 Jul 2004, Nick Kew wrote: On Mon, 19 Jul 2004, Joe Orton wrote: 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? Yes, mostly (it fixes the important bug that was previously a showstopper). I attach a new patch based on yours. it fixes my testcases including headers for HEAD requests. Look OK to you? Yup, other than the tab character. +if (APR_BUCKET_IS_EOS(APR_BRIGADE_FIRST(bb))) { + if (!encoding || !strcasecmp(encoding, identity)) { ^^^
Re: cvs commit: httpd-2.0/modules/filters mod_deflate.c
On Sat, Jul 17, 2004 at 03:22:35PM -, [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 re-process issue just ap_remove_output_filter(f) if finding an EOS-only brigade? joe
Re: cvs commit: httpd-2.0/modules/filters mod_deflate.c
On Mon, 19 Jul 2004, Joe Orton wrote: On Sat, Jul 17, 2004 at 03:22:35PM -, [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. 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. -- Nick Kew
Re: cvs commit: httpd-2.0/modules/filters mod_deflate.c
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 -, [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 - 1.56 +++ mod_deflate.c 19 Jul 2004 15:29:02 - @@ -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);
Re: cvs commit: httpd-2.0/modules/filters mod_deflate.c
* [EMAIL PROTECTED] wrote: +f-ctx = ctx = (void*)-1; I personally consider defining arbitrary pointer values as bad style, though I'm not sure what the general opinion here is (if any). I'd suggest to use a static pointer, like a global static char foo_sentinel; /* choose a speaking name ;-) */ /* and later */ f-ctx = ctx = foo_sentinel; Additionally - afair - the use of arbirtrary pointer values can even lead to bus errors on not-so-usual systems (loading undefined bits into an address register...). nd -- die (eval q-qq[Just Another Perl Hacker ] ;-) # André Malo, http://www.perlig.de/ #
Re: cvs commit: httpd-2.0/modules/filters mod_deflate.c
On Sat, 17 Jul 2004, [ISO-8859-15] André Malo wrote: * [EMAIL PROTECTED] wrote: +f-ctx = ctx = (void*)-1; I personally consider defining arbitrary pointer values as bad style, though I'm not sure what the general opinion here is (if any). I'd suggest to use a static pointer, like a global static char foo_sentinel; /* choose a speaking name ;-) */ /* and later */ f-ctx = ctx = foo_sentinel; Additionally - afair - the use of arbirtrary pointer values can even lead to bus errors on not-so-usual systems (loading undefined bits into an address register...). Yes, you're right. Actually this patch has a deeper problem, as does the patch it fixes. Setting the headers at this point depends entirely on the behaviour of the headers filter. With current behaviour, the previous mod_deflate was broken (because it could delay setting headers until after the headers have been sent down the wire). With my patch it might still risk minor breakage (repeated gzip header) if the headers filter changes sometime in future. Any more issues with this? If not I'll make nd's fix and leave it. -- Nick Kew
Re: cvs commit: httpd-2.0/modules/filters mod_deflate.c
At 12:04 PM 3/11/2003, [EMAIL PROTECTED] wrote: ianh2003/03/11 10:04:37 Modified:.Tag: APACHE_2_0_BRANCH CHANGES STATUS docs/manual/mod Tag: APACHE_2_0_BRANCH mod_deflate.xml modules/filters Tag: APACHE_2_0_BRANCH mod_deflate.c Log: Backport from 2.1 tree. Only difference is that the name of the directive was changed to DeflateCompressionLevel Ahhh... we plan to change that directive name in the 2.1-dev tree as well? Bill
Re: cvs commit: httpd-2.0/modules/filters mod_deflate.c
William A. Rowe, Jr. wrote: At 12:04 PM 3/11/2003, [EMAIL PROTECTED] wrote: ianh2003/03/11 10:04:37 Modified:.Tag: APACHE_2_0_BRANCH CHANGES STATUS docs/manual/mod Tag: APACHE_2_0_BRANCH mod_deflate.xml modules/filters Tag: APACHE_2_0_BRANCH mod_deflate.c Log: Backport from 2.1 tree. Only difference is that the name of the directive was changed to DeflateCompressionLevel Ahhh... we plan to change that directive name in the 2.1-dev tree as well? yes.. r 1.33 I believe has this Bill
Re: cvs commit: httpd-2.0/modules/filters mod_deflate.c
* Ian Holsman wrote: [2.0.45] + *) mod_deflate: Extend the DeflateFilterNote directive to + allow accurate logging of the filter's in- and outstream. + [André Malo] ah, although it wasn't voted ... I'd give my late +1 on it. But please keep the changes consistent ;-) nd -- Da fällt mir ein, wieso gibt es eigentlich in Unicode kein i mit einem Herzchen als Tüpfelchen? Das wär sooo süüss! -- Björn Höhrmann in darw
branching -- was (Re: cvs commit: httpd-2.0/modules/filters mod_deflate.c)
ok.. can someone remind me what we are supposed to do in this situation? Although I dislike putting in 'unvoted' changes into the stable release, I hate having 2 seperate sets of code, with bits pieces of each other in them worse. André Malo wrote: * Ian Holsman wrote: [2.0.45] + *) mod_deflate: Extend the DeflateFilterNote directive to + allow accurate logging of the filter's in- and outstream. + [André Malo] ah, although it wasn't voted ... I'd give my late +1 on it. But please keep the changes consistent ;-) nd
RE: cvs commit: httpd-2.0/modules/filters mod_deflate.c
IMHO, there should be an extremely high bar to creating a macro. -- justin I fricking hate macros but there are times they are useful. This isn't one of them IMHO. Bill
Re: cvs commit: httpd-2.0/modules/filters mod_deflate.c
--On Wednesday, January 1, 2003 8:31 PM + [EMAIL PROTECTED] wrote: @@ -239,6 +256,11 @@ apr_bucket_brigade *bb, *proc_bb; } deflate_ctx; +#define LeaveNote(type, value) \ +if (c-note##type##Name) \ +apr_table_setn(r-notes, c-note##type##Name, \ + (ctx-stream.total_in 0) ? (value) : -) We don't use mixed case for any definitions, and we require braces at all times, and for macros longer than one line, we wrap it with a do {} while (0) clause so that some compilers are a bit happier. So, I would probably suggest something like: #define leave_note(type, value) \ do { \ if (c-note##type##Name) { \ apr_table_setn(r-notes, c-note##type##Name, \ (ctx-stream.total_in 0) ? (value) : -) \ } \ } while (0) +LeaveNote(Input, apr_off_t_toa(r-pool, ctx-stream.total_in)); +LeaveNote(Output, apr_off_t_toa(r-pool, ctx-stream.total_out)); +LeaveNote(Ratio, apr_itoa(r-pool, (int) + (ctx-stream.total_out * 100 / ctx-stream.total_in))); All of that said, I'm not really sure this code even merits being a macro. I'd rather see the conditional execution clear at the scope where LeaveNote is called rather than hidden in its definition. Yeah, it's slightly repetitive, but I'm not really buying what the macro is getting us here. IMHO, there should be an extremely high bar to creating a macro. -- justin
Re: cvs commit: httpd-2.0/modules/filters mod_deflate.c
--On Sunday, November 10, 2002 6:09 AM + [EMAIL PROTECTED] wrote: jerenkrantz2002/11/09 22:09:20 Modified:.CHANGES modules/filters mod_deflate.c Log: Always emit Vary header if mod_deflate is involved in the request. Submitted by: Andr?©Malo [EMAIL PROTECTED] Reviewed by: Justin Erenkrantz Oh, bah. I'm getting used to a certain SCM that supports UTF-8 (heck, I'm not even sure if that got inputed as UTF-8 either). My bad. André, how do you want your name reflected in the CHANGES file? (Sending a patch would probably be best so I don't screw it up anymore...) -- justin
Re: cvs commit: httpd-2.0/modules/filters mod_deflate.c
* Rasmus Lerdorf wrote: Shouldn't you also Vary on User-Agent when a BrowserMatch no-gzip is present? Yes, but mod_deflate doesn't know anything about BrowserMatch. So one has to configure an explicit Header append Vary User-Agent in that case (no-gzip can be set in various ways, not only with BrowserMatch). In fact, I was only extending the docs a little bit, see http://cvs.apache.org/~nd/manual/mod/mod_deflate.html ;-) nd -- Eine Eieruhr, erklärt ihr Hermann, besteht aus einem Ei. Du nimmst das Ei und kochst es. Wenn es hart ist, sind fünf Minuten um. Dann weißt du, daß die Zeit vergangen ist. -- Hannes Hüttner in Das Blaue vom Himmel
Re: cvs commit: httpd-2.0/modules/filters mod_deflate.c
* Justin Erenkrantz wrote: André, how do you want your name reflected in the CHANGES file? you may simply drop the eh... accent (?). (Sending a patch would probably be best so I don't screw it up anymore...) hehe, done. nd -- sub the($){+shift} sub answer (){ord q [* It is always 42! *] } print the answer # André Malo # http://www.perlig.de/ # Index: CHANGES === RCS file: /home/cvspublic/httpd-2.0/CHANGES,v retrieving revision 1.975 diff -u -r1.975 CHANGES --- CHANGES 10 Nov 2002 06:09:19 - 1.975 +++ CHANGES 10 Nov 2002 07:01:27 - @@ -1,7 +1,7 @@ Changes with Apache 2.0.44 *) Always emit Vary header if mod_deflate is involved in the - request. [AndréMalo [EMAIL PROTECTED]] + request. [André Malo [EMAIL PROTECTED]] *) mod_isapi: Stop unsetting the 'empty' query string result with a NULL argument in ecb-lpszQueryString, eliminating segfaults
Re: cvs commit: httpd-2.0/modules/filters mod_deflate.c
On Sat, Nov 09, 2002 at 10:17:58PM -0800, Justin Erenkrantz wrote: Oh, bah. I'm getting used to a certain SCM that supports UTF-8 (heck, I'm not even sure if that got inputed as UTF-8 either). My bad. Did you edit that file in your SCM? How did you input that anyway? What kind of encoding is valid for our CHANGES file (I'm really out of my league here, but I'd be happy if someone could point my sorry sheltered US-keyboard-centric self in the right direction here). André, how do you want your name reflected in the CHANGES file? (Sending a patch would probably be best so I don't screw it up anymore...) -- justin I just took out the accent, but it would be cool someone could tell me how to make it work with accents. :) -aaron
Re: cvs commit: httpd-2.0/modules/filters mod_deflate.c
At 08:06 PM 5/19/2002, Cliff wrote: Whoa, wait a minute. That doesn't strike me as the right solution. The encoding should be one-hop only. If it's encoded and we want to maintain that encoding, chances are we'll have to decode it and re-encode it later. Why you ask? Because leaving it encoded makes it impossible to apply another filter on the proxy server (eg mod_include). Now perhaps if we can guarantee that there are no other filters in the chain that will want to modify the content *and* that the client can actually accept the encoding, then as an optimization we can pass the data through the filter chain still encoded. But that would only be an optimization. And it seems like it could be tricky to get it to always work doing it that way, perhaps. (Is there ever a case where the client does not accept an encoding but the proxy does?) I'm not sure. It seems that inflating would be a proxy input side filter, much like dechunking. However, if no module needs to process the body, it would be a horrible waste to inflate+deflate the content. Why can't we insist on the admin inserting a mod_inflate filter on the proxy end if they want to rewrite proxied content [for only the content they want to touch?] Bill
Re: cvs commit: httpd-2.0/modules/filters mod_deflate.c
Cliff Woolley wrote: On 20 May 2002 [EMAIL PROTECTED] wrote: ianh02/05/19 17:07:33 Modified:modules/filters mod_deflate.c Log: content with Content-Encoding header, content is encoded. But mod_deflate does not check it. It cause to encode content twice. This problem is reproducable by getting encoded content via mod_proxy. Patch Contributed by [EMAIL PROTECTED] (ASADA Kazuhisa) Bug #9222 Whoa, wait a minute. That doesn't strike me as the right solution. The encoding should be one-hop only. If it's encoded and we want to maintain that encoding, chances are we'll have to decode it and re-encode it later. deflate runs at the end (I think it's a header/network type output filter) and the output of this is compressed data. we do check if the user can accept the gzip encoding, i assumed that if this has been content-encoded already that module has done all the checks necessary to make sure the client supports it. This should also fix the case where you have multiple content-encoders running on the same file.. the one with the highest priority (first in the chain) will do the encoding and the others will leave it alone. I think what your after is a mod-inflate which de-compresses the data Why you ask? Because leaving it encoded makes it impossible to apply another filter on the proxy server (eg mod_include). Now perhaps if we can guarantee that there are no other filters in the chain that will want to modify the content *and* that the client can actually accept the encoding, then as an optimization we can pass the data through the filter chain still encoded. But that would only be an optimization. And it seems like it could be tricky to get it to always work doing it that way, perhaps. (Is there ever a case where the client does not accept an encoding but the proxy does?) It shouldn't .. the proxy should be fixed if it does IMHO --Ian --Cliff -- Cliff Woolley [EMAIL PROTECTED] Charlottesville, VA
Content Codings (was: Re: cvs commit: httpd-2.0/modules/filters mod_deflate.c)
On Mon, May 20, 2002 at 08:40:03AM -0700, Ian Holsman wrote: Cliff Woolley wrote: On 20 May 2002 [EMAIL PROTECTED] wrote: ianh02/05/19 17:07:33 Modified:modules/filters mod_deflate.c Log: content with Content-Encoding header, content is encoded. But mod_deflate does not check it. It cause to encode content twice. This problem is reproducable by getting encoded content via mod_proxy. Patch Contributed by [EMAIL PROTECTED] (ASADA Kazuhisa) Bug #9222 This is the wrong fix. Take a look at RFC 2616. MULTIPLE encodings are allowed. If the Content-Encoding header said identity, then we'll skip the compression. Bad Bad Bad. The following is entirely legal: Content-Encoding: gzip, compress, deflate (dumb, but legal) [ see sections 14.11 and 3.5 of RFC 2616 for more info ] Whoa, wait a minute. That doesn't strike me as the right solution. The encoding should be one-hop only. If it's encoded and we want to maintain that encoding, chances are we'll have to decode it and re-encode it later. Depends on what we want to do with proxied requests. Do we run them through our own filter stack? (uncompressed) But also, it is entirely reasonable that we could build an inflator on the client-piece of our proxy to handle the case where Apache can speak compressed to the origin server, but we have to inflate it for our client. (right now, the proxy is just copying over what the client allows; it would be entirely reasonable to tell the origin server we can always accept deflated content; of course, that also means we need to write the inflate logic :-) deflate runs at the end (I think it's a header/network type output filter) and the output of this is compressed data. we do check if the user can accept the gzip encoding, i assumed that if this has been content-encoded already that module has done all the checks necessary to make sure the client supports it. It happens to work because the proxy client stands in for the true client. But that isn't always a good assumption. This should also fix the case where you have multiple content-encoders running on the same file.. the one with the highest priority (first in the chain) will do the encoding and the others will leave it alone. Absolutely wrong. Each of them can and should be able to apply themselves if they so choose. The correct test is something like this: if Content-Encoding is present: if Content-Encoding contains deflate then skip-self Essentially, we will apply a deflate encoding if it isn't already present. After doing the deflate, we then need to set the Content-Encoding using: if Content-Encoding is not present, or it equals identity: set Content-Encoding to deflate else append , deflate to Content-Encoding ... Why you ask? Because leaving it encoded makes it impossible to apply another filter on the proxy server (eg mod_include). Now perhaps if we can guarantee that there are no other filters in the chain that will want to modify the content *and* that the client can actually accept the encoding, then as an optimization we can pass the data through the filter chain still encoded. But that would only be an optimization. And it Tough optimization. Right now, we don't have these kinds of whole-chain introspection concepts. Really, what we'd want is some kind of flag for the filters to say I want uncompressed content. When the proxy wants to deliver compressed content, but it sees that filter in the stack, then it will decompress it. (or the filter code itself would examine the stack and insert the right translators between filters to ensure their inputs and ouputs are matched up with the right encodings, character sets, etc) seems like it could be tricky to get it to always work doing it that way, perhaps. (Is there ever a case where the client does not accept an encoding but the proxy does?) We could certainly implement the proxy that way. Cheers, -g -- Greg Stein, http://www.lyra.org/
Re: cvs commit: httpd-2.0/modules/filters mod_deflate.c
On 20 May 2002 [EMAIL PROTECTED] wrote: ianh02/05/19 17:07:33 Modified:modules/filters mod_deflate.c Log: content with Content-Encoding header, content is encoded. But mod_deflate does not check it. It cause to encode content twice. This problem is reproducable by getting encoded content via mod_proxy. Patch Contributed by [EMAIL PROTECTED] (ASADA Kazuhisa) Bug #9222 Whoa, wait a minute. That doesn't strike me as the right solution. The encoding should be one-hop only. If it's encoded and we want to maintain that encoding, chances are we'll have to decode it and re-encode it later. Why you ask? Because leaving it encoded makes it impossible to apply another filter on the proxy server (eg mod_include). Now perhaps if we can guarantee that there are no other filters in the chain that will want to modify the content *and* that the client can actually accept the encoding, then as an optimization we can pass the data through the filter chain still encoded. But that would only be an optimization. And it seems like it could be tricky to get it to always work doing it that way, perhaps. (Is there ever a case where the client does not accept an encoding but the proxy does?) --Cliff -- Cliff Woolley [EMAIL PROTECTED] Charlottesville, VA