Re: cvs commit: httpd-2.0/modules/filters mod_deflate.c

[Nick Kew]

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

[Nick Kew]

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

[Joe Orton]

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

[Joe Orton]

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

[Nick Kew]

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

[Joe Orton]

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

[Andr Malo]

* [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

[Nick Kew]

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

[William A. Rowe, Jr.]

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

[Ian Holsman]

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

[André Malo]

* 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)

[Ian Holsman]

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

[Bill Stoddard]

 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

[Justin Erenkrantz]

--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

[Justin Erenkrantz]

--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

[André Malo]

* 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

[André Malo]

* 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

[Aaron Bannert]

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

[William A. Rowe, Jr.]

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

[Ian Holsman]

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)

[Greg Stein]

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

[Cliff Woolley]

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