Re: [PATCH] mod_deflate: remove filter after seeing EOS

[Eric Norris via dev]

On Wed, Dec 20, 2023 at 10:58 AM Joe Orton  wrote:
>
> On Wed, Dec 20, 2023 at 04:24:32PM +0100, Ruediger Pluem wrote:
> > On 12/20/23 4:08 PM, Yann Ylavic wrote:
> > > On Wed, Dec 20, 2023 at 2:40 PM Joe Orton  wrote:
> > >> https://github.com/apache/httpd/pull/400
> > >
> > > Thanks, looks good to me.
> >
> > +1
>
> Thanks a lot for the quick reviews. Merged in r1914804 and happy Xmas :)

Likewise, thanks for taking a look everyone, and enjoy the holidays!

Re: [PATCH] mod_deflate: remove filter after seeing EOS

[Yann Ylavic]

On Wed, Dec 20, 2023 at 4:57 PM Joe Orton  wrote:
>
> On Wed, Dec 20, 2023 at 04:24:32PM +0100, Ruediger Pluem wrote:
> > On 12/20/23 4:08 PM, Yann Ylavic wrote:
> > > On Wed, Dec 20, 2023 at 2:40 PM Joe Orton  wrote:
> > >> https://github.com/apache/httpd/pull/400
> > >
> > > Thanks, looks good to me.
> >
> > +1
>
> Thanks a lot for the quick reviews. Merged in r1914804 and happy Xmas :)

Happy Holidays! ;)

Re: [PATCH] mod_deflate: remove filter after seeing EOS

[Eric Norris via dev]

On Wed, Dec 20, 2023 at 10:43 AM Joe Orton  wrote:
>
> In the repro case you posted, only one brigade is passed by the handler,
> with that I saw the "delayed last chunk" behaviour but not the Zlib
> double-deinit error log. I think the Zlib error would only be triggered
> by passing a second brigade with a FLUSH.

Ah, now that you mention it, that sounds right. That might have been
another piece of context I had lost between writing the patches and
sending them :)

> > If I understand correctly, either patch would send the flush bucket after
> > the last chunk marker and before the EOS bucket (i.e. [crlf last-chunk
> > FLUSH EOS]), so there's no need for "userspace" to send an additional flush
> > after the EOS bucket, and so mod_deflate wouldn't complain. Does that sound
> > right?
>
> I don't think the mod_deflate patch fixed the "delayed last-chunk"
> problem in my testing, but it is definitely correct & necessary to fix
> the Zlib error as above.

Right, I wasn't expecting the mod_deflate patch to fix the "delayed
last-chunk" issue - by "either patch", I meant either your proposed
chunk filter patch or my not-submitted-and-only-in-my-local-fork chunk
filter patch. My understanding now is that my patch to mod_deflate
makes it more robust, but the real underlying issue is the "delayed
last-chunk" problem, which you're fixing.

Re: [PATCH] mod_deflate: remove filter after seeing EOS

[Yann Ylavic]

On Wed, Dec 20, 2023 at 4:45 PM Yann Ylavic  wrote:
>
> On Wed, Dec 20, 2023 at 4:18 PM Eric Norris  wrote:
> >
> > On Wed, Dec 20, 2023 at 10:09 AM Yann Ylavic  wrote:
> > >
> > > So I think what the POC or mod_php should be doing is [FLUSH EOS] or
> > > something might not work in the chain sooner or later?
> >
> > I believe that is what the POC was doing here
> > https://gist.github.com/ericnorris/37c7dac401b50d270867e82a47bdd294#file-mod_example_hooks-c-L58-L64
> > - unfortunately though, the chunk filter behavior requires that we
> > send another FLUSH bucket
> > (https://gist.github.com/ericnorris/37c7dac401b50d270867e82a47bdd294#file-mod_example_hooks-c-L67-L68)
> > in order to get the last chunk marker to go immediately to the client.
>
> Ah ok I see now (didn't look at the PR before where the POC is
> linked), besides the second FLUSH it seems to be doing things
> correctly.
>
> > Without this, the last chunk marker sits in the bucket brigade until
> > the POC finishes and something in the Apache internals sends it out. I
> > agree though that sending a flush after an EOS is strange, and I noted
> > in my response to Joe that maybe the chunk filter change alone is
> > enough to solve our problem.
>
> I think it's fixed by Joe's PR/patch, the last-chunk is now inserted
> before the FLUSH (previously it was before EOS even if a FLUSH was
> there too).
>
> But Joe's patch won't make it before the next release at best, until
> then you could either:
> 1. [FLUSH][EOS] (two passes on r->output_filters)
> 2. [EOS][FLUSH] (first EOS passed on r->output_filters, second FLUSH
> passed on r->connection->output_filters).
> Not pretty but should work..

You might want to consider "FlushMaxPipelined 0" in the VirtualHost if
you want every response to be flushed too..

>
> Regards;
> Yann.

Re: [PATCH] mod_deflate: remove filter after seeing EOS

[Joe Orton]

On Wed, Dec 20, 2023 at 04:24:32PM +0100, Ruediger Pluem wrote:
> On 12/20/23 4:08 PM, Yann Ylavic wrote:
> > On Wed, Dec 20, 2023 at 2:40 PM Joe Orton  wrote:
> >> https://github.com/apache/httpd/pull/400
> > 
> > Thanks, looks good to me.
> 
> +1

Thanks a lot for the quick reviews. Merged in r1914804 and happy Xmas :)

Regards, Joe

Re: [PATCH] mod_deflate: remove filter after seeing EOS

[Yann Ylavic]

On Wed, Dec 20, 2023 at 4:18 PM Eric Norris  wrote:
>
> On Wed, Dec 20, 2023 at 10:09 AM Yann Ylavic  wrote:
> >
> > So I think what the POC or mod_php should be doing is [FLUSH EOS] or
> > something might not work in the chain sooner or later?
>
> I believe that is what the POC was doing here
> https://gist.github.com/ericnorris/37c7dac401b50d270867e82a47bdd294#file-mod_example_hooks-c-L58-L64
> - unfortunately though, the chunk filter behavior requires that we
> send another FLUSH bucket
> (https://gist.github.com/ericnorris/37c7dac401b50d270867e82a47bdd294#file-mod_example_hooks-c-L67-L68)
> in order to get the last chunk marker to go immediately to the client.

Ah ok I see now (didn't look at the PR before where the POC is
linked), besides the second FLUSH it seems to be doing things
correctly.

> Without this, the last chunk marker sits in the bucket brigade until
> the POC finishes and something in the Apache internals sends it out. I
> agree though that sending a flush after an EOS is strange, and I noted
> in my response to Joe that maybe the chunk filter change alone is
> enough to solve our problem.

I think it's fixed by Joe's PR/patch, the last-chunk is now inserted
before the FLUSH (previously it was before EOS even if a FLUSH was
there too).

But Joe's patch won't make it before the next release at best, until
then you could either:
1. [FLUSH][EOS] (two passes on r->output_filters)
2. [EOS][FLUSH] (first EOS passed on r->output_filters, second FLUSH
passed on r->connection->output_filters).
Not pretty but should work..

Regards;
Yann.

Re: [PATCH] mod_deflate: remove filter after seeing EOS

[Joe Orton]

On Wed, Dec 20, 2023 at 10:07:19AM -0500, Eric Norris via dev wrote:
> Thanks Joe, and no need to apologize, that's totally understandable.
> 
> I also appreciate you taking a look at the chunk filter behavior as that
> was actually going to be the next patch I proposed. I had written it here:
> https://github.com/ericnorris/httpd/commit/5f8fa24786b937ab611160b3c765cededa6dcb12,
> though in retrospect I'm not sure why this (or your patch) alone isn't
> enough, and why I thought the mod_deflate patch was also needed.

In the repro case you posted, only one brigade is passed by the handler, 
with that I saw the "delayed last chunk" behaviour but not the Zlib 
double-deinit error log. I think the Zlib error would only be triggered 
by passing a second brigade with a FLUSH.

It is incorrect for a handler to pass anything after EOS but filters are 
also supposed to be robust against it anyway.

> If I understand correctly, either patch would send the flush bucket after
> the last chunk marker and before the EOS bucket (i.e. [crlf last-chunk
> FLUSH EOS]), so there's no need for "userspace" to send an additional flush
> after the EOS bucket, and so mod_deflate wouldn't complain. Does that sound
> right?

I don't think the mod_deflate patch fixed the "delayed last-chunk" 
problem in my testing, but it is definitely correct & necessary to fix 
the Zlib error as above.

> Apologies on my part if that's the case - it had been a few months since I
> had written the patches, so I might have lost that context by the time I
> was able to investigate submitting the patches. I possibly should have
> submitted the chunk filter patch first. Either way, like I said I
> appreciate your time.

Thanks for taking the time to investigate & report it!

Regards, Joe

Re: [PATCH] mod_deflate: remove filter after seeing EOS

[Ruediger Pluem]

On 12/20/23 4:08 PM, Yann Ylavic wrote:
> On Wed, Dec 20, 2023 at 2:40 PM Joe Orton  wrote:
>>
>> I was surprised this made a difference to the behaviour on the wire. It
>> seems like the chunk filter has suboptimal behaviour here. If you take
>> an output brigade like either:
>>
>> a) [HEAP FLUSH EOS]
>> b) [HEAP FLUSH EOS FLUSH]
>>
>> in both cases the chunk filter would output two brigades:
>>
>> [chunk-hdr HEAP crlf FLUSH] [last-chunk EOS]
>>
>> Significantly there is no FLUSH in the second brigade even for case (b),
>> because the chunk filter also drops everything after EOS. It would be
>> clearly better/correct if the chunk filter produces a single brigade for
>> this very common output pattern:
>>
>> [chunk-hdr HEAP crlf last-chunk FLUSH EOS]
>>
>> correctly preserving the semantics of the FLUSH. I've tried this here:
>>
>> https://github.com/apache/httpd/pull/400
> 
> Thanks, looks good to me.

+1

> 
>>>
>>> On Mon, Oct 9, 2023 at 2:50 PM Eric Norris  wrote:

 At Etsy, we use mod_php and were investigating what we thought was
 surprising behavior - that code executed during PHP's shutdown phase
 prevented the request from terminating, even if we didn't expect to send
 any additional output to the client.

 We determined that this was not surprising given mod_php's
 implementation, but after we developed a proof-of-concept patch that
 sent an EOS bucket and a flush bucket via a "userland" PHP function, we
 were surprised that it didn't work when compression was enabled for the
 request.
> 
> I'm wondering if these cases are valid/supported though:
> c) [HEAP EOS FLUSH]
> d) [HEAP EOS] [FLUSH] (with separate FLUSH but on r->output_filters still)
> which seems to be what mod_php and the "userland" POC do?
> 
> I thought nothing should be sent on r->output_filters after EOS (only
> c->output_filters might forward metadata in between requests), and at
> least in ap_http_chunk_filter() this won't work since, as Joe said,
> everything after EOS being dropped breaks c) and the filter not
> removing itself after EOS breaks d).
> 
> So I think what the POC or mod_php should be doing is [FLUSH EOS] or
> something might not work in the chain sooner or later?

+1

Regards

Rüdiger

Re: [PATCH] mod_deflate: remove filter after seeing EOS

[Eric Norris via dev]

On Wed, Dec 20, 2023 at 10:09 AM Yann Ylavic  wrote:
>
> On Wed, Dec 20, 2023 at 2:40 PM Joe Orton  wrote:
> >
> > I was surprised this made a difference to the behaviour on the wire. It
> > seems like the chunk filter has suboptimal behaviour here. If you take
> > an output brigade like either:
> >
> > a) [HEAP FLUSH EOS]
> > b) [HEAP FLUSH EOS FLUSH]
> >
> > in both cases the chunk filter would output two brigades:
> >
> > [chunk-hdr HEAP crlf FLUSH] [last-chunk EOS]
> >
> > Significantly there is no FLUSH in the second brigade even for case (b),
> > because the chunk filter also drops everything after EOS. It would be
> > clearly better/correct if the chunk filter produces a single brigade for
> > this very common output pattern:
> >
> > [chunk-hdr HEAP crlf last-chunk FLUSH EOS]
> >
> > correctly preserving the semantics of the FLUSH. I've tried this here:
> >
> > https://github.com/apache/httpd/pull/400
>
> Thanks, looks good to me.
>
> > >
> > > On Mon, Oct 9, 2023 at 2:50 PM Eric Norris  wrote:
> > > >
> > > > At Etsy, we use mod_php and were investigating what we thought was
> > > > surprising behavior - that code executed during PHP's shutdown phase
> > > > prevented the request from terminating, even if we didn't expect to send
> > > > any additional output to the client.
> > > >
> > > > We determined that this was not surprising given mod_php's
> > > > implementation, but after we developed a proof-of-concept patch that
> > > > sent an EOS bucket and a flush bucket via a "userland" PHP function, we
> > > > were surprised that it didn't work when compression was enabled for the
> > > > request.
>
> I'm wondering if these cases are valid/supported though:
> c) [HEAP EOS FLUSH]
> d) [HEAP EOS] [FLUSH] (with separate FLUSH but on r->output_filters still)
> which seems to be what mod_php and the "userland" POC do?

Small note, mod_php doesn't yet do this, but it may be something that
we consider proposing. For now, it's a private patch that we're
running at Etsy.

> I thought nothing should be sent on r->output_filters after EOS (only
> c->output_filters might forward metadata in between requests), and at
> least in ap_http_chunk_filter() this won't work since, as Joe said,
> everything after EOS being dropped breaks c) and the filter not
> removing itself after EOS breaks d).
>
> So I think what the POC or mod_php should be doing is [FLUSH EOS] or
> something might not work in the chain sooner or later?

I believe that is what the POC was doing here
https://gist.github.com/ericnorris/37c7dac401b50d270867e82a47bdd294#file-mod_example_hooks-c-L58-L64
- unfortunately though, the chunk filter behavior requires that we
send another FLUSH bucket
(https://gist.github.com/ericnorris/37c7dac401b50d270867e82a47bdd294#file-mod_example_hooks-c-L67-L68)
in order to get the last chunk marker to go immediately to the client.
Without this, the last chunk marker sits in the bucket brigade until
the POC finishes and something in the Apache internals sends it out. I
agree though that sending a flush after an EOS is strange, and I noted
in my response to Joe that maybe the chunk filter change alone is
enough to solve our problem.

>
>
> Regards;
> Yann.

Re: [PATCH] mod_deflate: remove filter after seeing EOS

[Yann Ylavic]

On Wed, Dec 20, 2023 at 2:40 PM Joe Orton  wrote:
>
> I was surprised this made a difference to the behaviour on the wire. It
> seems like the chunk filter has suboptimal behaviour here. If you take
> an output brigade like either:
>
> a) [HEAP FLUSH EOS]
> b) [HEAP FLUSH EOS FLUSH]
>
> in both cases the chunk filter would output two brigades:
>
> [chunk-hdr HEAP crlf FLUSH] [last-chunk EOS]
>
> Significantly there is no FLUSH in the second brigade even for case (b),
> because the chunk filter also drops everything after EOS. It would be
> clearly better/correct if the chunk filter produces a single brigade for
> this very common output pattern:
>
> [chunk-hdr HEAP crlf last-chunk FLUSH EOS]
>
> correctly preserving the semantics of the FLUSH. I've tried this here:
>
> https://github.com/apache/httpd/pull/400

Thanks, looks good to me.

> >
> > On Mon, Oct 9, 2023 at 2:50 PM Eric Norris  wrote:
> > >
> > > At Etsy, we use mod_php and were investigating what we thought was
> > > surprising behavior - that code executed during PHP's shutdown phase
> > > prevented the request from terminating, even if we didn't expect to send
> > > any additional output to the client.
> > >
> > > We determined that this was not surprising given mod_php's
> > > implementation, but after we developed a proof-of-concept patch that
> > > sent an EOS bucket and a flush bucket via a "userland" PHP function, we
> > > were surprised that it didn't work when compression was enabled for the
> > > request.

I'm wondering if these cases are valid/supported though:
c) [HEAP EOS FLUSH]
d) [HEAP EOS] [FLUSH] (with separate FLUSH but on r->output_filters still)
which seems to be what mod_php and the "userland" POC do?

I thought nothing should be sent on r->output_filters after EOS (only
c->output_filters might forward metadata in between requests), and at
least in ap_http_chunk_filter() this won't work since, as Joe said,
everything after EOS being dropped breaks c) and the filter not
removing itself after EOS breaks d).

So I think what the POC or mod_php should be doing is [FLUSH EOS] or
something might not work in the chain sooner or later?


Regards;
Yann.

Re: [PATCH] mod_deflate: remove filter after seeing EOS

[Eric Norris via dev]

Thanks Joe, and no need to apologize, that's totally understandable.

I also appreciate you taking a look at the chunk filter behavior as that
was actually going to be the next patch I proposed. I had written it here:
https://github.com/ericnorris/httpd/commit/5f8fa24786b937ab611160b3c765cededa6dcb12,
though in retrospect I'm not sure why this (or your patch) alone isn't
enough, and why I thought the mod_deflate patch was also needed.

If I understand correctly, either patch would send the flush bucket after
the last chunk marker and before the EOS bucket (i.e. [crlf last-chunk
FLUSH EOS]), so there's no need for "userspace" to send an additional flush
after the EOS bucket, and so mod_deflate wouldn't complain. Does that sound
right?

Apologies on my part if that's the case - it had been a few months since I
had written the patches, so I might have lost that context by the time I
was able to investigate submitting the patches. I possibly should have
submitted the chunk filter patch first. Either way, like I said I
appreciate your time.

Eric Norris
Etsy 


On Wed, Dec 20, 2023 at 8:37 AM Joe Orton  wrote:

> On Mon, Oct 30, 2023 at 10:47:44AM -0400, Eric Norris via dev wrote:
> > Hello again,
> >
> > I'd like to politely bump this message to see if anyone would mind
> > taking a look at this patch, either here or on GitHub.
>
> Apologies, I got quite distracted by the "rapid reset" security stuff
> earlier in the year. I've merged your patch - thanks!
>
> I was surprised this made a difference to the behaviour on the wire. It
> seems like the chunk filter has suboptimal behaviour here. If you take
> an output brigade like either:
>
> a) [HEAP FLUSH EOS]
> b) [HEAP FLUSH EOS FLUSH]
>
> in both cases the chunk filter would output two brigades:
>
> [chunk-hdr HEAP crlf FLUSH] [last-chunk EOS]
>
> Significantly there is no FLUSH in the second brigade even for case (b),
> because the chunk filter also drops everything after EOS. It would be
> clearly better/correct if the chunk filter produces a single brigade for
> this very common output pattern:
>
> [chunk-hdr HEAP crlf last-chunk FLUSH EOS]
>
> correctly preserving the semantics of the FLUSH. I've tried this here:
>
> https://github.com/apache/httpd/pull/400
>
> Regards, Joe
>
> >
> > Eric Norris
> > Etsy
> >
> > On Mon, Oct 9, 2023 at 2:50 PM Eric Norris  wrote:
> > >
> > > Hello all,
> > >
> > > I've submitted a pull request on GitHub here:
> > > https://github.com/apache/httpd/pull/387, per the instructions I found
> > > at https://httpd.apache.org/contribute. I figured it may also be
> > > useful to share the patch here, but please feel free to redirect me as
> > > this is my first time contributing.
> > >
> > > Thanks!
> > >
> > > Eric Norris
> > > Etsy
> > >
> > >
> > > At Etsy, we use mod_php and were investigating what we thought was
> > > surprising behavior - that code executed during PHP's shutdown phase
> > > prevented the request from terminating, even if we didn't expect to
> send
> > > any additional output to the client.
> > >
> > > We determined that this was not surprising given mod_php's
> > > implementation, but after we developed a proof-of-concept patch that
> > > sent an EOS bucket and a flush bucket via a "userland" PHP function, we
> > > were surprised that it didn't work when compression was enabled for the
> > > request.
> > >
> > > I was able to reproduce this behavior with a simple Apache module built
> > > on top of mod_example_hooks:
> > >
> > > https://gist.github.com/ericnorris/37c7dac401b50d270867e82a47bdd294
> > >
> > > It seems that this is because mod_deflate receives the EOS bucket and
> > > then calls `deflateEnd` but remains in the filter chain. This means
> that
> > > it receives the next flush bucket and attempts to call
> > > `flush_libz_buffer`, which of course fails, causing it to print an
> > > AH01385 error to the Apache error log, and perhaps most importantly, to
> > > "eat" the flush bucket and not pass it down the bucket brigade.
> > >
> > > We found that this patch allows us to successfully send an EOS bucket
> > > *and* promptly flush the connection so that the client knows the
> request
> > > is finished.
> > > ---
> > >  modules/filters/mod_deflate.c | 4 
> > >  1 file changed, 4 insertions(+)
> > >
> > > diff --git a/modules/filters/mod_deflate.c
> b/modules/filters/mod_deflate.c
> > > index b7a68d2d43..ad753dc618 100644
> > > --- a/modules/filters/mod_deflate.c
> > > +++ b/modules/filters/mod_deflate.c
> > > @@ -941,6 +941,10 @@ static apr_status_t
> deflate_out_filter(ap_filter_t *f,
> > >  }
> > >
> > >  deflateEnd(&ctx->stream);
> > > +
> > > +/* We've ended the libz stream, so remove ourselves. */
> > > +ap_remove_output_filter(f);
> > > +
> > >  /* No need for cleanup any longer */
> > >  apr_pool_cleanup_kill(r->pool, ctx, deflate_ctx_cleanup);
> > >
> > > --
> > > 2.40.1
> >
>
>

Re: [PATCH] mod_deflate: remove filter after seeing EOS

[Joe Orton]

On Mon, Oct 30, 2023 at 10:47:44AM -0400, Eric Norris via dev wrote:
> Hello again,
> 
> I'd like to politely bump this message to see if anyone would mind
> taking a look at this patch, either here or on GitHub.

Apologies, I got quite distracted by the "rapid reset" security stuff 
earlier in the year. I've merged your patch - thanks!

I was surprised this made a difference to the behaviour on the wire. It 
seems like the chunk filter has suboptimal behaviour here. If you take 
an output brigade like either:

a) [HEAP FLUSH EOS]
b) [HEAP FLUSH EOS FLUSH]

in both cases the chunk filter would output two brigades:

[chunk-hdr HEAP crlf FLUSH] [last-chunk EOS]

Significantly there is no FLUSH in the second brigade even for case (b), 
because the chunk filter also drops everything after EOS. It would be 
clearly better/correct if the chunk filter produces a single brigade for 
this very common output pattern:

[chunk-hdr HEAP crlf last-chunk FLUSH EOS]

correctly preserving the semantics of the FLUSH. I've tried this here:

https://github.com/apache/httpd/pull/400

Regards, Joe

> 
> Eric Norris
> Etsy
> 
> On Mon, Oct 9, 2023 at 2:50 PM Eric Norris  wrote:
> >
> > Hello all,
> >
> > I've submitted a pull request on GitHub here:
> > https://github.com/apache/httpd/pull/387, per the instructions I found
> > at https://httpd.apache.org/contribute. I figured it may also be
> > useful to share the patch here, but please feel free to redirect me as
> > this is my first time contributing.
> >
> > Thanks!
> >
> > Eric Norris
> > Etsy
> >
> >
> > At Etsy, we use mod_php and were investigating what we thought was
> > surprising behavior - that code executed during PHP's shutdown phase
> > prevented the request from terminating, even if we didn't expect to send
> > any additional output to the client.
> >
> > We determined that this was not surprising given mod_php's
> > implementation, but after we developed a proof-of-concept patch that
> > sent an EOS bucket and a flush bucket via a "userland" PHP function, we
> > were surprised that it didn't work when compression was enabled for the
> > request.
> >
> > I was able to reproduce this behavior with a simple Apache module built
> > on top of mod_example_hooks:
> >
> > https://gist.github.com/ericnorris/37c7dac401b50d270867e82a47bdd294
> >
> > It seems that this is because mod_deflate receives the EOS bucket and
> > then calls `deflateEnd` but remains in the filter chain. This means that
> > it receives the next flush bucket and attempts to call
> > `flush_libz_buffer`, which of course fails, causing it to print an
> > AH01385 error to the Apache error log, and perhaps most importantly, to
> > "eat" the flush bucket and not pass it down the bucket brigade.
> >
> > We found that this patch allows us to successfully send an EOS bucket
> > *and* promptly flush the connection so that the client knows the request
> > is finished.
> > ---
> >  modules/filters/mod_deflate.c | 4 
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/modules/filters/mod_deflate.c b/modules/filters/mod_deflate.c
> > index b7a68d2d43..ad753dc618 100644
> > --- a/modules/filters/mod_deflate.c
> > +++ b/modules/filters/mod_deflate.c
> > @@ -941,6 +941,10 @@ static apr_status_t deflate_out_filter(ap_filter_t *f,
> >  }
> >
> >  deflateEnd(&ctx->stream);
> > +
> > +/* We've ended the libz stream, so remove ourselves. */
> > +ap_remove_output_filter(f);
> > +
> >  /* No need for cleanup any longer */
> >  apr_pool_cleanup_kill(r->pool, ctx, deflate_ctx_cleanup);
> >
> > --
> > 2.40.1
> 

Re: [PATCH] mod_deflate: remove filter after seeing EOS

[Eric Norris via dev]

Hello again,

I'd like to politely bump this message to see if anyone would mind
taking a look at this patch, either here or on GitHub.

Eric Norris
Etsy

On Mon, Oct 9, 2023 at 2:50 PM Eric Norris  wrote:
>
> Hello all,
>
> I've submitted a pull request on GitHub here:
> https://github.com/apache/httpd/pull/387, per the instructions I found
> at https://httpd.apache.org/contribute. I figured it may also be
> useful to share the patch here, but please feel free to redirect me as
> this is my first time contributing.
>
> Thanks!
>
> Eric Norris
> Etsy
>
>
> At Etsy, we use mod_php and were investigating what we thought was
> surprising behavior - that code executed during PHP's shutdown phase
> prevented the request from terminating, even if we didn't expect to send
> any additional output to the client.
>
> We determined that this was not surprising given mod_php's
> implementation, but after we developed a proof-of-concept patch that
> sent an EOS bucket and a flush bucket via a "userland" PHP function, we
> were surprised that it didn't work when compression was enabled for the
> request.
>
> I was able to reproduce this behavior with a simple Apache module built
> on top of mod_example_hooks:
>
> https://gist.github.com/ericnorris/37c7dac401b50d270867e82a47bdd294
>
> It seems that this is because mod_deflate receives the EOS bucket and
> then calls `deflateEnd` but remains in the filter chain. This means that
> it receives the next flush bucket and attempts to call
> `flush_libz_buffer`, which of course fails, causing it to print an
> AH01385 error to the Apache error log, and perhaps most importantly, to
> "eat" the flush bucket and not pass it down the bucket brigade.
>
> We found that this patch allows us to successfully send an EOS bucket
> *and* promptly flush the connection so that the client knows the request
> is finished.
> ---
>  modules/filters/mod_deflate.c | 4 
>  1 file changed, 4 insertions(+)
>
> diff --git a/modules/filters/mod_deflate.c b/modules/filters/mod_deflate.c
> index b7a68d2d43..ad753dc618 100644
> --- a/modules/filters/mod_deflate.c
> +++ b/modules/filters/mod_deflate.c
> @@ -941,6 +941,10 @@ static apr_status_t deflate_out_filter(ap_filter_t *f,
>  }
>
>  deflateEnd(&ctx->stream);
> +
> +/* We've ended the libz stream, so remove ourselves. */
> +ap_remove_output_filter(f);
> +
>  /* No need for cleanup any longer */
>  apr_pool_cleanup_kill(r->pool, ctx, deflate_ctx_cleanup);
>
> --
> 2.40.1

Re: [PATCH] mod_deflate: hardcoded "%ld" -> APR_OFF_T_FMT

[Stefan Sperling]

On Sun, Dec 23, 2018 at 02:32:30PM +0100, Yann Ylavic wrote:
> Thanks Stefan, I didn't notice before in your proposed patch, but it
> looks like uint64_t casts should be apr_uint64_t too.
> 
> Regards,
> Yann.

Right. I went ahead and fixed it in r1849630.

Thanks,
Stefan

Re: [PATCH] mod_deflate: hardcoded "%ld" -> APR_OFF_T_FMT

[Yann Ylavic]

On Sun, Dec 23, 2018 at 10:33 AM Stefan Sperling  wrote:
>
> On Wed, Dec 19, 2018 at 07:03:39PM +0100, Stefan Sperling wrote:
> > On Wed, Dec 19, 2018 at 02:58:28PM +0100, Yann Ylavic wrote:
> > > On Wed, Dec 19, 2018 at 9:53 AM Stefan Sperling  wrote:
> > > >
> > > > On Tue, Dec 18, 2018 at 12:29:18AM +0100, Yann Ylavic wrote:
> > > > > But yes, upcast is better, while at it I'd go for uint64_t...
> > > >
> > > > Like this?
> > >
> > > I think APR_UINT64_T_FMT/apr_uint64_t would be more portable ;)
> > >
> > > Thanks for taking care of this !
> >
> > Sure :)
>
> Committed.

Thanks Stefan, I didn't notice before in your proposed patch, but it
looks like uint64_t casts should be apr_uint64_t too.

Regards,
Yann.

Re: [PATCH] mod_deflate: hardcoded "%ld" -> APR_OFF_T_FMT

[Stefan Sperling]

On Wed, Dec 19, 2018 at 07:03:39PM +0100, Stefan Sperling wrote:
> On Wed, Dec 19, 2018 at 02:58:28PM +0100, Yann Ylavic wrote:
> > On Wed, Dec 19, 2018 at 9:53 AM Stefan Sperling  wrote:
> > >
> > > On Tue, Dec 18, 2018 at 12:29:18AM +0100, Yann Ylavic wrote:
> > > > But yes, upcast is better, while at it I'd go for uint64_t...
> > >
> > > Like this?
> > 
> > I think APR_UINT64_T_FMT/apr_uint64_t would be more portable ;)
> > 
> > Thanks for taking care of this !
> 
> Sure :)

Committed.
 
> Index: modules/filters/mod_deflate.c
> ===
> --- modules/filters/mod_deflate.c (revision 1849274)
> +++ modules/filters/mod_deflate.c (working copy)
> @@ -893,8 +893,10 @@ static apr_status_t deflate_out_filter(ap_filter_t
> f->c->bucket_alloc);
>  APR_BRIGADE_INSERT_TAIL(ctx->bb, b);
>  ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(01384)
> -  "Zlib: Compressed %ld to %ld : URL %s",
> -  ctx->stream.total_in, ctx->stream.total_out, 
> r->uri);
> +  "Zlib: Compressed %" APR_UINT64_T_FMT
> +  " to %" APR_UINT64_T_FMT " : URL %s",
> +  (uint64_t)ctx->stream.total_in,
> +  (uint64_t)ctx->stream.total_out, r->uri);
>  
>  /* leave notes for logging */
>  if (c->note_input_name) {
> @@ -1438,9 +1440,10 @@ static apr_status_t deflate_in_filter(ap_filter_t
>  ctx->validation_buffer_length += valid;
>  
>  ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(01393)
> -  "Zlib: Inflated %ld to %ld : URL %s",
> -  ctx->stream.total_in, ctx->stream.total_out,
> -  r->uri);
> +  "Zlib: Inflated %" APR_UINT64_T_FMT
> +  " to %" APR_UINT64_T_FMT " : URL %s",
> +  (uint64_t)ctx->stream.total_in,
> +  (uint64_t)ctx->stream.total_out, r->uri);
>  
>  consume_buffer(ctx, c, c->bufferSize - ctx->stream.avail_out,
> UPDATE_CRC, ctx->proc_bb);
> @@ -1459,9 +1462,10 @@ static apr_status_t deflate_in_filter(ap_filter_t
>  if ((ctx->stream.total_out & 0x) != compLen) {
>  inflateEnd(&ctx->stream);
>  ap_log_rerror(APLOG_MARK, APLOG_WARNING, 0, r, 
> APLOGNO(01395)
> -  "Zlib: Length %ld of inflated data 
> does "
> -  "not match expected value %ld",
> -  ctx->stream.total_out, compLen);
> +  "Zlib: Length %" APR_UINT64_T_FMT
> +  " of inflated data does not match"
> +  " expected value %ld",
> +  (uint64_t)ctx->stream.total_out, 
> compLen);
>  return APR_EGENERAL;
>  }
>  }
> @@ -1628,8 +1632,10 @@ static apr_status_t inflate_out_filter(ap_filter_t
>   */
>  flush_libz_buffer(ctx, c, inflate, Z_SYNC_FLUSH, UPDATE_CRC);
>  ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(01398)
> -  "Zlib: Inflated %ld to %ld : URL %s",
> -  ctx->stream.total_in, ctx->stream.total_out, 
> r->uri);
> +  "Zlib: Inflated %" APR_UINT64_T_FMT 
> +  " to %" APR_UINT64_T_FMT " : URL %s",
> +  (uint64_t)ctx->stream.total_in,
> +  (uint64_t)ctx->stream.total_out, r->uri);
>  
>  if (ctx->validation_buffer_length == VALIDATION_SIZE) {
>  unsigned long compCRC, compLen;

Re: [PATCH] mod_deflate: hardcoded "%ld" -> APR_OFF_T_FMT

[Stefan Sperling]

On Wed, Dec 19, 2018 at 02:58:28PM +0100, Yann Ylavic wrote:
> On Wed, Dec 19, 2018 at 9:53 AM Stefan Sperling  wrote:
> >
> > On Tue, Dec 18, 2018 at 12:29:18AM +0100, Yann Ylavic wrote:
> > > But yes, upcast is better, while at it I'd go for uint64_t...
> >
> > Like this?
> 
> I think APR_UINT64_T_FMT/apr_uint64_t would be more portable ;)
> 
> Thanks for taking care of this !

Sure :)

Index: modules/filters/mod_deflate.c
===
--- modules/filters/mod_deflate.c   (revision 1849274)
+++ modules/filters/mod_deflate.c   (working copy)
@@ -893,8 +893,10 @@ static apr_status_t deflate_out_filter(ap_filter_t
f->c->bucket_alloc);
 APR_BRIGADE_INSERT_TAIL(ctx->bb, b);
 ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(01384)
-  "Zlib: Compressed %ld to %ld : URL %s",
-  ctx->stream.total_in, ctx->stream.total_out, r->uri);
+  "Zlib: Compressed %" APR_UINT64_T_FMT
+  " to %" APR_UINT64_T_FMT " : URL %s",
+  (uint64_t)ctx->stream.total_in,
+  (uint64_t)ctx->stream.total_out, r->uri);
 
 /* leave notes for logging */
 if (c->note_input_name) {
@@ -1438,9 +1440,10 @@ static apr_status_t deflate_in_filter(ap_filter_t
 ctx->validation_buffer_length += valid;
 
 ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(01393)
-  "Zlib: Inflated %ld to %ld : URL %s",
-  ctx->stream.total_in, ctx->stream.total_out,
-  r->uri);
+  "Zlib: Inflated %" APR_UINT64_T_FMT
+  " to %" APR_UINT64_T_FMT " : URL %s",
+  (uint64_t)ctx->stream.total_in,
+  (uint64_t)ctx->stream.total_out, r->uri);
 
 consume_buffer(ctx, c, c->bufferSize - ctx->stream.avail_out,
UPDATE_CRC, ctx->proc_bb);
@@ -1459,9 +1462,10 @@ static apr_status_t deflate_in_filter(ap_filter_t
 if ((ctx->stream.total_out & 0x) != compLen) {
 inflateEnd(&ctx->stream);
 ap_log_rerror(APLOG_MARK, APLOG_WARNING, 0, r, 
APLOGNO(01395)
-  "Zlib: Length %ld of inflated data does "
-  "not match expected value %ld",
-  ctx->stream.total_out, compLen);
+  "Zlib: Length %" APR_UINT64_T_FMT
+  " of inflated data does not match"
+  " expected value %ld",
+  (uint64_t)ctx->stream.total_out, 
compLen);
 return APR_EGENERAL;
 }
 }
@@ -1628,8 +1632,10 @@ static apr_status_t inflate_out_filter(ap_filter_t
  */
 flush_libz_buffer(ctx, c, inflate, Z_SYNC_FLUSH, UPDATE_CRC);
 ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(01398)
-  "Zlib: Inflated %ld to %ld : URL %s",
-  ctx->stream.total_in, ctx->stream.total_out, r->uri);
+  "Zlib: Inflated %" APR_UINT64_T_FMT 
+  " to %" APR_UINT64_T_FMT " : URL %s",
+  (uint64_t)ctx->stream.total_in,
+  (uint64_t)ctx->stream.total_out, r->uri);
 
 if (ctx->validation_buffer_length == VALIDATION_SIZE) {
 unsigned long compCRC, compLen;

Re: [PATCH] mod_deflate: hardcoded "%ld" -> APR_OFF_T_FMT

[Yann Ylavic]

On Wed, Dec 19, 2018 at 9:53 AM Stefan Sperling  wrote:
>
> On Tue, Dec 18, 2018 at 12:29:18AM +0100, Yann Ylavic wrote:
> > But yes, upcast is better, while at it I'd go for uint64_t...
>
> Like this?

I think APR_UINT64_T_FMT/apr_uint64_t would be more portable ;)

Thanks for taking care of this !

Regards,
Yann.

Re: [PATCH] mod_deflate: hardcoded "%ld" -> APR_OFF_T_FMT

[Stefan Sperling]

On Tue, Dec 18, 2018 at 12:29:18AM +0100, Yann Ylavic wrote:
> But yes, upcast is better, while at it I'd go for uint64_t...

Like this?

I've noticed that the same problem seems to exist in some other modules.
I'll send separate patches for those once this patch has settled.

Index: modules/filters/mod_deflate.c
===
--- modules/filters/mod_deflate.c   (revision 1849274)
+++ modules/filters/mod_deflate.c   (working copy)
@@ -893,8 +893,9 @@ static apr_status_t deflate_out_filter(ap_filter_t
f->c->bucket_alloc);
 APR_BRIGADE_INSERT_TAIL(ctx->bb, b);
 ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(01384)
-  "Zlib: Compressed %ld to %ld : URL %s",
-  ctx->stream.total_in, ctx->stream.total_out, r->uri);
+  "Zlib: Compressed %llu to %llu: URL %s",
+  (uint64_t)ctx->stream.total_in,
+  (uint64_t)ctx->stream.total_out, r->uri);
 
 /* leave notes for logging */
 if (c->note_input_name) {
@@ -1438,9 +1439,9 @@ static apr_status_t deflate_in_filter(ap_filter_t
 ctx->validation_buffer_length += valid;
 
 ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(01393)
-  "Zlib: Inflated %ld to %ld : URL %s",
-  ctx->stream.total_in, ctx->stream.total_out,
-  r->uri);
+  "Zlib: Inflated %llu to %llu : URL %s",
+  (uint64_t)ctx->stream.total_in,
+  (uint64_t)ctx->stream.total_out, r->uri);
 
 consume_buffer(ctx, c, c->bufferSize - ctx->stream.avail_out,
UPDATE_CRC, ctx->proc_bb);
@@ -1459,9 +1460,9 @@ static apr_status_t deflate_in_filter(ap_filter_t
 if ((ctx->stream.total_out & 0x) != compLen) {
 inflateEnd(&ctx->stream);
 ap_log_rerror(APLOG_MARK, APLOG_WARNING, 0, r, 
APLOGNO(01395)
-  "Zlib: Length %ld of inflated data does "
+  "Zlib: Length %llu of inflated data does 
"
   "not match expected value %ld",
-  ctx->stream.total_out, compLen);
+  (uint64_t)ctx->stream.total_out, 
compLen);
 return APR_EGENERAL;
 }
 }
@@ -1628,8 +1629,9 @@ static apr_status_t inflate_out_filter(ap_filter_t
  */
 flush_libz_buffer(ctx, c, inflate, Z_SYNC_FLUSH, UPDATE_CRC);
 ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(01398)
-  "Zlib: Inflated %ld to %ld : URL %s",
-  ctx->stream.total_in, ctx->stream.total_out, r->uri);
+  "Zlib: Inflated %llu to %llu: URL %s",
+  (uint64_t)ctx->stream.total_in,
+  (uint64_t)ctx->stream.total_out, r->uri);
 
 if (ctx->validation_buffer_length == VALIDATION_SIZE) {
 unsigned long compCRC, compLen;

Re: [PATCH] mod_deflate: hardcoded "%ld" -> APR_OFF_T_FMT

[Yann Ylavic]

On Mon, Dec 17, 2018 at 5:40 PM William A Rowe Jr  wrote:
>
> On Sun, Dec 16, 2018 at 7:27 AM Yann Ylavic  wrote:
>>
>>
>> Since it's logging only, it may be easier to cast to (long) each
>> total_in/out though.
>
> Downcast? Why not upcast to apr_off_t and use the _FMT macro as first 
> suggested?

Neither is ideal I think since in the unsigned long case that's still
a cast to signed off_t.
But yes, upcast is better, while at it I'd go for uint64_t...

Re: [PATCH] mod_deflate: hardcoded "%ld" -> APR_OFF_T_FMT

[William A Rowe Jr]

On Sun, Dec 16, 2018 at 7:27 AM Yann Ylavic  wrote:

>
> Since it's logging only, it may be easier to cast to (long) each
> total_in/out though.
>

Downcast? Why not upcast to apr_off_t and use the _FMT macro as first
suggested?

Re: [PATCH] mod_deflate: hardcoded "%ld" -> APR_OFF_T_FMT

[Yann Ylavic]

On Sun, Dec 16, 2018 at 2:21 PM Yann Ylavic  wrote:
>
> On Sun, Dec 16, 2018 at 2:14 PM Stefan Sperling  wrote:
> >
> > The file /usr/include/zlib.h I have on OpenBSD -current has this:
> >
> > typedef struct z_stream_s {
> > [...]
> > z_off_t  total_in;  /* total nb of input bytes read so far */
> > [...]
> > z_off_t  total_out; /* total nb of bytes output so far */
> > [...]
> > } z_stream;
>
> Ouch.
>
> >
> > This looks like an OpenBSD-specific change though (diff below).
> > I guess this will force OpenBSD to carry a local patch for
> > mod_deflate then, or just compile without -Werror.
>
> I think we need a bit of #ifdef-ery in mod_deflate then, a BSD only
> fix wouldn't help those who build from sources.
>
> Do you know which BSD(s) are concerned (if not all)?

Since it's logging only, it may be easier to cast to (long) each
total_in/out though.

Re: [PATCH] mod_deflate: hardcoded "%ld" -> APR_OFF_T_FMT

[Yann Ylavic]

On Sun, Dec 16, 2018 at 2:14 PM Stefan Sperling  wrote:
>
> The file /usr/include/zlib.h I have on OpenBSD -current has this:
>
> typedef struct z_stream_s {
> [...]
> z_off_t  total_in;  /* total nb of input bytes read so far */
> [...]
> z_off_t  total_out; /* total nb of bytes output so far */
> [...]
> } z_stream;

Ouch.

>
> This looks like an OpenBSD-specific change though (diff below).
> I guess this will force OpenBSD to carry a local patch for
> mod_deflate then, or just compile without -Werror.

I think we need a bit of #ifdef-ery in mod_deflate then, a BSD only
fix wouldn't help those who build from sources.

Do you know which BSD(s) are concerned (if not all)?

Re: [PATCH] mod_deflate: hardcoded "%ld" -> APR_OFF_T_FMT

[Stefan Sperling]

On Sun, Dec 16, 2018 at 02:03:45PM +0100, Yann Ylavic wrote:
> On Sun, Dec 16, 2018 at 1:28 PM Stefan Sperling  wrote:
> >
> > mod_deflates hard-codes some off_t format directives to "%ld".
> > It seems to me this code should use the macro provided by APR instead.
> >
> > Looking for another pair of eyes. Does this patch look good to commit?
> 
> It seems that zlib defines total_in/out as uLong, i.e.:
> typedef struct z_stream_s {
> [...]
> uLongtotal_in;  /* total number of input bytes read so far */
> [...]
> uLongtotal_out; /* total number of bytes output so far */
> [...]
> } z_stream;
> 
> So %ld (or %lu) looks correct to me.
> 
> (mod_brotli uses apr_off_t for them but it's an internal struct there).
> 
> Regards,
> Yann.

Thanks for checking. This seems to depend on the version of zlib.
The file /usr/include/zlib.h I have on OpenBSD -current has this:

typedef struct z_stream_s {
[...]
z_off_t  total_in;  /* total nb of input bytes read so far */
[...]
z_off_t  total_out; /* total nb of bytes output so far */
[...]
} z_stream;

And the compiler (clang) now rightly complains as follows:

/home/stsp/svn/src/httpd-2.4.37/modules/filters/mod_deflate.c:856:27: error:   
  format specifies type 'long' but the argument has type 'off_t'   
  (aka 'long long') [-Werror,-Wformat] 
  ctx->stream.total_in, ctx->stream.total_out, r->uri);
  ^~~~ 
/home/stsp/svn/src/httpd-2.4.37/modules/filters/mod_deflate.c:856:49: error:   
  format specifies type 'long' but the argument has type 'off_t'   
  (aka 'long long') [-Werror,-Wformat] 
  ctx->stream.total_in, ctx->stream.total_out, r->uri);
^  
/home/stsp/svn/src/httpd-2.4.37/modules/filters/mod_deflate.c:1423:31: error:  
  format specifies type 'long' but the argument has type 'off_t'   
  (aka 'long long') [-Werror,-Wformat] 
  ctx->stream.total_in, ctx->stream.total_out, 
  ^~~~ 
/home/stsp/svn/src/httpd-2.4.37/modules/filters/mod_deflate.c:1423:53: error:  
  format specifies type 'long' but the argument has type 'off_t'   
  (aka 'long long') [-Werror,-Wformat] 
  ctx->stream.total_in, ctx->stream.total_out, 
^  
/home/stsp/svn/src/httpd-2.4.37/modules/filters/mod_deflate.c:1450:39: error:  
  format specifies type 'long' but the argument has type 'off_t'   
  (aka 'long long') [-Werror,-Wformat] 
  ctx->stream.total_out, compLen); 
  ^
/home/stsp/svn/src/httpd-2.4.37/modules/filters/mod_deflate.c:1626:27: error:  
  format specifies type 'long' but the argument has type 'off_t'   
  (aka 'long long') [-Werror,-Wformat] 
  ctx->stream.total_in, ctx->stream.total_out, r->uri);
  ^~~~ 
/home/stsp/svn/src/httpd-2.4.37/modules/filters/mod_deflate.c:1626:49: error:  
  format specifies type 'long' but the argument has type 'off_t'   
  (aka 'long long') [-Werror,-Wformat] 
  ctx->stream.total_in, ctx->stream.total_out, r->uri);
^  
7 errors generated.

This looks like an OpenBSD-specific change though (diff below).
I guess this will force OpenBSD to carry a local patch for
mod_deflate then, or just compile without -Werror.

RCS file: /cvs/src/lib/libz/zlib.h,v
Working file: zlib.h
head: 1.10
[...]

revision 1.7
date: 2003/12/16 22:35:50;  author: henning;  state: Exp;  lines: +2 -2;
total_in and total_out need to be off_t, not unsigned long.
some bugs return: i fixed the same some months ago when we had this other gzip
there.
this bug resulted in wrong size stats for > 4GB files, and in the case
that the input file was > 4GB and could be compressed to < 4GB gzip
not zipping it as it would grow in its eyes.
=

Index: zlib.h
===
RCS file: /cvs/src/lib/libz/zlib.h,v
retrieving revision 1.6
retrieving revision 1.7
diff -u -p -r1.6 -r1.7
--- zlib.h  16 Dec 2003 22:33:02 -  1.6

Re: [PATCH] mod_deflate: hardcoded "%ld" -> APR_OFF_T_FMT

[Yann Ylavic]

On Sun, Dec 16, 2018 at 1:28 PM Stefan Sperling  wrote:
>
> mod_deflates hard-codes some off_t format directives to "%ld".
> It seems to me this code should use the macro provided by APR instead.
>
> Looking for another pair of eyes. Does this patch look good to commit?

It seems that zlib defines total_in/out as uLong, i.e.:
typedef struct z_stream_s {
[...]
uLongtotal_in;  /* total number of input bytes read so far */
[...]
uLongtotal_out; /* total number of bytes output so far */
[...]
} z_stream;

So %ld (or %lu) looks correct to me.

(mod_brotli uses apr_off_t for them but it's an internal struct there).

Regards,
Yann.

Re: [PATCH]mod_deflate check return of apr_bucket_read -- correct

[Brian Akins]

Sent the wrong one.


--
Brian Akins
Lead Systems Engineer
CNN Internet Technologies
--- mod_deflate.c~  2005-11-10 10:20:05.0 -0500
+++ mod_deflate.c   2006-03-28 14:07:32.0 -0500
@@ -401,7 +401,8 @@
 apr_bucket *b;
 apr_size_t len;
 int done = 0;
-
+apr_status_t rv;
+
 e = APR_BRIGADE_FIRST(bb);
 
 if (APR_BUCKET_IS_EOS(e)) {
@@ -489,7 +490,6 @@
 
 if (APR_BUCKET_IS_FLUSH(e)) {
 apr_bucket *bkt;
-apr_status_t rv;
 
 apr_bucket_delete(e);
 
@@ -518,7 +518,10 @@
 }
 
 /* read */
-apr_bucket_read(e, &data, &len, APR_BLOCK_READ);
+rv = apr_bucket_read(e, &data, &len, APR_BLOCK_READ);
+if (rv != APR_SUCCESS) {
+return rv;
+}
 
 /* This crc32 function is from zlib. */
 ctx->crc = crc32(ctx->crc, (const Bytef *)data, len);
@@ -711,7 +714,10 @@
 }
 
 /* read */
-apr_bucket_read(bkt, &data, &len, APR_BLOCK_READ);
+rv = apr_bucket_read(bkt, &data, &len, APR_BLOCK_READ);
+if (rv != APR_SUCCESS) {
+return rv;
+}
 
 /* pass through zlib inflate. */
 ctx->stream.next_in = (unsigned char *)data;
@@ -953,7 +959,10 @@
 }
 
 /* read */
-apr_bucket_read(bkt, &data, &len, APR_BLOCK_READ);
+rv = apr_bucket_read(bkt, &data, &len, APR_BLOCK_READ);
+if (rv != APR_SUCCESS) {
+return rv;
+}
 
 /* first bucket contains zlib header */
 if (!deflate_init++) {

Re: [PATCH] mod_deflate + mod_proxy bug

[Allan Edwards]

Joe Orton wrote:
Wouldn't it be simpler to just check for a brigade containing just EOS
and do nothing for that case in the first place?
Yes I had considered that. The initial patch covered some pathological
cases but after having looked over the code some more I think the simpler
more efficient way of bailing on just EOS is sufficient.
But the fact that the proxy passes such a brigade through the output
filters in the first place sounds like the real bug, it doesn't happen
for a non-proxied 304 response.
Whether or not this is a bug I guess is open for debate. What happens
for non proxied error responses is that ap_send_error_response resets
r->output_flters to r->proto_output_filters so the deflate filter is
taken out of the respone path. In the proxy path there is no such logic
for error responses, so error pages would get zipped. But I don't know
that this violates the RFC and browsers seem to be able to handle it.
Allan

Re: [PATCH] mod_deflate + mod_proxy bug

[Nick Kew]

On Wed, 9 Jun 2004, Allan Edwards wrote:

> Running ProxyPass with mod_deflate results in
> an extraneous 20 bytes being tacked onto 304
> responses from the backend.
>
> The problem is that mod_deflate doesn't handle
> the zero byte body, adds the gzip header and
> tries to compress 0 bytes.
>
> This patch detects the fact that there was no
> data to compress and removes the gzip header
> from the bucket brigade.
>
> Any comments before I commit to head?

This is part of a slightly broader problem with proxying and mod_deflate:
it'll also waste time gzipping already-compressed data from the backend
in those cases where the compression is not explicitly indicated in the
Content-Encoding header.  Obvious examples are all the main image formats.

I'm currently running a hack that works around this, and planning a
better review when time permits (i.e. when I've caught up with things
after http://www.theatreroyal.com/showpage.php?dd=1&theid=2578
which now has three nights left to run).

More interesting is the entire subject of filtering in a dynamic
context such as a proxy.  The directives available to control filtering
are simply not up to it.  Watch this space:-)

-- 
Nick Kew

Re: [PATCH] mod_deflate + mod_proxy bug

[Joe Orton]

On Wed, Jun 09, 2004 at 05:23:38PM -0400, Allan Edwards wrote:
> Running ProxyPass with mod_deflate results in
> an extraneous 20 bytes being tacked onto 304
> responses from the backend.
> 
> The problem is that mod_deflate doesn't handle
> the zero byte body, adds the gzip header and
> tries to compress 0 bytes.
> 
> This patch detects the fact that there was no
> data to compress and removes the gzip header
> from the bucket brigade.

Wouldn't it be simpler to just check for a brigade containing just EOS
and do nothing for that case in the first place?

But the fact that the proxy passes such a brigade through the output
filters in the first place sounds like the real bug, it doesn't happen
for a non-proxied 304 response.

joe

Re: [PATCH] mod_deflate + mod_proxy bug

[Cliff Woolley]

On Wed, 9 Jun 2004, Allan Edwards wrote:

> Also just realized I need to add a call to deflateEnd().

Oh right, that too.  :-)

> e is on the brigade passed into deflate_out_filter and the gzip
> header bucket is in ctx->bb so that is not a problem.

Ah, yeah, that would make sense.  Cool.

Re: [PATCH] mod_deflate + mod_proxy bug

[Allan Edwards]

Cliff Woolley wrote:
I haven't looked at the entire context of this, but if you remove a bucket
(brigade_first(ctx->bb) from a brigade without deleting it and without
having any extra pointers to it, you'll leak memory.
Thanks for catching that! I'll replace APR_BUCKET_REMOVE with
a call to apr_bucket_delete(). Also just realized I need to add
a call to deflateEnd().
Also, what happens if e *is* the first bucket in the brigade?  Can that
occur?  I think that by coincidence given the implementation of
APR_BUCKET_REMOVE, nothing bad would happen by double-removing a given
bucket twice in a row, but in general that seems like a bad idea and
should be avoided.
e is on the brigade passed into deflate_out_filter and the gzip
header bucket is in ctx->bb so that is not a problem.
Thanks, Allan

Re: [PATCH] mod_deflate + mod_proxy bug

[Cliff Woolley]

On Wed, 9 Jun 2004, Allan Edwards wrote:

> +}
> +else {
> +/* this was a zero length response, remove gzip header bucket then 
> pass down the EOS */
> +APR_BUCKET_REMOVE(APR_BRIGADE_FIRST(ctx->bb));
> +APR_BUCKET_REMOVE(e);
> +APR_BRIGADE_INSERT_TAIL(ctx->bb, e);
> +return ap_pass_brigade(f->next, ctx->bb);
> +}

I haven't looked at the entire context of this, but if you remove a bucket
(brigade_first(ctx->bb) from a brigade without deleting it and without
having any extra pointers to it, you'll leak memory.

Also, what happens if e *is* the first bucket in the brigade?  Can that
occur?  I think that by coincidence given the implementation of
APR_BUCKET_REMOVE, nothing bad would happen by double-removing a given
bucket twice in a row, but in general that seems like a bad idea and
should be avoided.

--Cliff

Re: [PATCH] mod_deflate extensions

[Henri Gomez]

William A. Rowe, Jr. wrote:

At 09:58 AM 11/21/2002, Henri Gomez wrote:


So what about for 2.0.45 dev ?


My prediction about interest in such a switch was independent of
timeframe.  I doubt that such a switch will ever happen.


2.0.44 won't be the last 2.0 release isn't it ?



No... 2.0.45 will be compatible with 2.0.44 modules, if and when 
a significant number of bugfixes or a security hole is patched.


2.0.x will simply remain compatible with 2.0.42 forward.

Authors may choose not to backport new features to 2.0, or they
may do so, it's up to them.


Ok



What do you means ?




Actually, we do need decompression in some cases (which was subject
to the security hole suffered with earlier zlib versions).  Proxy might be
one such example.  Another would be ungzipping cached content based 
on the client accept-encoding details.

Another candidate for zlib stuff in Apache 2.0 ;)


- Put part of zlib code in Apache 2.0 source ?

That is an interesting concept - I actually just took the time to get
zlib building on win32 with a parallel configure.pl script.  I'll post that
fun at some point.  The biggest PITA on Win32 was the choice of
compiling against static c libraries instead of msvcrt which Apache
lives with.  

That's bad


Building in some static clib while leaving others dynamic just seemed 
evil to me, so none of the win32 library-build examples work out of 
the box to give you msvcrt linkage.  My playing with a configure.pl
script was just another means to that same end.



Or what about adding zlib compression in APR project, which is more system related and use it ? The APR 0.9 branch is still open ?



Good idea.  The point is that we can always add to a branch for a future
release as long as we aren't breaking compatibility.


And I feel that others APRIZED applications will benefits a ZLIB wrapper.

Re: [PATCH] mod_deflate extensions

[Henri Gomez]

Ok, let be pragmatic. Did Apache HTTP developpers agree that
compression should be added in Apache 2.0 by incorporating mod_gzip
comp code in Apache 2.0 ?



mod_deflate is already there and it uses an external zlib library, so 
I'm confused why we should also provide mod_gzip and/or its proprietary 
compression code.

I'm fine with mod_deflate, my only request will be to make it built by 
default by configure/make if configure find a zlib shared lib or dll on 
the build machine.

mod_gzip is freely available, and the ASF doesn't need to distribute it 
(Remote Communications evangelizes it enough).  One of the main reasons 
for selecting mod_deflate was that it didn't unnecessarily duplicate 
code.  Less code is better.   We don't need to repackage zlib.  I have 
no desire for us to compete with the zlib maintainers. We have enough 
work as-is.  -- justin

The idea was to make APR support zlib to make mod_deflate or others 
modules, I speak about mod_jk2 future requirements, William about 
mod_proxy current'one.

I think we could have APR hidding zlib the same way it does for DBM.

What do you think ?

Re: [PATCH] mod_deflate extensions

[TOKILEY]

>--On Friday, November 22, 2002 12:03 PM +0100 Henri Gomez
><[EMAIL PROTECTED]> wrote:
>
>> So we should use a copy of mod_gzip compression code in Apache 2.0.
>> Also as someone involved in mod_jk/jk2, I'll need gzip
>> compress/uncompress support in Apache 2.0 for a new ajp protocol
>> I'm working on, so having compression/uncompression in Apache 2.0
>> will be mandatory for me.
>
> Justin wrote...
>
> No, we shouldn't be including zlib code (or any variant) in our
> distribution.  That's not our responsibility.  It's also not
> important enough for us to further bloat our code just because an
> insignificant number of distributions haven't provided a good package
> of zlib.  If it's that important, those administrators can build
> their own zlib or just not use any functionality requiring zlib.  The
> point here is that no functionality is lost if zlib is missing.

I guess that's where some would disagree and the point
of Henri's original posting. I happen to think that a LOT
of 'functionality' is 'missing' from Apache if it can't
do compression/decompression 'out of the box' but that's
certainly no secret since I've been voicing that opinion
for some years now. It's still just one man's opinion,
as is yours. Diff strokes for Diff folks.

> (And, if you are doing a mod_jk, zlib support should be optional not
> mandatory.)
>
>> Ok, let be pragmatic. Did Apache HTTP developpers agree that
>> compression should be added in Apache 2.0 by incorporating mod_gzip
>> comp code in Apache 2.0 ?
>
> mod_deflate is already there and it uses an external zlib library, so
> I'm confused why we should also provide mod_gzip and/or its
> proprietary compression code.

No one has suggested 'also providing mod_gzip'. That's a
dead issue. Apache will never be using 'mod_gzip'.

The issue was whether or not it's time to think about
adding some actual compression/decompression code (like
ZLIB) to the source tree itself and/or the Non-ZLIB inline
compression engine that mod_gzip uses which is perfectly
do-able and pretty much a no-brainer.

...and for the record... the 'compression code' in mod_gzip
is NOT PROPRIETARY. It is still just simple public domain
LZ77 + Huffman. It is as 'open-source' as your own
product (Apache), if not even more-so. ALL of the code
for mod-gzip ( compression engine(s) included ) has been
donated to Apache 3 different times and it's all still
sitting there on your hard drives ready for you to do
whatever the heck you want with it. All of mod_gzip
is also now sitting up at SOURCEFORGE free as the wind
and under the same 'do whatever you like with this'
conditions.

> mod_gzip is freely available, and the ASF doesn't need to distribute
> it (Remote Communications evangelizes it enough).

RCI has nothing do with mod_gzip anymore.
mod_gzip is all sitting up on SOURCEFORGE for some time now.

> One of the main
> reasons for selecting mod_deflate was that it didn't unnecessarily
> duplicate code.  Less code is better.   We don't need to repackage
> zlib.  I have no desire for us to compete with the zlib maintainers.
> We have enough work as-is.  -- justin

All points well taken.

Your points go against the advice of people
who actually wrote ZLIB ( Dr. Mark Adler and others )
with regards to anyone who 'distributes' applications
that (may) depend on it (ZLIB) but your concerns about
how much the Apache Group is able to deal with are
well-grounded.

Jeff Trawick has already said he thinks including
the 'minimal' amount of code neeeded by Apache
to do what mod_deflate needs to do in the SRC
tree itself would be GOODNESS and would
circumvent a lot of build problems/bug reports but
therein lies the issue itself.

If it comes to a vote I would focus on that one
single issue. Forget about mod_gzip's LZ77 engine
or any other version of LZ77 that 'could' be used...
if the Apache Group isn't even willing to add
a minimal subset of ZLIB code to the SRC tree
and compile/link against it then there's no use
discussing whether that extends to any 'other'
compression/decompresion code as well.

Yours...
Kevin

Re: [PATCH] mod_deflate extensions

[Justin Erenkrantz]

--On Friday, November 22, 2002 12:03 PM +0100 Henri Gomez 
<[EMAIL PROTECTED]> wrote:

So we should use a copy of mod_gzip compression code in Apache 2.0.
Also as someone involved in mod_jk/jk2, I'll need gzip
compress/uncompress support in Apache 2.0 for a new ajp protocol
I'm working on, so having compression/uncompression in Apache 2.0
will be mandatory for me.


No, we shouldn't be including zlib code (or any variant) in our 
distribution.  That's not our responsibility.  It's also not 
important enough for us to further bloat our code just because an 
insignificant number of distributions haven't provided a good package 
of zlib.  If it's that important, those administrators can build 
their own zlib or just not use any functionality requiring zlib.  The 
point here is that no functionality is lost if zlib is missing. 
(And, if you are doing a mod_jk, zlib support should be optional not 
mandatory.)

Ok, let be pragmatic. Did Apache HTTP developpers agree that
compression should be added in Apache 2.0 by incorporating mod_gzip
comp code in Apache 2.0 ?


mod_deflate is already there and it uses an external zlib library, so 
I'm confused why we should also provide mod_gzip and/or its 
proprietary compression code.

mod_gzip is freely available, and the ASF doesn't need to distribute 
it (Remote Communications evangelizes it enough).  One of the main 
reasons for selecting mod_deflate was that it didn't unnecessarily 
duplicate code.  Less code is better.   We don't need to repackage 
zlib.  I have no desire for us to compete with the zlib maintainers. 
We have enough work as-is.  -- justin

Re: [PATCH] mod_deflate extensions

[William A. Rowe, Jr.]

At 09:58 AM 11/21/2002, Henri Gomez wrote:
>>>So what about for 2.0.45 dev ?
>>
>>My prediction about interest in such a switch was independent of
>>timeframe.  I doubt that such a switch will ever happen.
>
>2.0.44 won't be the last 2.0 release isn't it ?

No... 2.0.45 will be compatible with 2.0.44 modules, if and when 
a significant number of bugfixes or a security hole is patched.

2.0.x will simply remain compatible with 2.0.42 forward.

Authors may choose not to backport new features to 2.0, or they
may do so, it's up to them.

>>>What do you means ?
>>>
>>>- Put zlib full source tree in Apache 2.0 tree and make
>>>  a static build ?
>>
>>not all of zlib is necessary...  and if we build zlib as a
>>library (instead of pulling specific objects into mod_deflate.so) we
>>have portability problems to deal with...
>
>We could grab the compression part, like does mod_gzip, mod_gzip even use zlib 
>includes.

Actually, we do need decompression in some cases (which was subject
to the security hole suffered with earlier zlib versions).  Proxy might be
one such example.  Another would be ungzipping cached content based 
on the client accept-encoding details.

>>>- Put part of zlib code in Apache 2.0 source ?
>>
>>that is what I suspect to be the safest, easiest-to-understand way...
>>the build would work like on Windows, where the project file for
>>mod_deflate pulls in the right parts when building mod_deflate.so
>
>Ok

That is an interesting concept - I actually just took the time to get
zlib building on win32 with a parallel configure.pl script.  I'll post that
fun at some point.  The biggest PITA on Win32 was the choice of
compiling against static c libraries instead of msvcrt which Apache
lives with.  

Building in some static clib while leaving others dynamic just seemed 
evil to me, so none of the win32 library-build examples work out of 
the box to give you msvcrt linkage.  My playing with a configure.pl
script was just another means to that same end.

>>another nice feature of this is that if we know we are using our build
>>of the zlib source then we can turn on the zlib compile option to
>>namespace-protect the zlib routines and then change mod_deflate to
>>assume that (but perhaps the compile switch will make mod_deflate use
>>the namespace-protected versions automatically anyway)...  OtherBill
>>mentioned this namespace protection before but I haven't had a chance
>>to play with it
>
>Or what about adding zlib compression in APR project, which is more system related 
>and use it ? The APR 0.9 branch is still open ?

Good idea.  The point is that we can always add to a branch for a future
release as long as we aren't breaking compatibility.

>>I already saw a user having problems because a shared library used by
>>a 3rd party module also had a routine called crc32().
>
>Yes, and I've encountered problem with mod_gzip because one the PHP module was linked 
>against ct_lib ;)

Such a hassle, yes.

>>>- or make configure detect zlib shared lib and make mod_deflate
>>>  available by default ?
>>
>>if this can be made foolproof then this is perhaps a good option
>>too...  but I'm not sure that is portable on whether the DSO needs to
>>be linked with zlib or whether httpd needs to be linked with zlib...
>>and if httpd needs to be linked with zlib then the export file
>>generation needs to do the right thing...  IIRC, one solution that
>>would work fine on Linux and Solaris and FreeBSD wouldn't work on
>>HP-UX (I might have the platforms listed in the wrong place; maybe AIX
>>was one of the platforms)...
>
>So extracting zlib source part and include them in Apache or APR seems to be the best 
>solution.

Re: [PATCH] mod_deflate extensions

[Henri Gomez]

I also refer you to the discussion thread regarding the
original inclusion of mod_deflate which contains some
'advice' posted to the Apache forum from Dr. Mark Adler
( one of the original authors of all this GZIP/ZLIB LZ77
code ).

He suggested that compiling your OWN version of GZIP/ZLIB
was pretty much the only 'sane' thing to do and I agree
100%. There are actually a number of 'flaky' distributions
of GZIP/ZLIB 'out there'.

He ( Mark Adler ) himself pointed out that there are 
some 'patches' floating around for GZIP/ZLIB that never 
made it into ANY standard distribution and only by applying 
them yourself to your own compiled OBJ/LIB could you be
sure what you are actually using and what shape it is in.

So we should use a copy of mod_gzip compression code in Apache 2.0.
Also as someone involved in mod_jk/jk2, I'll need gzip 
compress/uncompress support in Apache 2.0 for a new ajp protocol I'm 
working on, so having compression/uncompression in Apache 2.0 will be 
mandatory for me.


If they REALLY WANT some 'feature' in their Server... they
will tough it out and get it done. That's the way it's 
always been with Apache so no one would be shocked if some LIB 
wasn't in the right place or a makefile needed tweaking.

Ok, let be pragmatic. Did Apache HTTP developpers agree that compression
should be added in Apache 2.0 by incorporating mod_gzip comp code in 
Apache 2.0 ?

Re: [PATCH] mod_deflate extensions

[TOKILEY]

>> Henri Gomez wrote...
>>
>> - Put part of zlib code in Apache 2.0 source ?
>
> Jeff Trawick wrote...
>
> that is what I suspect to be the safest, easiest-to-understand way...
> the build would work like on Windows, where the project file for
> mod_deflate pulls in the right parts when building mod_deflate.so
>
> Henri Gomez also wrote...
>
> We could grab the compression part, like does mod_gzip, mod_gzip even 
> use zlib includes.

If the discussion is once again open about including compression
code in a base Apache source release then let me clarify what
Henri just said...

The compression engine in the orginal mod_gzip ( and the
current one in the 1.3 series mod_gzip ) is NOT based
on ZLIB at all. It is based on the ORIGINAL public 
domain GZIP/LZ77 code that pre-dates ZLIB. That LZ77 code
is, in many ways, much 'simpler' than ZLIB since ZLIB
became this kind of 'all things to all people' package and the
I/O and API interface(s) got a lot more complicated than
the original GZIP stuff. ZLIB was meant to be 'easier
to deal with' at the API level but the sacrafice was
more overhead during the compression itself.

The original GZIP code was not thread-safe so the engine
in the original mod_gzip was a thread-safe rewrite that
uses a Finite State Processor ( like SSL codebases ).

Every compression task has a 'context handle' just like
SSL does to maintain the integrity of all the compression
tasks that might be running at the same time. There 
are no 'globals' as there are in GZIP ( and ZLIB? ).

It also got 'tweaked' to do straight pointer based
in-memory compressions which made it a better candidate
for a 'real-time' compression engine. A lot of the
time-consuming things in GZIP were also speeded up with
better loops and code that produced better .ASM output.

So... to make a long story short... mod_gzip actually
includes some original GZIP 'headers' and uses the
same functions names ( deflate/inflate, etc. ) as
legacy GZIP and ZLIB... but those headers were not
from ZLIB.

They didn't change the headers much when they created
ZLIB from GZIP but there are some subtle differences.

NOTE: The original sumbmission of mod_gzip for Apache
2.0 used the same built-in compression engine as the
1.3 series. It was already thread-safe so it would
have been fine for 2.0. There was a LATER re-submission
of mod_gzip for Apache 2.0 that followed an intense
discussion about NOT using such code and it is more
like mod_deflate and does, in fact, just include
ZLIB headers and has the same build expectations
as mod_deflate. BOTH VERSIONS are still sitting there
somewhere in the Apache archives. They were both
submitted some long time ( years now? ) ago.

I also refer you to the discussion thread regarding the
original inclusion of mod_deflate which contains some
'advice' posted to the Apache forum from Dr. Mark Adler
( one of the original authors of all this GZIP/ZLIB LZ77
code ).

He suggested that compiling your OWN version of GZIP/ZLIB
was pretty much the only 'sane' thing to do and I agree
100%. There are actually a number of 'flaky' distributions
of GZIP/ZLIB 'out there'.

He ( Mark Adler ) himself pointed out that there are 
some 'patches' floating around for GZIP/ZLIB that never 
made it into ANY standard distribution and only by applying 
them yourself to your own compiled OBJ/LIB could you be
sure what you are actually using and what shape it is in.

Now that Apache is multi-thread capable and there are some
known issues with legacy GZIP/ZLIB distributions and 
multi-threading it makes even more sense to know EXACTLY 
what 'version' of whatever compression code you are dealing 
with when the threading bug reports start coming in.

> Jeff Trawick also wrote...
>
> The reason I'm being a real stick-in-the-mud about this is because
> Apache users already seem to have enough trouble getting Apache to
> build out of the box, usually because of some screwy setup or other
> unexpected conditions that nobody can ever reproduce, and it is very
> difficult for me to accept that some quick change to turn on
> mod_deflate is going to do anything other than increase these
> problems, particularly since I committed various "user errors" myself
> getting consistent builds of mod_deflate and zlib.  (Heck, I even
> broke the cvs executable on one machine before I found out what was
> going on :) )

I think you underestimate your legacy user base.

Everyone already accepts the fact that building Apache
is usually an absolute nightmare, especially if you
are trying to get any kind of SSL going. 

It hasn't slowed anyone down that I can tell.

They ( historically ) just stay up all night and figure it out.
That's how you build Apache.

If they REALLY WANT some 'feature' in their Server... they
will tough it out and get it done. That's the way it's 
always been with Apache so no one would be shocked if some LIB 
wasn't in the right place or a makefile needed tweaking.

Later...
Kevin Kiley

Re: [PATCH] mod_deflate extensions

[Sander Striker]

Peter J. Cranstone wrote:


And in fine what about mod_deflate to be added by default in
Apache 2.0.44 ?



Here's a reason for this. Content encoding and the ability to send
compressed data is part of the HTTP standard and if Apache 2.x is really
HTTP compliant then it should support it.


The spec defines it as optional.


Why build and offer a new version of Apache which is not 100% HTTP
compliant?


It is 100% HTTP compliant.




Regards,


Peter


Sander

Re: [PATCH] mod_deflate extensions

[Henri Gomez]

So what about for 2.0.45 dev ?



My prediction about interest in such a switch was independent of
timeframe.  I doubt that such a switch will ever happen.


2.0.44 won't be the last 2.0 release isn't it ?


What do you means ?

- Put zlib full source tree in Apache 2.0 tree and make
  a static build ?



not all of zlib is necessary...  and if we build zlib as a
library (instead of pulling specific objects into mod_deflate.so) we
have portability problems to deal with...


We could grab the compression part, like does mod_gzip, mod_gzip even 
use zlib includes.

- Put part of zlib code in Apache 2.0 source ?



that is what I suspect to be the safest, easiest-to-understand way...
the build would work like on Windows, where the project file for
mod_deflate pulls in the right parts when building mod_deflate.so


Ok


another nice feature of this is that if we know we are using our build
of the zlib source then we can turn on the zlib compile option to
namespace-protect the zlib routines and then change mod_deflate to
assume that (but perhaps the compile switch will make mod_deflate use
the namespace-protected versions automatically anyway)...  OtherBill
mentioned this namespace protection before but I haven't had a chance
to play with it


Or what about adding zlib compression in APR project, which is more 
system related and use it ? The APR 0.9 branch is still open ?

I already saw a user having problems because a shared library used by
a 3rd party module also had a routine called crc32().


Yes, and I've encountered problem with mod_gzip because one the PHP 
module was linked against ct_lib ;)

- or make configure detect zlib shared lib and make mod_deflate
  available by default ?



if this can be made foolproof then this is perhaps a good option
too...  but I'm not sure that is portable on whether the DSO needs to
be linked with zlib or whether httpd needs to be linked with zlib...
and if httpd needs to be linked with zlib then the export file
generation needs to do the right thing...  IIRC, one solution that
would work fine on Linux and Solaris and FreeBSD wouldn't work on
HP-UX (I might have the platforms listed in the wrong place; maybe AIX
was one of the platforms)...


So extracting zlib source part and include them in Apache or APR seems 
to be the best solution.

--/--

The reason I'm being a real stick-in-the-mud about this is because
Apache users already seem to have enough trouble getting Apache to
build out of the box, usually because of some screwy setup or other
unexpected conditions that nobody can ever reproduce, and it is very
difficult for me to accept that some quick change to turn on
mod_deflate is going to do anything other than increase these
problems, particularly since I committed various "user errors" myself
getting consistent builds of mod_deflate and zlib.  (Heck, I even
broke the cvs executable on one machine before I found out what was
going on :) )


Under Linux I've got little problem, but it's a widely used and 
supported OS ;)

RE: [PATCH] mod_deflate extensions

[Peter J. Cranstone]

>> And in fine what about mod_deflate to be added by default in
>> Apache 2.0.44 ?

Here's a reason for this. Content encoding and the ability to send
compressed data is part of the HTTP standard and if Apache 2.x is really
HTTP compliant then it should support it.

Why build and offer a new version of Apache which is not 100% HTTP
compliant?

Regards,


Peter



-Original Message-
From: Henri Gomez [mailto:[EMAIL PROTECTED]] 
Sent: Thursday, November 21, 2002 2:55 AM
To: [EMAIL PROTECTED]
Subject: Re: [PATCH] mod_deflate extensions

Bill Stoddard wrote:
>>Pie is rarely free at a truck stop.
>>
> 
> At least none that you would want to dip your fingers into

And in fine what about mod_deflate to be added by default in
Apache 2.0.44 ?

And if so should we use the mod_gzip compression functions instead of 
depending on zlib ?

Re: [PATCH] mod_deflate extensions

[Jeff Trawick]

Henri Gomez <[EMAIL PROTECTED]> writes:

> Jeff Trawick wrote:
> > Henri Gomez <[EMAIL PROTECTED]> writes:
> >
> >>And in fine what about mod_deflate to be added by default in
> >>Apache 2.0.44 ?
> >>
> >>And if so should we use the mod_gzip compression functions instead of
> >>depending on zlib ?
> > I would be shocked if any significant subset of the people who have
> > to
> > support this code would be interested in switching the compression
> > engine anyway.  (And changing deflate to use a different compression
> > engine works against the goal of 2.0.44 being a stable release.)
> 
> So what about for 2.0.45 dev ?

My prediction about interest in such a switch was independent of
timeframe.  I doubt that such a switch will ever happen.

> What do you means ?
> 
> - Put zlib full source tree in Apache 2.0 tree and make
>a static build ?

not all of zlib is necessary...  and if we build zlib as a
library (instead of pulling specific objects into mod_deflate.so) we
have portability problems to deal with...

> - Put part of zlib code in Apache 2.0 source ?

that is what I suspect to be the safest, easiest-to-understand way...
the build would work like on Windows, where the project file for
mod_deflate pulls in the right parts when building mod_deflate.so

another nice feature of this is that if we know we are using our build
of the zlib source then we can turn on the zlib compile option to
namespace-protect the zlib routines and then change mod_deflate to
assume that (but perhaps the compile switch will make mod_deflate use
the namespace-protected versions automatically anyway)...  OtherBill
mentioned this namespace protection before but I haven't had a chance
to play with it

I already saw a user having problems because a shared library used by
a 3rd party module also had a routine called crc32().

> - or make configure detect zlib shared lib and make mod_deflate
>available by default ?

if this can be made foolproof then this is perhaps a good option
too...  but I'm not sure that is portable on whether the DSO needs to
be linked with zlib or whether httpd needs to be linked with zlib...
and if httpd needs to be linked with zlib then the export file
generation needs to do the right thing...  IIRC, one solution that
would work fine on Linux and Solaris and FreeBSD wouldn't work on
HP-UX (I might have the platforms listed in the wrong place; maybe AIX
was one of the platforms)...

--/--

The reason I'm being a real stick-in-the-mud about this is because
Apache users already seem to have enough trouble getting Apache to
build out of the box, usually because of some screwy setup or other
unexpected conditions that nobody can ever reproduce, and it is very
difficult for me to accept that some quick change to turn on
mod_deflate is going to do anything other than increase these
problems, particularly since I committed various "user errors" myself
getting consistent builds of mod_deflate and zlib.  (Heck, I even
broke the cvs executable on one machine before I found out what was
going on :) )

-- 
Jeff Trawick | [EMAIL PROTECTED]
Born in Roswell... married an alien...

Re: [PATCH] mod_deflate extensions

[Henri Gomez]

Jeff Trawick wrote:

Henri Gomez <[EMAIL PROTECTED]> writes:



And in fine what about mod_deflate to be added by default in
Apache 2.0.44 ?

And if so should we use the mod_gzip compression functions instead of
depending on zlib ?



I would be shocked if any significant subset of the people who have to
support this code would be interested in switching the compression
engine anyway.  (And changing deflate to use a different compression
engine works against the goal of 2.0.44 being a stable release.)


So what about for 2.0.45 dev ?


Meanwhile I haven't seen anybody besides myself suggesting that we
bundle the relevant parts of zlib, and in my experience that is
required in order to get a consistently successful build on Unix (not
that I would advocate doing it before 2.0.44 ships anyway).


What do you means ?

- Put zlib full source tree in Apache 2.0 tree and make
  a static build ?

- Put part of zlib code in Apache 2.0 source ?

- or make configure detect zlib shared lib and make mod_deflate
  available by default ?

Re: [PATCH] mod_deflate extensions

[Jeff Trawick]

Henri Gomez <[EMAIL PROTECTED]> writes:

> And in fine what about mod_deflate to be added by default in
> Apache 2.0.44 ?
> 
> And if so should we use the mod_gzip compression functions instead of
> depending on zlib ?

I would be shocked if any significant subset of the people who have to
support this code would be interested in switching the compression
engine anyway.  (And changing deflate to use a different compression
engine works against the goal of 2.0.44 being a stable release.)

Meanwhile I haven't seen anybody besides myself suggesting that we
bundle the relevant parts of zlib, and in my experience that is
required in order to get a consistently successful build on Unix (not
that I would advocate doing it before 2.0.44 ships anyway).

-- 
Jeff Trawick | [EMAIL PROTECTED]
Born in Roswell... married an alien...

Re: [PATCH] mod_deflate extensions

[Henri Gomez]

Bill Stoddard wrote:

Pie is rarely free at a truck stop.



At least none that you would want to dip your fingers into


And in fine what about mod_deflate to be added by default in
Apache 2.0.44 ?

And if so should we use the mod_gzip compression functions instead of 
depending on zlib ?

RE: [PATCH] mod_deflate extensions

[Bill Stoddard]

> Pie is rarely free at a truck stop.
> 
At least none that you would want to dip your fingers into

B 

Re: [PATCH] mod_deflate extensions

[TOKILEY]

>> Peter J. Cranstone wrote...
>>
>> Since when does web server throughput drop by x% factor using
>> mod_deflate?
>
> Jeff Trawick wrote...
>
> I don't think you need me to explain the "why" or the "when" to you.

Think again.

Exactly what scenario are you assuming is supposed to
be so 'obvious' that it doesn't need and explanation/discussion?

There has never been a good discussion and/or presentation
of real data on this topic... just a bunch of 'assumptions'...
and now that compression modules have caching ability whatever
testing HAS been done needs to be done again because perhaps
any/all of the sore spots in anyone's testing can now be
completely eliminated by real-time caching of compressed objects.

All of my experience with compressing Internet Content in
real time on Servers, with or without the caching of the compression
objects, indicates that it USUALLY, if done correctly,
does nothing but INCREASE the 'throughput' of the Server.

Same experience has also shown that if something ends
up being much SLOWER then something is bad WRONG with
the code that's doing it and it is FIXABLE.

The assumption that YOU seem to be clinging to is that once
the Server has bounced through enough APR calls to handle
the transaction with as few things showing up in STRACE
as possible that the Server has done it's job and the
transaction is OVER ( and the CPU somehow magically free
again ).

This is never the case.

Pie is rarely free at a truck stop.

If you dump 100,000 bytes into the I/O subsystem without
taking the (few) milliseconds needed to compress down
to 70-80 percent LESS then SOMETHING in the CPU is still
working MUCH harder than it has to.

The 'data' is not GONE from the box just because the
Server has made some socket calls and gone
about it's business. It still has to be SENT, one
byte at a time, by the same CPU in the same machine.

NIC cards are interrupt driven.

Asking the I/O subsystem to constantly send 70-80 percent
more data than it has to via an interrupt driven
mechanism is basically the most expensive thing you
could ask the CPU to do.

In-memory compression is NOT interrupt driven.

As compared to interrupt driven I/O it is one of the 
LEAST expensive things to ask the CPU to do, on average.

Do not confuse the performance of any given standard distribution
of some legacy compression library called ZLIB with whether
or not, in THEORY, the real-time compression of content
is able to INCREASE the throughput of the Server.

ZLIB was never designed to be used as a 'real-time'
compression engine. The code is VERY OLD and is
still based on a streaming I/O model with heavy
overhead versus direct in-memory compression.

It is a FILE based implementation of LZ77 and
while it performs very well in a batch job against
disk files it still lacks some things which could
qualify it as a high-perfomance real-time
compression engine.

mod_gzip does NOT use 'standard ZLIB' for this
very reason. The performance was not good enough
to produce consistently good throughput.

>> We went through this debate with mod_gzip and it doesn't hold much
>> water. Server boxes are cheap and adding some more ram or even a faster
>> processor is a cheap price to pay when compared to customer satisfaction
>> when their pages load faster.
>
> Your "Server boxes are cheap" comment is very telling; if I add more
> ram or a faster processor we aren't talking about the same web server.

Exactly.

Regardless of the fact that content compression at the source
origin CAN actually 'improve' the throughput of one single
server ( if done correctly ) let me chime in on this point
and say that if adding a little hardware or perhaps even
another ( dirt cheap these days ) Server box is what it
takes to provide a DRAMATIC improvement in the user
experience then what's the gripe?

If that's what it takes to provide a better experience for the
USER then I agree 100% with Peter. That is what SHOULD be the
focus. Your point of view seems to indicate that you believe
it's better to let your USERS have a 'worse experience' than
they need to just to avoid having to beef up the Server side.

I have always believed that the END USER experience should
be more important than how some single piece of software
'looks' on a benchmark test. Those benchmarks that produce
these holy TPS ratings are usually flawed when it comes
to imitating a REAL user-experience.

It's a classic argument and there have always been
2 camps...

Which is more important...

1. Having a minimal amount of Server to deal with/maintain
and let the users suffer more than they need to.

2. Do whatever it takes to make sure all the technology
that is currently available is being put into play to
provide the best USER experience possible.

I have always pitched my tent in camp # 2 and I think
most people that are serious about hosting Web sites
circle their wagons around the same camp.

> But overall I agree completely that compressing content and adding
> more ram and/or a faster

Re: [PATCH] mod_deflate extensions

[Jeff Trawick]

"Peter J. Cranstone" <[EMAIL PROTECTED]> writes:

> Since when does web server throughput drop by x% factor using
> mod_deflate?

I don't think you need me to explain the "why" or the "when" to you.

> We went through this debate with mod_gzip and it doesn't hold much
> water. Server boxes are cheap and adding some more ram or even a faster
> processor is a cheap price to pay when compared to customer satisfaction
> when their pages load faster.

Your "Server boxes are cheap" comment is very telling; if I add more
ram or a faster processor we aren't talking about the same web server.

But overall I agree completely that compressing content and adding
more ram and/or a faster processor as appropriate is the right thing
to do in many situations.

-- 
Jeff Trawick | [EMAIL PROTECTED]
Born in Roswell... married an alien...

Re: [PATCH] mod_deflate extensions

[Henri Gomez]

something like this in httpd.conf:

   JkWorkersFile /tomcat/conf/workers.properties

   JkExtractSSL On
   JkHTTPSIndicator HTTPS
   JkSESSIONIndicator SSL_SESSION_ID
   JkCIPHERIndicator SSL_CIPHER
   JkCERTSIndicator SSL_CLIENT_CERT


PS, there is no need to set Jk SSL env since there
are set to mod_ssl vars by default :

c->https_indicator  = "HTTPS";
c->certs_indicator  = "SSL_CLIENT_CERT";
c->cipher_indicator = "HTTPS_CIPHER";
c->cipher_indicator = "SSL_CIPHER";
c->session_indicator = "SSL_SESSION_ID";
c->key_size_indicator = "SSL_CIPHER_USEKEYSIZE";

Regards ;)

Re: [PATCH] mod_deflate extensions

[Henri Gomez]

Kris Verbeeck wrote:

Henri Gomez wrote:


BTW, I'll next check if mod_deflate works in conjunction with 
mod_jk/mod_jk2 (where I'm commiter).


It works, at least for me.



Good news :)

And it works for any JSP/Servlet contents send back from the servlet 
engine to the browser ?

I'll be happy to see your settings about it 


something like this in httpd.conf:

   JkWorkersFile /tomcat/conf/workers.properties

   JkExtractSSL On
   JkHTTPSIndicator HTTPS
   JkSESSIONIndicator SSL_SESSION_ID
   JkCIPHERIndicator SSL_CIPHER
   JkCERTSIndicator SSL_CLIENT_CERT

   JkMount /* ajp13

   Listen x.x.x.x:y
   NameVirtualHost x.x.x.x:y
   
   JkMount /* ajp13
   ServerName host.domain.tld
   
  AddOutputFilter DEFLATE html
   
   

Only tested with a servlet behind it.  No JSP testing, but shouldn't be
different I think.



Great, I check it ASAP :)

Re: [PATCH] mod_deflate extensions

[Henri Gomez]

Jeff Trawick wrote:

Henri Gomez <[EMAIL PROTECTED]> writes:



When you drop the network bandwith by 30 to 70% factor, you make your IT
managers happy since they save money and you make end-users very happy
since they feel you application is faster.



when you drop the web server throughput by x% factor you may make
yourself sad :)


What do you means by dropping the web server throughput by x% factor,
when you compress replies you send them quicker isn't it ?


So adding mod_deflate to the default distribution, under control of
configure which will verify if zlib is available on the system to enable
it, shouldn't hurt.



I wish it were so simple as finding a zlib, but static zlib
distributed with some OSs vs dynamic zlib distributed with others
seems to be the difference between success and failure.



More users will use mod_deflate, more chance to see remaining bugs
discovered and fixed.



I definitely agree that it would be nice to turn on mod_deflate in the
build automagically, I just don't want to do it at the expense of more
problems encountered by users.  I suspect that we would need to ship a
subset of zlib ourselves in order to have a fool-proof build of it.


If you recall mod_gzip included its own copy of zlib compression/expand
functions, and mod_deflate could do the same. May be APR friends could 
help us by making an APR facade to gzip/zlib  (as they do for DBM for 
example).

Nota that mod_gzip only use zlib system includes to rebuild its own 
compress code. Do you want me to send code to include zlib code this way 
in mod_deflate ?

Re: [PATCH] mod_deflate extensions

[Kris Verbeeck]

Henri Gomez wrote:


BTW, I'll next check if mod_deflate works in conjunction with 
mod_jk/mod_jk2 (where I'm commiter).

It works, at least for me.


Good news :)

And it works for any JSP/Servlet contents send back from the servlet 
engine to the browser ?

I'll be happy to see your settings about it 

something like this in httpd.conf:

   JkWorkersFile /tomcat/conf/workers.properties

   JkExtractSSL On
   JkHTTPSIndicator HTTPS
   JkSESSIONIndicator SSL_SESSION_ID
   JkCIPHERIndicator SSL_CIPHER
   JkCERTSIndicator SSL_CLIENT_CERT

   JkMount /* ajp13

   Listen x.x.x.x:y
   NameVirtualHost x.x.x.x:y
   
   JkMount /* ajp13
   ServerName host.domain.tld
   
  AddOutputFilter DEFLATE html
   
   

Only tested with a servlet behind it.  No JSP testing, but shouldn't be
different I think.

--
ir. Kris Verbeeck
Development Engineer

Ubizen - Ubicenter - Philipssite 5 - 3001 Leuven - Belgium
T:  +32 16 28 70 64
F:  +32 16 28 70 77

Ubizen - We Secure e-business - www.ubizen.com

RE: [PATCH] mod_deflate extensions

[Peter J. Cranstone]

Since when does web server throughput drop by x% factor using
mod_deflate?

We went through this debate with mod_gzip and it doesn't hold much
water. Server boxes are cheap and adding some more ram or even a faster
processor is a cheap price to pay when compared to customer satisfaction
when their pages load faster.

If mod_deflate is doing it's job correctly then compressing a page even
if it's a 100K should be in the microseconds. Transmitting 80% less data
will be reflected in the web server performance as it returns to work on
other connections.

Regards,

Peter J. Cranstone



-Original Message-
From: [EMAIL PROTECTED]
[mailto:[EMAIL PROTECTED]] On Behalf Of Jeff Trawick
Sent: Tuesday, November 19, 2002 5:33 AM
To: [EMAIL PROTECTED]
Subject: Re: [PATCH] mod_deflate extensions

Henri Gomez <[EMAIL PROTECTED]> writes:

> When you drop the network bandwith by 30 to 70% factor, you make your
IT
> managers happy since they save money and you make end-users very happy
> since they feel you application is faster.

when you drop the web server throughput by x% factor you may make
yourself sad :)

> So adding mod_deflate to the default distribution, under control of
> configure which will verify if zlib is available on the system to
enable
> it, shouldn't hurt.

I wish it were so simple as finding a zlib, but static zlib
distributed with some OSs vs dynamic zlib distributed with others
seems to be the difference between success and failure.

> More users will use mod_deflate, more chance to see remaining bugs
> discovered and fixed.

I definitely agree that it would be nice to turn on mod_deflate in the
build automagically, I just don't want to do it at the expense of more
problems encountered by users.  I suspect that we would need to ship a
subset of zlib ourselves in order to have a fool-proof build of it.

-- 
Jeff Trawick | [EMAIL PROTECTED]
Born in Roswell... married an alien...

RE: [PATCH] mod_deflate extensions

[Juan Rivera]

Title: RE: [PATCH] mod_deflate extensions





I agree that mod_deflate should be part of the distribution.


Are there any issues with apache including zlib in the distribution?




Best regards,


Juan C. Rivera
Citrix Systems, Inc.



-Original Message-
From: Jeff Trawick [mailto:[EMAIL PROTECTED]] 
Sent: Tuesday, November 19, 2002 7:33 AM
To: [EMAIL PROTECTED]
Subject: Re: [PATCH] mod_deflate extensions


Henri Gomez <[EMAIL PROTECTED]> writes:


> When you drop the network bandwith by 30 to 70% factor, you make your IT
> managers happy since they save money and you make end-users very happy
> since they feel you application is faster.


when you drop the web server throughput by x% factor you may make
yourself sad :)


> So adding mod_deflate to the default distribution, under control of
> configure which will verify if zlib is available on the system to enable
> it, shouldn't hurt.


I wish it were so simple as finding a zlib, but static zlib
distributed with some OSs vs dynamic zlib distributed with others
seems to be the difference between success and failure.


> More users will use mod_deflate, more chance to see remaining bugs
> discovered and fixed.


I definitely agree that it would be nice to turn on mod_deflate in the
build automagically, I just don't want to do it at the expense of more
problems encountered by users.  I suspect that we would need to ship a
subset of zlib ourselves in order to have a fool-proof build of it.


-- 
Jeff Trawick | [EMAIL PROTECTED]
Born in Roswell... married an alien...

Re: [PATCH] mod_deflate extensions

[Jeff Trawick]

Henri Gomez <[EMAIL PROTECTED]> writes:

> When you drop the network bandwith by 30 to 70% factor, you make your IT
> managers happy since they save money and you make end-users very happy
> since they feel you application is faster.

when you drop the web server throughput by x% factor you may make
yourself sad :)

> So adding mod_deflate to the default distribution, under control of
> configure which will verify if zlib is available on the system to enable
> it, shouldn't hurt.

I wish it were so simple as finding a zlib, but static zlib
distributed with some OSs vs dynamic zlib distributed with others
seems to be the difference between success and failure.

> More users will use mod_deflate, more chance to see remaining bugs
> discovered and fixed.

I definitely agree that it would be nice to turn on mod_deflate in the
build automagically, I just don't want to do it at the expense of more
problems encountered by users.  I suspect that we would need to ship a
subset of zlib ourselves in order to have a fool-proof build of it.

-- 
Jeff Trawick | [EMAIL PROTECTED]
Born in Roswell... married an alien...

Re: [PATCH] mod_deflate extensions

[Henri Gomez]

William A. Rowe, Jr. wrote:
> At 02:42 AM 11/11/2002, Henri Gomez wrote:
>
>>Justin Erenkrantz wrote:
>>
>>>--On Friday, November 8, 2002 5:52 PM +0100 Henri Gomez 
<[EMAIL PROTECTED]> wrote:
>>>
>>>
Some questions about mod_deflate :

1) Why this module is not enabled by default built ?
  compression should be on to meet HTTP recommandations ?
>>>
>>>No, that's not part of the HTTP specification per se.
>>
>>I didn't agree, they are covered in rfc2616 :
>>
>>http://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html#sec14.11
>
>
> What Justin TRIED to say is that rfc2616 neither recommends nor requires
> gzip compression.  It simply defines the transmission of 
transfer-encoding
> and content-encoding headers and defines the gzip method amoungst
> others.

Agreed

> Justin disagreed (and I concur) that there is no such thing as meeting
> some "HTTP recommandations" from RFC 2616, since the RFC makes
> no such recommendation.

OK

> However, I agree with you that we should make it trivial for the admin
> to enable this module.

When you drop the network bandwith by 30 to 70% factor, you make your IT
managers happy since they save money and you make end-users very happy
since they feel you application is faster.

So adding mod_deflate to the default distribution, under control of
configure which will verify if zlib is available on the system to enable
it, shouldn't hurt.

More users will use mod_deflate, more chance to see remaining bugs
discovered and fixed.

Regards

Re: [PATCH] mod_deflate extensions

[Henri Gomez]

Kris Verbeeck wrote:

Henri Gomez wrote:


BTW, I'll next check if mod_deflate works in conjunction with 
mod_jk/mod_jk2 (where I'm commiter).


It works, at least for me.


Good news :)

And it works for any JSP/Servlet contents send back from the servlet 
engine to the browser ?

I'll be happy to see your settings about it 

Thanks

Re: [PATCH] mod_deflate extensions

[Kris Verbeeck]

Henri Gomez wrote:


BTW, I'll next check if mod_deflate works in conjunction with 
mod_jk/mod_jk2 (where I'm commiter).

It works, at least for me.

--
ir. Kris Verbeeck
Development Engineer

Ubizen - Ubicenter - Philipssite 5 - 3001 Leuven - Belgium
T:  +32 16 28 70 64
F:  +32 16 28 70 77

Ubizen - We Secure e-business - www.ubizen.com

Re: [PATCH] mod_deflate extensions

[William A. Rowe, Jr.]

At 02:42 AM 11/11/2002, Henri Gomez wrote:
>Justin Erenkrantz wrote:
>>--On Friday, November 8, 2002 5:52 PM +0100 Henri Gomez <[EMAIL PROTECTED]> wrote:
>>
>>>Some questions about mod_deflate :
>>>
>>>1) Why this module is not enabled by default built ?
>>>   compression should be on to meet HTTP recommandations ?
>>
>>No, that's not part of the HTTP specification per se.
>
>I didn't agree, they are covered in rfc2616 :
>
>http://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html#sec14.11

What Justin TRIED to say is that rfc2616 neither recommends nor requires
gzip compression.  It simply defines the transmission of transfer-encoding
and content-encoding headers and defines the gzip method amoungst
others.

Justin disagreed (and I concur) that there is no such thing as meeting
some "HTTP recommandations" from RFC 2616, since the RFC makes
no such recommendation.

However, I agree with you that we should make it trivial for the admin 
to enable this module.

Bill

Re: [PATCH] mod_deflate extensions

[Henri Gomez]

Justin Erenkrantz wrote:

--On Friday, November 8, 2002 5:52 PM +0100 Henri Gomez 
<[EMAIL PROTECTED]> wrote:

Some questions about mod_deflate :

1) Why this module is not enabled by default built ?
   compression should be on to meet HTTP recommandations ?



No, that's not part of the HTTP specification per se.


I didn't agree, they are covered in rfc2616 :

http://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html#sec14.11


2) What about a more advanced system ?

   Currently :

SetEnv gzip-only-text/html 1

-> make mod_deflate compress only text/html.



You should take a look at the AddOutputFilterByType and AddOutputFilter 
directives.  Those allow the same functionality as your patch without 
introducing any redundant directives.  -- justin

Ok, I'll take a look, thanks.

BTW, I'll next check if mod_deflate works in conjunction with 
mod_jk/mod_jk2 (where I'm commiter).

Re: [PATCH] mod_deflate extensions

[Henri Gomez]

Dietz, Phil E. wrote:

On a side note...

I think there should be a SetNote command so admins can tweak module settings through (instead of SetEnv like below)...

Using SetEnv exposes the result to cgi applications...which is not always a good thing.


I followed the current mod_deflate behaviour which use allready the
SetEnv gzip-only-text/html 1


-Original Message-
From:	Henri Gomez [SMTP:[EMAIL PROTECTED]]
Sent:	Friday, November 08, 2002 10:49 AM
To:	[EMAIL PROTECTED]
Subject:	[PATCH] mod_deflate extensions

Some questions about mod_deflate :

1) Why this module is not enabled by default built ?
 compression should be on to meet HTTP recommandations ?

2) What about a more advanced system ?

 Currently :

SetEnv gzip-only-text/html 1

-> make mod_deflate compress only text/html.

 Extension :

SetEnv gzip-mime-/ allow

-> make mod_deflate compress content matching /.

SetEnv gzip-mime-/  deny

-> make mod_deflate don't compress content matching /.

SetEnv gzip-file-jsp allow

-> make mod_deflate compress content caming from jsp

SetEnv gzip-file-js deny

-> make mod_deflate don't compress content caming for javascript


SetEnv gzip-mime-text/html allow
SetEnv gzip-mime-text/xml allow
SetEnv gzip-mime-text/plain allow
SetEnv gzip-mime-default deny
SetEnv gzip-file-jsp allow
SetEnv gzip-file-js deny
SetEnv gzip-file-default deny
SetOutputFilter DEFLATE



Attached is a patch ...

Thanks for your feedback


	

<< File: mod_deflate.c.diff >> 

Re: [PATCH] mod_deflate extensions

[Jeff Trawick]

Henri Gomez <[EMAIL PROTECTED]> writes:

> Some questions about mod_deflate :
> 
> 1) Why this module is not enabled by default built ?
>   compression should be on to meet HTTP recommandations ?

asking for trouble w.r.t. the build unless we package the zlib code
ourselves and built it ourself in some consistent way

--enable-deflate=shared simply does not work across a large number of
systems

-- 
Jeff Trawick | [EMAIL PROTECTED]
Born in Roswell... married an alien...

RE: [PATCH] mod_deflate extensions

[Dietz, Phil E.]

On a side note...

I think there should be a SetNote command so admins can tweak module settings through 
(instead of SetEnv like below)...

Using SetEnv exposes the result to cgi applications...which is not always a good thing.

> -Original Message-
> From: Henri Gomez [SMTP:[EMAIL PROTECTED]]
> Sent: Friday, November 08, 2002 10:49 AM
> To:   [EMAIL PROTECTED]
> Subject:  [PATCH] mod_deflate extensions
> 
> Some questions about mod_deflate :
> 
> 1) Why this module is not enabled by default built ?
>   compression should be on to meet HTTP recommandations ?
> 
> 2) What about a more advanced system ?
> 
>   Currently :
> 
> SetEnv gzip-only-text/html 1
> 
> -> make mod_deflate compress only text/html.
> 
>   Extension :
> 
> SetEnv gzip-mime-/ allow
> 
> -> make mod_deflate compress content matching /.
> 
> SetEnv gzip-mime-/  deny
> 
> -> make mod_deflate don't compress content matching /.
> 
> SetEnv gzip-file-jsp allow
> 
> -> make mod_deflate compress content caming from jsp
> 
> SetEnv gzip-file-js deny
> 
> -> make mod_deflate don't compress content caming for javascript
> 
> 
> SetEnv gzip-mime-text/html allow
> SetEnv gzip-mime-text/xml allow
> SetEnv gzip-mime-text/plain allow
> SetEnv gzip-mime-default deny
> SetEnv gzip-file-jsp allow
> SetEnv gzip-file-js deny
> SetEnv gzip-file-default deny
> SetOutputFilter DEFLATE
> 
> 
> 
> Attached is a patch ...
> 
> Thanks for your feedback
> 
> 
>   
> 
>  << File: mod_deflate.c.diff >> 

Re: [PATCH] mod_deflate extensions

[Justin Erenkrantz]

--On Friday, November 8, 2002 5:52 PM +0100 Henri Gomez 
<[EMAIL PROTECTED]> wrote:

Some questions about mod_deflate :

1) Why this module is not enabled by default built ?
   compression should be on to meet HTTP recommandations ?


No, that's not part of the HTTP specification per se.


2) What about a more advanced system ?

   Currently :

SetEnv gzip-only-text/html 1

-> make mod_deflate compress only text/html.


You should take a look at the AddOutputFilterByType and 
AddOutputFilter directives.  Those allow the same functionality as 
your patch without introducing any redundant directives.  -- justin

Re: [PATCH] mod_deflate with a static libz

[William A. Rowe, Jr.]

At 09:23 AM 10/17/2002, Jeff Trawick wrote:
>Another little note:
>
>A user of my mod_deflate build encountered a problem loading
>mod_deflate alongside another module loaded another library which also
>had a symbol called crc32 and the wrong one was used.  So to my apxs
>command I add
>
>  -Wc,-Dcrc32=deflate_crc32
>
>(I should probably change that to zlib_crc32.)
>
>It would be smart to identify other potential conflicts, or add
>support to zlib for reasonable namespace protection.

That's extremely simple... -Wc,-DZ_PREFIX

All exports will be z_ prefixed.  They ask that stock, 'standard'
libz installed into /usr be compiled without it.  It's probably something
that we could discover from AC if we were clever.

Bill

Re: [PATCH] mod_deflate with a static libz

[Jeff Trawick]

"William A. Rowe, Jr." <[EMAIL PROTECTED]> writes:

> The follow patch will allow binds to a static zlib, but it's not entirely portable.

This isn't what you were going for exactly, but it works for me on
HP-UX, Linux, AIX, and Solaris:

  apxs -c mod_deflate.c zlib1.c zlib2.c zlib3.c etc.

(replace zlib*.c with the proper zlib source filenames)

Another little note:

A user of my mod_deflate build encountered a problem loading
mod_deflate alongside another module loaded another library which also
had a symbol called crc32 and the wrong one was used.  So to my apxs
command I add

  -Wc,-Dcrc32=deflate_crc32

(I should probably change that to zlib_crc32.)

It would be smart to identify other potential conflicts, or add
support to zlib for reasonable namespace protection.

-- 
Jeff Trawick | [EMAIL PROTECTED]
Born in Roswell... married an alien...

Re: [PATCH] mod_deflate 2.0.40 logic error

[Brian Pane]

Thanks.  There was a fix committed for this a couple of weeks
ago, so 2.0.41 will have the right logic.

Brian

Spinka, Kristofer wrote:

>Description:
>
>  The Apache environment variable "gzip-only-text/html" was designed to
>allow control over whether non-text/html content will be compressed with
>the mod_deflate filter.  However, there is a small logic error in the
>mod_deflate.c code that prevents this from functioning properly.
>
>
>Resolution:
>---
>  There is an "if" statement that contains a call to the function strcmp,
>which compares the value of the environment variable "gzip-only-text/html"
>to the character string "1".  However, strcmp returns 0 on success, which
>in this case will cause an undesired decision in this "if" statement.
>  In addition, the ap_pass_brigade was "mispositioned", causing an
>additional failure of this logic set; this is also corrected.
>
>
>Patch:
>--
>--- orig/httpd-2.0.40/modules/filters/mod_deflate.c Wed Aug  7
>11:26:17 2002
>+++ httpd-2.0.40/modules/filters/mod_deflate.c  Tue Sep 10 23:09:15 2002
>@@ -281,10 +281,10 @@
>  || strncmp(r->content_type, "text/html", 9)) {
> const char *env_value = apr_table_get(r->subprocess_env,
>   "gzip-only-text/html");
>-if ( env_value == NULL || strcmp(env_value,"1") ) {
>+if ( env_value == NULL || strncmp(env_value,"1",1) == 0 ) {
> ap_remove_output_filter(f);
>+return ap_pass_brigade(f->next, bb);
> }
>-return ap_pass_brigade(f->next, bb);
> }
>
> /* Let's see what our current Content-Encoding is.
>--- end patch
>
>/kris
>
>  
>

Re: [PATCH] mod_deflate

[Igor Sysoev]

On Sun, 17 Feb 2002, Graham Leggett wrote:

> Igor Sysoev wrote:
> 
> > The main problem is proxies, especially Squid (~70% of all proxies)
> > Proxies can store compressed response and return it to browser
> > that does not understand gzipped content.
> 
> Is this verified behavior? If a proxy returns compressed content to a
> browser that cannot handle it, then the content negotiation function
> inside the proxy is broken - and as squid has an active developer
> community, I seriously doubt that a bug this serious would go unfixed.

Yes, it is verified behavior. Squid can return cached compressed content
to client that does not send "Accept-Encoding: gzip". As far as I
know MSProxy 2.0 does the same.

> RFC2616 describes the "Vary" header, which helps determine on what basis
> a document was negotiated. mod_deflate should use content negotiation
> and the presence of the Vary header to determine what to do, as is laid
> down in the HTTP spec.
>
> > So you should by default disable encoding for requests
> > with "Via" header and HTTP/1.0 requests (HTTP/1.1-compatible
> > proxy must set "Via" header, HTTP/1.0-compatible should but not have).
> 
> I disagree. Virtually all content is going to go through a proxy of some
> kind before reaching a browser. Doing this will effective render
> mod_deflate useless.
>
> mod_deflate should behave according to RFC2616 - and you won't have
> problems.

Using "Vary" does not resolve the problem completetly.
"Vary" defined for HTTP/1.1 but we need to work with HTTP/1.0
requests also. Yes, Squid understand "Vary" and does not cache such
response at all (at least Squid 1.2.x-2.4.x). But MSIE 4.x does not cache
documents with "Vary" too. I don't know about later MSIE version -
I will investigate it.

Yes, compressing HTTP/1.1 and non-proxied requests only is not such
effective as compressing any request with "Accept-Encoding: gzip".
But any web master can choose is he intrested in old clients or not.

About efficiency - my mod_deflate module with conservative settings
(HTTP/1.1, non-proxied requests) save up to 8M/s bandwidth for
www.rambler.ru and search.rambler.ru.

Igor Sysoev

Re: [PATCH] mod_deflate

[Graham Leggett]

Igor Sysoev wrote:

> The main problem is proxies, especially Squid (~70% of all proxies)
> Proxies can store compressed response and return it to browser
> that does not understand gzipped content.

Is this verified behavior? If a proxy returns compressed content to a
browser that cannot handle it, then the content negotiation function
inside the proxy is broken - and as squid has an active developer
community, I seriously doubt that a bug this serious would go unfixed.

RFC2616 describes the "Vary" header, which helps determine on what basis
a document was negotiated. mod_deflate should use content negotiation
and the presence of the Vary header to determine what to do, as is laid
down in the HTTP spec.

> So you should by default disable encoding for requests
> with "Via" header and HTTP/1.0 requests (HTTP/1.1-compatible
> proxy must set "Via" header, HTTP/1.0-compatible should but not have).

I disagree. Virtually all content is going to go through a proxy of some
kind before reaching a browser. Doing this will effective render
mod_deflate useless.

mod_deflate should behave according to RFC2616 - and you won't have
problems.

Regards,
Graham
-- 
-
[EMAIL PROTECTED]"There's a moon
over Bourbon Street
tonight..."


smime.p7s
Description: S/MIME Cryptographic Signature

RE: [PATCH] mod_deflate

[Sander Striker]

> From: [EMAIL PROTECTED]
> Sent: 16 February 2002 22:53

> Justin Erenkrantz <[EMAIL PROTECTED]> writes:
> 
> > On Sat, Feb 16, 2002 at 06:59:40PM +0100, Sander Striker wrote:
> > > > Wow!  Obviously the code/default config need to be extremely
> > > > conservative!
> > > 
> > > Yes.  But browsers change (evolve to better things we hope), so config has
> > > my preference.  Hardcoding in default rules is badness IMHO.  But maybe that's
> > > just me.
> > 
> > -1 if these restrictions are hardcoded in the module like it was
> > before Sander's patch.  These problems should be handled by the
> > configuration of mod_deflate not by hardcoding rules.
> 
> I guess I don't follow this about restrictions being hardcoded in the
> module.  When you say "restriction" I just see default behavior.  A
> default behavior is hardcoded into Apache or modules for so many
> things.
> 
> I wonder what the real disagreement is...  It is hard to tell exactly
> who wants what.  It looks like there is violent agreement in some
> areas.
> 
> Are these the choices so far?
> 
> .. mod_deflate shoud stay as it was (only compress text/html, can't
>   add more mime types
> 
>   (I don't see that anybody was in favor of this.)

Me neither.
 
> .. mod_deflate defaults to only compressing text/html (and may perhaps
>   have further restrictions in the interest of been fool-proof) in
>   case the admin takes the bare minimum steps to enable it, but a
>   directive will be available so that admins can turn on compression
>   for more mime types
> 
>   and I guess there are subchoices here on whether the default
>   set of directives in httpd.conf would be very conservative or would
>   simply turn on compression for everything we think is safe until we
>   find out from admins that some browser doesn't do this or that
>   correctly

This is what this little disagreement seems to be about.

> It would be helpful to some of us if, forgetting the vetos for a
> moment, people could put a concise wording of their position in the
> STATUS file and allow folks to indicate how they feel about it.  (Can
> you tell that I can't keep everything straight?)

Ok, I can't commit to STATUS, but I can tell you my take on things:

SetOutputFilter DEFLATE, should compress all by default.  There is an
exception which is 'gzip-only-text/html' which can be set on browser
matches.  We do _not_ put a SetOutputFilter DEFLATE line in the std
config.

In the std config we should have:


AddOutputFilter DEFLATE text/html
...



We would need to adjust AddOutputFilter for this so it can add filters
on content type aswell as on extension.

Now, if you wish to enable compression for everything, like for a
subversion repository, you would do:


SetOutputFilter DEFLATE
...




Sander

Re: [PATCH] mod_deflate

[Jeff Trawick]

Justin Erenkrantz <[EMAIL PROTECTED]> writes:

> On Sat, Feb 16, 2002 at 06:59:40PM +0100, Sander Striker wrote:
> > > Wow!  Obviously the code/default config need to be extremely
> > > conservative!
> > 
> > Yes.  But browsers change (evolve to better things we hope), so config has
> > my preference.  Hardcoding in default rules is badness IMHO.  But maybe that's
> > just me.
> 
> -1 if these restrictions are hardcoded in the module like it was
> before Sander's patch.  These problems should be handled by the
> configuration of mod_deflate not by hardcoding rules.

I guess I don't follow this about restrictions being hardcoded in the
module.  When you say "restriction" I just see default behavior.  A
default behavior is hardcoded into Apache or modules for so many
things.

I wonder what the real disagreement is...  It is hard to tell exactly
who wants what.  It looks like there is violent agreement in some
areas.

Are these the choices so far?

. mod_deflate shoud stay as it was (only compress text/html, can't
  add more mime types

  (I don't see that anybody was in favor of this.)

. mod_deflate defaults to only compressing text/html (and may perhaps
  have further restrictions in the interest of been fool-proof) in
  case the admin takes the bare minimum steps to enable it, but a
  directive will be available so that admins can turn on compression
  for more mime types

  and I guess there are subchoices here on whether the default
  set of directives in httpd.conf would be very conservative or would
  simply turn on compression for everything we think is safe until we
  find out from admins that some browser doesn't do this or that
  correctly

It would be helpful to some of us if, forgetting the vetos for a
moment, people could put a concise wording of their position in the
STATUS file and allow folks to indicate how they feel about it.  (Can
you tell that I can't keep everything straight?)

-- 
Jeff Trawick | [EMAIL PROTECTED] | PGP public key at web site:
   http://www.geocities.com/SiliconValley/Park/9289/
 Born in Roswell... married an alien...

Re: [PATCH] mod_deflate

[Igor Sysoev]

On Sat, 16 Feb 2002, Ian Holsman wrote:

> Justin Erenkrantz wrote:
> > On Sat, Feb 16, 2002 at 06:59:40PM +0100, Sander Striker wrote:
> > 
> >>>Wow!  Obviously the code/default config need to be extremely
> >>>conservative!
> >>>
> >>Yes.  But browsers change (evolve to better things we hope), so config has
> >>my preference.  Hardcoding in default rules is badness IMHO.  But maybe that's
> >>just me.
> >>
> > 
> > -1 if these restrictions are hardcoded in the module like it was
> > before Sander's patch.  These problems should be handled by the
> > configuration of mod_deflate not by hardcoding rules.
> 
> this is BULLSHIT justin.
> you can't veto a change to make it behave like the old (more 
> conservative) behavior.
> GZIP encoding it VERY badly broken in all of the common browsers
> and saying 'well fix the browsers' isn't going to cut it. for a couple 
> of reasons
> 1. apache 2 has 0% market share
> 2. browsers arent going to get fixed just because we want them to
> 3. people are still using netscape 3.x out there, people will be using
> these broken browsers for a VERY long time.

The main problem is not old browsers.
They do not send "Accept-Encoding: gzip" header so
they do not receive compressed content.
I know two browsers that send "Accept-Encoding" and
can incorrectly handle compressed response - MSIE 4.x
and Mozilla 0.9.1.

The main problem is proxies, especially Squid (~70% of all proxies)
Proxies can store compressed response and return it to browser
that does not understand gzipped content.

So you should by default disable encoding for requests
with "Via" header and HTTP/1.0 requests (HTTP/1.1-compatible 
proxy must set "Via" header, HTTP/1.0-compatible should but not have).
 
Igor Sysoev

Re: [PATCH] mod_deflate

[Ian Holsman]

Justin Erenkrantz wrote:
> On Sat, Feb 16, 2002 at 06:59:40PM +0100, Sander Striker wrote:
> 
>>>Wow!  Obviously the code/default config need to be extremely
>>>conservative!
>>>
>>Yes.  But browsers change (evolve to better things we hope), so config has
>>my preference.  Hardcoding in default rules is badness IMHO.  But maybe that's
>>just me.
>>
> 
> -1 if these restrictions are hardcoded in the module like it was
> before Sander's patch.  These problems should be handled by the
> configuration of mod_deflate not by hardcoding rules.

this is BULLSHIT justin.
you can't veto a change to make it behave like the old (more 
conservative) behavior.
GZIP encoding it VERY badly broken in all of the common browsers
and saying 'well fix the browsers' isn't going to cut it. for a couple 
of reasons
1. apache 2 has 0% market share
2. browsers arent going to get fixed just because we want them to
3. people are still using netscape 3.x out there, people will be using
these broken browsers for a VERY long time.
4. with sander's 2 patches we are now forcing everybody running a server
to suffer. I'm still not sure how you will be able to serve normal
pages on the same server as SVN, without having to resort to some
kind of trickery. I don't with the current method of browsermatch it
will work.

I like Igor's approach.
we should be creating a directive/environment variable for SVN's case 
and forcing them to configure it, not the other way around.


> 
> When we added mod_deflate to the tree, we knew that we would have to
> fine tune its configuration syntax to compensate for bad browsers.
> Igor's listing is an invaluable first step.  -- justin
> 
since when.
in the discussions I always said 'text/html' only as browsers were way 
too buggy to handle anything else. I didn't just go add the check 
because it looked nice. most of the RC's 1.3 gzip's problems where due 
to people trying to compress too much and the browsers busting all the 
time. the text/html was a concisous choice only to do a 80/20 rule. 80% 
of the compression savings with 20% of the effort.

..Ian


> 

Re: [PATCH] mod_deflate

[Justin Erenkrantz]

On Sat, Feb 16, 2002 at 06:59:40PM +0100, Sander Striker wrote:
> > Wow!  Obviously the code/default config need to be extremely
> > conservative!
> 
> Yes.  But browsers change (evolve to better things we hope), so config has
> my preference.  Hardcoding in default rules is badness IMHO.  But maybe that's
> just me.

-1 if these restrictions are hardcoded in the module like it was
before Sander's patch.  These problems should be handled by the
configuration of mod_deflate not by hardcoding rules.

When we added mod_deflate to the tree, we knew that we would have to
fine tune its configuration syntax to compensate for bad browsers.
Igor's listing is an invaluable first step.  -- justin

Re: [PATCH] mod_deflate

[Igor Sysoev]

On Sat, 16 Feb 2002, Eli Marmor wrote:

> Igor Sysoev wrote:
> > 
> > On Sat, 16 Feb 2002, Zvi Har'El wrote:
> > 
> > ...
> > 
> > In my mod_deflate module (for Apache 1.3.x) I'd enabled by default
> > "text/html" only. You can add or remove another type with DeflateTypes
> > directive. Here are some recomendations:
> > 
> > application/x-javascript   NN4 does not understand it compressed.
> > text/css   the same.
> > 
> > text/plain   Macromedia FlashPlayer 4.x-5.x does not understand it
> >  compressed when get it with loadVariables() function via browser.
> > text/xml Macromedia FlashPlayer 5.x does not understand it
> >  compressed when get it with XML.load() function via browser.
> > 
> > application/x-shockwave-flash   FlashPlayer plugin for NN4 for Windows
> >  does not understand it compressed. Although plugin for Linux
> >  NN4 work correctly.
> > 
> > text/rtf   MSIE 4.x-6.x understand correctly them
> > application/msword when compressed. NN and Opera does not.
> > application/vnd.ms-excel
> > application/vnd.ms-powerpoint
> 
> I want to add that these issues (what to compress and what to leave as-
> is), were discussed very deeply and heavilly in the mod_gzip list.
> 
> If we don't adopt mod_gzip but develop our own mod_deflate (both are
> good, by the way), we should at least use the long experience that
> mod_gzip has had.
> 
> After being used in so many installations, and even being included in
> leading Linux distros, there is almost no combination of format/browser
> that has not been tested yet.
> 
> Your research, Igor, is very helpful (and Zvi's as well), but we can
> base more default definitions on the defaults (or conclusions) of
> mod_gzip.

By default my mod_deflate compresses content if:
1. mime type is text/html;
2. request is not proxied;
3. request is HTTP/1.1;

As far as I know mod_gzip use only first rule by default.
My rules are safer.
Also mod_gzip has not workaround with broken browaers.

> The list of default definitions may become quite long, but putting it
> inside an IfModule section, which separates it from the other parts of
> httpd.conf, may help. I believe that the improvement in bandwidth,
> deserves the price in size of httpd.conf.

Igor Sysoev

RE: [PATCH] mod_deflate

[Sander Striker]

> From: [EMAIL PROTECTED]
> [mailto:[EMAIL PROTECTED]]On Behalf Of Jeff Trawick

> Ian Holsman <[EMAIL PROTECTED]> writes:
> 
> > > In my mod_deflate module (for Apache 1.3.x) I'd enabled by default
> > > "text/html" only. You can add or remove another type with DeflateTypes
> > > directive. Here are some recomendations:
> > >
> > 
> > This is EXACTLY what we should be doing IMHO.
> > if a person wants to run a SVN server they can add a directive
> > to deflate everything, otherwise it should just do text/html out of the
> > box. we want to make it easy for the 'general' person to use.
> 
> +1
> 
> > > application/x-javascript   NN4 does not understand it compressed.
> > > text/css   the same.
> > > text/plain   Macromedia FlashPlayer 4.x-5.x does not understand it
> > >  compressed when get it with loadVariables() function via browser.
> > > text/xml Macromedia FlashPlayer 5.x does not understand it
> > >  compressed when get it with XML.load() function via browser.
> > > application/x-shockwave-flash   FlashPlayer plugin for NN4 for
> > > Windows
> > >  does not understand it compressed. Although plugin for
> > > LinuxNN4 work correctly.
> > > text/rtf   MSIE 4.x-6.x understand correctly them
> > > application/msword when compressed. NN and Opera does not.
> > > application/vnd.ms-excel
> > > application/vnd.ms-powerpoint
> > > Igor Sysoev
> 
> Wow!  Obviously the code/default config need to be extremely
> conservative!

Yes.  But browsers change (evolve to better things we hope), so config has
my preference.  Hardcoding in default rules is badness IMHO.  But maybe that's
just me.


Sander

Re: [PATCH] mod_deflate

[Jeff Trawick]

Ian Holsman <[EMAIL PROTECTED]> writes:

> > In my mod_deflate module (for Apache 1.3.x) I'd enabled by default
> > "text/html" only. You can add or remove another type with DeflateTypes
> > directive. Here are some recomendations:
> >
> 
> This is EXACTLY what we should be doing IMHO.
> if a person wants to run a SVN server they can add a directive
> to deflate everything, otherwise it should just do text/html out of the
> box. we want to make it easy for the 'general' person to use.

+1

> > application/x-javascript   NN4 does not understand it compressed.
> > text/css   the same.
> > text/plain   Macromedia FlashPlayer 4.x-5.x does not understand it
> >  compressed when get it with loadVariables() function via browser.
> > text/xml Macromedia FlashPlayer 5.x does not understand it
> >  compressed when get it with XML.load() function via browser.
> > application/x-shockwave-flash   FlashPlayer plugin for NN4 for
> > Windows
> >  does not understand it compressed. Although plugin for
> > LinuxNN4 work correctly.
> > text/rtf   MSIE 4.x-6.x understand correctly them
> > application/msword when compressed. NN and Opera does not.
> > application/vnd.ms-excel
> > application/vnd.ms-powerpoint
> > Igor Sysoev

Wow!  Obviously the code/default config need to be extremely
conservative!

-- 
Jeff Trawick | [EMAIL PROTECTED] | PGP public key at web site:
   http://www.geocities.com/SiliconValley/Park/9289/
 Born in Roswell... married an alien...

Re: [PATCH] mod_deflate

[Eli Marmor]

Igor Sysoev wrote:
> 
> On Sat, 16 Feb 2002, Zvi Har'El wrote:
> 
> ...
> 
> In my mod_deflate module (for Apache 1.3.x) I'd enabled by default
> "text/html" only. You can add or remove another type with DeflateTypes
> directive. Here are some recomendations:
> 
> application/x-javascript   NN4 does not understand it compressed.
> text/css   the same.
> 
> text/plain   Macromedia FlashPlayer 4.x-5.x does not understand it
>  compressed when get it with loadVariables() function via browser.
> text/xml Macromedia FlashPlayer 5.x does not understand it
>  compressed when get it with XML.load() function via browser.
> 
> application/x-shockwave-flash   FlashPlayer plugin for NN4 for Windows
>  does not understand it compressed. Although plugin for Linux
>  NN4 work correctly.
> 
> text/rtf   MSIE 4.x-6.x understand correctly them
> application/msword when compressed. NN and Opera does not.
> application/vnd.ms-excel
> application/vnd.ms-powerpoint

I want to add that these issues (what to compress and what to leave as-
is), were discussed very deeply and heavilly in the mod_gzip list.

If we don't adopt mod_gzip but develop our own mod_deflate (both are
good, by the way), we should at least use the long experience that
mod_gzip has had.

After being used in so many installations, and even being included in
leading Linux distros, there is almost no combination of format/browser
that has not been tested yet.

Your research, Igor, is very helpful (and Zvi's as well), but we can
base more default definitions on the defaults (or conclusions) of
mod_gzip.

The list of default definitions may become quite long, but putting it
inside an IfModule section, which separates it from the other parts of
httpd.conf, may help. I believe that the improvement in bandwidth,
deserves the price in size of httpd.conf.

Just my 2C...

-- 
Eli Marmor
[EMAIL PROTECTED]
CTO, Founder
Netmask (El-Mar) Internet Technologies Ltd.
__
Tel.:   +972-9-766-1020  8 Yad-Harutzim St.
Fax.:   +972-9-766-1314  P.O.B. 7004
Mobile: +972-50-23-7338  Kfar-Saba 44641, Israel

Re: [PATCH] mod_deflate

[Ian Holsman]

Igor Sysoev wrote:
> On Sat, 16 Feb 2002, Zvi Har'El wrote:
> 
> 
>>On Fri, 15 Feb 2002 09:44:19 -0800, Ian Holsman wrote about "Re: [PATCH] 
>mod_deflate":
>>
>>>
>>>I'm still not very happy about compressing EVERYTHING and excluding
>>>certain browsers
>>>as you would have to exclude IE & Netscape.
>>>
>>>so
>>>this is a
>>>-1 for this patch.
>>>in order to change this checks need to be there with a directive to
>>>ignore them (default:off)
>>>
>>>
>>IMHO, deflating everything is a waste of the computer resources. HTML files
>>really compress well. But most of the image files currently supported, e.g.,
>>PNG, GIF, JPEG are already compressed, and deflating them will really do
>>nothing -- just spend your CPU. I think that compressing text/html for browsers
>>who send "Accept-Encoding: gzip" is the right approach. A possible enhancement
>>is to have a directive (DeflateAddMimeType) which will enable deflating more
>>mime types, e.g., text/plain, but these are really rare! Another type which is
>>worth compressing is application/postscript, since many viewers (I am not an
>>expert which - at least those decendents of GhostScript) are capable of
>>viewing gzipped postscript files. The problem with that is that this is not a
>>function of the browser, which cannot handle such files, but a function of the
>>viewer, so the required "Accept-Encoding: gzip" doesn't prove anything about
>>the ability of the external viewer!
>>
>>To summerize, I suggest to deflate only types which can be handled by the
>>browser itself, and which are not already compressed, which amounts to
>>text/html or more generally text/* (text/css for instance).
>>
> 
> In my mod_deflate module (for Apache 1.3.x) I'd enabled by default
> "text/html" only. You can add or remove another type with DeflateTypes
> directive. Here are some recomendations:
> 

This is EXACTLY what we should be doing IMHO.
if a person wants to run a SVN server they can add a directive
to deflate everything, otherwise it should just do text/html out of the
box. we want to make it easy for the 'general' person to use.



> application/x-javascript   NN4 does not understand it compressed.
> text/css   the same.
> 
> text/plain   Macromedia FlashPlayer 4.x-5.x does not understand it
>  compressed when get it with loadVariables() function via browser.
> text/xml Macromedia FlashPlayer 5.x does not understand it
>  compressed when get it with XML.load() function via browser.
> 
> application/x-shockwave-flash   FlashPlayer plugin for NN4 for Windows
>  does not understand it compressed. Although plugin for Linux   
>  NN4 work correctly.
> 
> text/rtf   MSIE 4.x-6.x understand correctly them
> application/msword when compressed. NN and Opera does not.
> application/vnd.ms-excel
> application/vnd.ms-powerpoint
> 
> Igor Sysoev
> 
> 

Re: [PATCH] mod_deflate

[Igor Sysoev]

On Sat, 16 Feb 2002, Zvi Har'El wrote:

> On Fri, 15 Feb 2002 09:44:19 -0800, Ian Holsman wrote about "Re: [PATCH] 
>mod_deflate":
> > 
> > 
> > I'm still not very happy about compressing EVERYTHING and excluding
> > certain browsers
> > as you would have to exclude IE & Netscape.
> > 
> > so
> > this is a
> > -1 for this patch.
> > in order to change this checks need to be there with a directive to
> > ignore them (default:off)
> > 
> 
> IMHO, deflating everything is a waste of the computer resources. HTML files
> really compress well. But most of the image files currently supported, e.g.,
> PNG, GIF, JPEG are already compressed, and deflating them will really do
> nothing -- just spend your CPU. I think that compressing text/html for browsers
> who send "Accept-Encoding: gzip" is the right approach. A possible enhancement
> is to have a directive (DeflateAddMimeType) which will enable deflating more
> mime types, e.g., text/plain, but these are really rare! Another type which is
> worth compressing is application/postscript, since many viewers (I am not an
> expert which - at least those decendents of GhostScript) are capable of
> viewing gzipped postscript files. The problem with that is that this is not a
> function of the browser, which cannot handle such files, but a function of the
> viewer, so the required "Accept-Encoding: gzip" doesn't prove anything about
> the ability of the external viewer!
> 
> To summerize, I suggest to deflate only types which can be handled by the
> browser itself, and which are not already compressed, which amounts to
> text/html or more generally text/* (text/css for instance).

In my mod_deflate module (for Apache 1.3.x) I'd enabled by default
"text/html" only. You can add or remove another type with DeflateTypes
directive. Here are some recomendations:

application/x-javascript   NN4 does not understand it compressed.
text/css   the same.

text/plain   Macromedia FlashPlayer 4.x-5.x does not understand it
 compressed when get it with loadVariables() function via browser.
text/xml Macromedia FlashPlayer 5.x does not understand it
 compressed when get it with XML.load() function via browser.

application/x-shockwave-flash   FlashPlayer plugin for NN4 for Windows
 does not understand it compressed. Although plugin for Linux   
 NN4 work correctly.

text/rtf   MSIE 4.x-6.x understand correctly them
application/msword when compressed. NN and Opera does not.
application/vnd.ms-excel
application/vnd.ms-powerpoint

Igor Sysoev

RE: [PATCH] mod_deflate

[Sander Striker]

> From: Zvi Har'El [mailto:[EMAIL PROTECTED]]
> Sent: 16 February 2002 08:37

> On Fri, 15 Feb 2002 09:44:19 -0800, Ian Holsman wrote about "Re: [PATCH] 
>mod_deflate":
> > 
> > 
> > I'm still not very happy about compressing EVERYTHING and excluding
> > certain browsers
> > as you would have to exclude IE & Netscape.
> > 
> > so
> > this is a
> > -1 for this patch.
> > in order to change this checks need to be there with a directive to
> > ignore them (default:off)
> > 
> 
> IMHO, deflating everything is a waste of the computer resources. HTML files
> really compress well. But most of the image files currently supported, e.g.,
> PNG, GIF, JPEG are already compressed, and deflating them will really do
> nothing -- just spend your CPU. I think that compressing text/html for browsers
> who send "Accept-Encoding: gzip" is the right approach. A possible enhancement
> is to have a directive (DeflateAddMimeType) which will enable deflating more
> mime types, e.g., text/plain, but these are really rare! Another type which is
> worth compressing is application/postscript, since many viewers (I am not an
> expert which - at least those decendents of GhostScript) are capable of
> viewing gzipped postscript files. The problem with that is that this is not a
> function of the browser, which cannot handle such files, but a function of the
> viewer, so the required "Accept-Encoding: gzip" doesn't prove anything about
> the ability of the external viewer!
> 
> To summerize, I suggest to deflate only types which can be handled by the
> browser itself, and which are not already compressed, which amounts to
> text/html or more generally text/* (text/css for instance).

It is not a question of browsers.  Subversion is also an HTTP client (for details
on how subversion works: http://subversion.tigris.org), actually a DAV client,
but that's not the point.  We want to compress a lot more than what the common
browser can handle.  And from some preliminary testing, we've seen a big win
with compression (traffic size reduced to 10% of the original.  Disclaimer:
this number is not acurate).  I refuse to rule this out by saying: "the common
browser doesn't support this".  I'd much rather decide on browser and content
type, which could be done through configuration.  Your DeflateAddMimeType could
be an option, but I'd rather see AddOutputFilter take a content type argument:
AddOutputFilter  ( | ).  Anyhow, it is more
about configuration now than to the codes default.

> Best,
> 
> Zvi.

Sander

Re: [PATCH] mod_deflate

[Zvi Har'El]

On Fri, 15 Feb 2002 09:44:19 -0800, Ian Holsman wrote about "Re: [PATCH] mod_deflate":
> 
> 
> I'm still not very happy about compressing EVERYTHING and excluding
> certain browsers
> as you would have to exclude IE & Netscape.
> 
> so
> this is a
> -1 for this patch.
> in order to change this checks need to be there with a directive to
> ignore them (default:off)
> 

IMHO, deflating everything is a waste of the computer resources. HTML files
really compress well. But most of the image files currently supported, e.g.,
PNG, GIF, JPEG are already compressed, and deflating them will really do
nothing -- just spend your CPU. I think that compressing text/html for browsers
who send "Accept-Encoding: gzip" is the right approach. A possible enhancement
is to have a directive (DeflateAddMimeType) which will enable deflating more
mime types, e.g., text/plain, but these are really rare! Another type which is
worth compressing is application/postscript, since many viewers (I am not an
expert which - at least those decendents of GhostScript) are capable of
viewing gzipped postscript files. The problem with that is that this is not a
function of the browser, which cannot handle such files, but a function of the
viewer, so the required "Accept-Encoding: gzip" doesn't prove anything about
the ability of the external viewer!

To summerize, I suggest to deflate only types which can be handled by the
browser itself, and which are not already compressed, which amounts to
text/html or more generally text/* (text/css for instance).

Best,

Zvi.
-- 
Dr. Zvi Har'El mailto:[EMAIL PROTECTED] Department of Mathematics
tel:+972-54-227607   Technion - Israel Institute of Technology
fax:+972-4-8324654 http://www.math.technion.ac.il/~rl/ Haifa 32000, ISRAEL
"If you can't say somethin' nice, don't say nothin' at all." -- Thumper (1942)
  Saturday, 4 Adar 5762, 16 February 2002,  9:21AM

Re: [PATCH] mod_deflate

[Ian Holsman]

Sander Striker wrote:
>>From: Ian Holsman [mailto:[EMAIL PROTECTED]]
>>Sent: 15 February 2002 18:44
>>
>  
> Hi,
> 
> 
>>I'm still not very happy about compressing EVERYTHING and excluding
>>certain browsers as you would have to exclude IE & Netscape.
>>
>>so this is a -1 for this patch.
>>in order to change this checks need to be there with a directive to
>>ignore them (default:off)
>>
> 
> Hopefully this patch resolves your veto.  Without that patch, deflate
> would be useless for subversion (and any other client that can
> handle compressed responses other than text/html).

veto resolved.
go apply it.

> 
> 
> Sander
> 
> 
> Index: modules/experimental/mod_deflate.c
> ===
> RCS file: /home/cvspublic/httpd-2.0/modules/experimental/mod_deflate.c,v
> retrieving revision 1.3
> diff -u -r1.3 mod_deflate.c
> --- modules/experimental/mod_deflate.c  15 Feb 2002 16:33:33 -  1.3
> +++ modules/experimental/mod_deflate.c  15 Feb 2002 17:50:56 -
> @@ -235,6 +235,15 @@
>  return ap_pass_brigade(f->next, bb);
>  }
> 
> +/* Some browsers might have problems with content types
> + * other than text/html, so set gzip-only-text/html
> + * (with browsermatch) for them
> + */
> +if (strncmp(r->content_type, "text/html", 9)
> +&& apr_table_get(r->subprocess_env, "gzip-only-text/html")) {
> +return ap_pass_brigade(f->next, bb);
> +}
> +
>  /* some browsers might have problems, so set no-gzip
>   * (with browsermatch) for them */
>  if (apr_table_get(r->subprocess_env, "no-gzip")) {
> 
> 

RE: [PATCH] mod_deflate

[Sander Striker]

> From: Ian Holsman [mailto:[EMAIL PROTECTED]]
> Sent: 15 February 2002 18:44
 
Hi,

> I'm still not very happy about compressing EVERYTHING and excluding
> certain browsers as you would have to exclude IE & Netscape.
>
> so this is a -1 for this patch.
> in order to change this checks need to be there with a directive to
> ignore them (default:off)

Hopefully this patch resolves your veto.  Without that patch, deflate
would be useless for subversion (and any other client that can
handle compressed responses other than text/html).


Sander


Index: modules/experimental/mod_deflate.c
===
RCS file: /home/cvspublic/httpd-2.0/modules/experimental/mod_deflate.c,v
retrieving revision 1.3
diff -u -r1.3 mod_deflate.c
--- modules/experimental/mod_deflate.c  15 Feb 2002 16:33:33 -  1.3
+++ modules/experimental/mod_deflate.c  15 Feb 2002 17:50:56 -
@@ -235,6 +235,15 @@
 return ap_pass_brigade(f->next, bb);
 }

+/* Some browsers might have problems with content types
+ * other than text/html, so set gzip-only-text/html
+ * (with browsermatch) for them
+ */
+if (strncmp(r->content_type, "text/html", 9)
+&& apr_table_get(r->subprocess_env, "gzip-only-text/html")) {
+return ap_pass_brigade(f->next, bb);
+}
+
 /* some browsers might have problems, so set no-gzip
  * (with browsermatch) for them */
 if (apr_table_get(r->subprocess_env, "no-gzip")) {

Re: [PATCH] mod_deflate

[Ian Holsman]

I'm still not very happy about compressing EVERYTHING and excluding
certain browsers
as you would have to exclude IE & Netscape.

so
this is a
-1 for this patch.
in order to change this checks need to be there with a directive to
ignore them (default:off)




Sander Striker wrote:
>>Sander Striker wrote:
>>
>>
>>>@@ -297,6 +287,7 @@
>>>
>>> apr_table_setn(r->headers_out, "Content-Encoding", "gzip");
>>> apr_table_setn(r->headers_out, "Vary", "Accept-Encoding");
>>>+apr_table_unset(r->headers_out, "Content-Length");
>>> }
>>>
>>> APR_BRIGADE_FOREACH(e, bb) {
>>>
>>>  
>>>
>>Do you mean that till now, mod_deflate preserved the original CL, of
>>the file before being deflated ???
>>
> 
> yes
> 
> 
>>Wow...
>>Maybe I am confused, or miss something, but how has mod_deflate
>>succeeded to work till now?
>>Or are gzip-supporting-browsers smart enough to ignore the CL of
>>compressed responses?
>>
> 
> Apparently so*.  It is in modules/experimental for a reason, although
> I'm sure we'd all love to get it out.  It will require some work though.
> 
> *) Or we were just lucky.  Subversion only failed a long way into
> the process of checking out a source tree.  It had pulled down
> quite an amount of files prior to failing (due to the server
> closing the connection I might add).
> 
> 
>>Eli Marmor
>>
> 
> Sander
> 
> 
> 

RE: [PATCH] mod_deflate

[Sander Striker]

> Sander Striker wrote:
> 
>> @@ -297,6 +287,7 @@
>> 
>>  apr_table_setn(r->headers_out, "Content-Encoding", "gzip");
>>  apr_table_setn(r->headers_out, "Vary", "Accept-Encoding");
>> +apr_table_unset(r->headers_out, "Content-Length");
>>  }
>> 
>>  APR_BRIGADE_FOREACH(e, bb) {
>> 
>>   
> 
> Do you mean that till now, mod_deflate preserved the original CL, of
> the file before being deflated ???

yes

> Wow...
> Maybe I am confused, or miss something, but how has mod_deflate
> succeeded to work till now?
> Or are gzip-supporting-browsers smart enough to ignore the CL of
> compressed responses?

Apparently so*.  It is in modules/experimental for a reason, although
I'm sure we'd all love to get it out.  It will require some work though.

*) Or we were just lucky.  Subversion only failed a long way into
the process of checking out a source tree.  It had pulled down
quite an amount of files prior to failing (due to the server
closing the connection I might add).

> Eli Marmor

Sander

Re: [PATCH] mod_deflate

[Eli Marmor]

Sander Striker wrote:

> @@ -297,6 +287,7 @@
> 
>  apr_table_setn(r->headers_out, "Content-Encoding", "gzip");
>  apr_table_setn(r->headers_out, "Vary", "Accept-Encoding");
> +apr_table_unset(r->headers_out, "Content-Length");
>  }
> 
>  APR_BRIGADE_FOREACH(e, bb) {
> 
>   

Do you mean that till now, mod_deflate preserved the original CL, of
the file before being deflated ???

Wow...
Maybe I am confused, or miss something, but how has mod_deflate
succeeded to work till now?
Or are gzip-supporting-browsers smart enough to ignore the CL of
compressed responses?

-- 
Eli Marmor
[EMAIL PROTECTED]
CTO, Founder
Netmask (El-Mar) Internet Technologies Ltd.
__
Tel.:   +972-9-766-1020  8 Yad-Harutzim St.
Fax.:   +972-9-766-1314  P.O.B. 7004
Mobile: +972-50-23-7338  Kfar-Saba 44641, Israel