Yann Ylavic <ylavic.dev@xxxxxxxxx> writes:
>> I am thinking about fixing this with the attached patch and proposing it for
>> backport to 2.4.x.
>>
>> Would there be any objections to that?
>
> +1 for the patch, I missed the separate 304 handling in
> mod_brotli/deflate, thanks for catching this!
Thanks, I will commit it at the earliest opportunity.
> It seems that the 304 "shortcut" happens too late though, after
> entity/content-* headers are added to the response, which does not
> look right. Don't we need something like the attached patch too?
I haven't checked it in details, but at first glance I think that this patch
could break a few cases.
One example would be a case with several filters working in a chain for a
304 response. The first of them sees Content-Encoding identity, performs
some fix-ups, such as the one in deflate_check_etag() and removes itself
from the chain without altering the r->content-encoding or the Content-
Encoding header value. The next filter then sees the C-E identity again,
decides to do another fix-up before bailing out, and thus results in an
incorrect ETag value or something similar.
(An interesting observation is that https://svn.apache.org/r743814 does an
opposite of this patch by ensuring that C-E is actually set prior to bailing
out and removing itself when dealing with 304's.)
However, a more important question is whether there is an actual problem to
solve. I see that ap_http_header_filter() features a whitelist of headers
that are sent for 304 responses (http_filters.c:1428), and all headers such
as Content-Encoding are filtered anyway.
So maybe the current state doesn't require fixing at all — assuming that
neither mod_deflate nor mod_brotli can actually begin streaming the 304
response with the (unexpected) set of headers — which I don't think could
be happening.
Thanks,
Evgeny Kotkov