Geoffrey Young wrote:
Jeff Trawick wrote:
Geoffrey Young wrote:
I've griped a bit before about having default_handler make conditional GET decisions, and this is probably another instance where having ap_meets_conditions in it's own filter could avoid inevitable problems.
I'm up for laying out my issues if the list is interested in them and reevaluating the whole meets_conditions/filter_init stuff.
please do
ok. it's been outlined before in the archives in bits and pieces, but here is essentially the problem...
with the advent of output filters, content handlers (such as default_handler) can't possibly have enough information to accurately determine whether a 304 is warranted. the issue is, of course, that later filters might alter content to the point where the 304 would have been wrong.
...as in the case of mod_include which alters or unsets the Last-Modified value.
the way Apache chose to work around this was to add the filter_init callback to allow filters to add processing just prior to content generation. presumably, this is where filters could call ap_update_mtime or whatnot to add their information in the conditional GET calculations. the decision to use filter_init over putting ap_meets_conditions logic in its own filter was made in order to let the (presumably) most popular case - default_handler + mod_deflate - be as fast as possible,
This isn't entirely true. Yes, there is the filter-init callback, but the other accepted solution is to create a handler/filter hybrid (such as the cache code). The handler participates in making sure all the pieces and parts required to make a good decision are present, then the filter acts on the final decision. The handler can insert the desired filter.
I believe the decision was also motivated by a desire to make the 304 as streamlined as possible. Why run a 304 through *all* of the filters and associated processing if the effort was just to be thrown away and a 304 sent. This is why the error processing removed the filters. It was just too agressive about it without providing a way to undo the removal where necessary.
Also, your comments and observations hold some merit in relation to a 304 (understandably, since that is the primary motivator for my patch), but this fix also applies equally to other errors (including all the 300's and 200's). The alternate option of moving the ap_meets_conditions logic does not address these other non-304 errors.
>now, I wasn't present for that discussion (some of which happened on #irc) but that's how it looks like it happened to me from the archives. I certainly don't mean to misrepresent things if I'm not entirely accurate - please speak up if I've missed something.
so, for the most part how things are currently works for core. however, the first time I tried to use output filters with conditional GET logic I saw that the idea fell short.
just using current 2.1 core you can see for yourself how filter_init is suboptimal. here's an Apache-Test tarball you can view or run if you like (keep in mind it's 2.1 specific)
http://perl.apache.org/~geoff/bug-ap_meets_conditions.tar.gz
basically, what this tarball shows is that the filter_init won't run when the content-type is set during content-generation and the filter is added via AddOutputFilterByType. while this particular example might seem a bit abnormal, it was one I could quickly see that didn't involve anything other than core modules.
the basic problem, however, isn't in AddOutputFilterByType, it's in the entire idea that conditional GET logic can be fully accommodated via filter_init processing. basically, filters that rely on decisions made by content-generators are left in a catch-22: if they add logic to the request via filter_init they risk that logic being wrong due to later decisions, or if they postpone the logic until after content-generation they risk never having it called at all.
I agree that this is a potential pitfall. Perhaps not all situations can be addressed by a handler/filter hybrid. But moving the meets_condition logic to a filter doesn't address all of what my fix does. It also introduces the possibility of doing a lot of extra work that gets discarded. We are stuck in a bit of a dilemma, as I see it. We can run ap_meets_conditions early to avoid unnecessary cycles and risk sending 304's when we might not have, or wait until the last possible moment to check meets_conditions and be as accurate as possible at the price of performance. The 304 is supposed to be a time and bandwidth saver, this might reduce it to a bandwidth saver only.
hopefully, this kind of makes sense to at least some people. personally, the only thing that makes sense to me is moving conditonal GET logic to it's own filter, similar to Content-Length I suppose. yes, it would slow down the server for default+deflate responses, but I guess that would be the trade-off for allowing people to properly control the cache-correctness of their responses (among other things).
I have the *feeling* that Paul's patch is a very safe fix (i.e., no regression for 2.0.x) for the missing Expires on 304, and in general I like the idea of a module getting the chance to add a filter on the error path, but I have no awareness of other problems caused by the present meets-conditions handling.
I haven't looked at this particular issue to know whether the two ideas are mutually exclusive, but I can't help but wonder if the problem would go away if meets_conditons were held back until the very last moment. we might be talking about two different things, though :)
I think there is some concept overlap but there are issues solved by my patch that aren't addressed by your suggestion. My feeling right now is to commit my patch and address the meets_condition question as a seperate issue. Moving the meets_condition code also impacts other modules (such as mod_cache) which would need to be updated to work properly. Moving the meets_condition code may make the 304 related aspects of my patch irrelevant, but I suspect it will still need to be there for other errors.
anyway, as usual, thanks for listening.
--Geoff
-- Paul J. Reder ----------------------------------------------------------- "The strength of the Constitution lies entirely in the determination of each citizen to defend it. Only if every single citizen feels duty bound to do his share in this defense are the constitutional rights secure." -- Albert Einstein
