Hi Yann, 2018-07-19 22:55 GMT+02:00 Yann Ylavic <[email protected]>: > > Hi Luca, > > On Thu, Jul 19, 2018 at 9:59 PM, Luca Toscano <[email protected]> wrote: > > > > I confirm that it works fine in my local dev environment, plus now gdb's > > dump_brigade makes sense, but I am not sure that I have understood what was > > wrong. In my tests, the headers were the first heap bucket reported by > > dump_brigade in gdb (before your patch), so IIUC we were saving them to > > holdingbb and then finally passing them via ap_pass_brigade. But I thought > > that this was they way to go, what am I missing? I am pretty sure this is > > basic filter knowledge but I have missed it up to now, if not present in the > > docs already I'll make sure to add it. > > You probably didn't test with chunked encoding (neither did I!), see > how ap_http_header_filter() adds the "CHUNK" filter after the headers > have been sent, so if anything retains the headers they will be > considered as the body by the (late) ap_http_chunk_filter()..
Thanks a lot for the explanation, it took me a while to understand what you meant but I think I kinda got it. You are saying that ap_http_header_filter specifically passes the brigade containing the headers to the next filters in the chain, hoping that it would reach the client before any attempt to insert the chunk filter is made. But in this case, the rate_limit_filter interferes with ap_http_header_filter's assumptions, and buffers the brigade containing the headers (that never reaches the external client). At this point, the chunk filter is added by ap_http_header_filter, and during the next ap_pass_brigade call the buffered data is passed by rate_limit_filter to the chain, eventually reaching the chunk filter, that will interpret it as body (making the mess that Cory reported). Is my understanding vaguely resembling what happens? If so I think it would be an interesting use case to document in https://httpd.apache.org/docs/trunk/developer/output-filters.html :) > @team: the issue with HEAD request is that ap_http_header_filter() > eats the final EOS (along with the body), I don't think this is > correct because downstream filters may depend on it to bail out (like > rate_limit_filter() does now). > So how about the attached patch both with regard to level > FTYPE_CONNECTION - 1 for ratelimit filter and ap_http_header_filter() > to forward the final EOS? It looks good to me, but I'd really like to get others opinion on how to fix this problem. What do others think about it? Thanks a lot for this fix! Luca
