Yann Ylavic <ylavic....@gmail.com> 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
