Mailing List Archive: [PATCH] mod_deflate: remove filter after seeing EOS

[PATCH] mod_deflate: remove filter after seeing EOS

Oct 9, 2023, 11:50 AM

Post #1 of 14 (433 views)

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 [ In reply to ]

dev at httpd

Oct 30, 2023, 7:47 AM

Post #2 of 14 (416 views)

Permalink

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 <enorris@etsy.com> 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 [ In reply to ]

jorton at redhat

Dec 20, 2023, 5:37 AM

Post #3 of 14 (390 views)

Permalink

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 <enorris@etsy.com> 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 [ In reply to ]

dev at httpd

Dec 20, 2023, 7:07 AM

Post #4 of 14 (389 views)

Permalink

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 <https://www.etsy.com>

On Wed, Dec 20, 2023 at 8:37?AM Joe Orton <jorton@redhat.com> 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 <enorris@etsy.com> 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 [ In reply to ]

ylavic.dev at gmail

Dec 20, 2023, 7:08 AM

Post #5 of 14 (389 views)

Permalink

On Wed, Dec 20, 2023 at 2:40?PM Joe Orton <jorton@redhat.com> 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 <enorris@etsy.com> 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 [ In reply to ]

dev at httpd

Dec 20, 2023, 7:18 AM

Post #6 of 14 (389 views)

Permalink

On Wed, Dec 20, 2023 at 10:09?AM Yann Ylavic <ylavic.dev@gmail.com> wrote:
>
> On Wed, Dec 20, 2023 at 2:40?PM Joe Orton <jorton@redhat.com> 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 <enorris@etsy.com> 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 [ In reply to ]

rpluem at apache

Dec 20, 2023, 7:24 AM

Post #7 of 14 (389 views)

Permalink

On 12/20/23 4:08 PM, Yann Ylavic wrote:
> On Wed, Dec 20, 2023 at 2:40?PM Joe Orton <jorton@redhat.com> 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 <enorris@etsy.com> 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 [ In reply to ]

jorton at redhat

Dec 20, 2023, 7:43 AM

Post #8 of 14 (389 views)

Permalink

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 [ In reply to ]

ylavic.dev at gmail

Dec 20, 2023, 7:45 AM

Post #9 of 14 (389 views)

Permalink

On Wed, Dec 20, 2023 at 4:18?PM Eric Norris <enorris@etsy.com> wrote:
>
> On Wed, Dec 20, 2023 at 10:09?AM Yann Ylavic <ylavic.dev@gmail.com> 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 [ In reply to ]

jorton at redhat

Dec 20, 2023, 7:57 AM

Post #10 of 14 (389 views)

Permalink

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 <jorton@redhat.com> 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 [ In reply to ]

dev at httpd

Dec 20, 2023, 8:00 AM

Post #11 of 14 (389 views)

Permalink

On Wed, Dec 20, 2023 at 10:43?AM Joe Orton <jorton@redhat.com> 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 [ In reply to ]

ylavic.dev at gmail

Dec 20, 2023, 8:00 AM

Post #12 of 14 (389 views)

Permalink

On Wed, Dec 20, 2023 at 4:45?PM Yann Ylavic <ylavic.dev@gmail.com> wrote:
>
> On Wed, Dec 20, 2023 at 4:18?PM Eric Norris <enorris@etsy.com> wrote:
> >
> > On Wed, Dec 20, 2023 at 10:09?AM Yann Ylavic <ylavic.dev@gmail.com> 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 [ In reply to ]

ylavic.dev at gmail

Dec 20, 2023, 8:01 AM

Post #13 of 14 (389 views)

Permalink

On Wed, Dec 20, 2023 at 4:57?PM Joe Orton <jorton@redhat.com> 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 <jorton@redhat.com> 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 [ In reply to ]

dev at httpd

Dec 20, 2023, 8:03 AM

Post #14 of 14 (389 views)

Permalink

On Wed, Dec 20, 2023 at 10:58?AM Joe Orton <jorton@redhat.com> 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 <jorton@redhat.com> 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!