httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Paul J. Reder" <rede...@remulak.net>
Subject Re: [Patch] Hook to allow insertion of filters during error processing
Date Thu, 11 Dec 2003 16:15:17 GMT


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



Mime
View raw message