Extending ap_register_*_filter

[Justin Erenkrantz <je...@apache.org>]

In discussions about how to properly handle 304 (not modified), it
seems to me that we need a way for a filter to explicitly run some
code before the handler (and hence any data is generated) is
executed.

So, I propose adding another argument to the register functions -

typedef int (*ap_init_filter_func)(ap_filter_t *f);

ap_register_output_filter will look like:

AP_DECLARE(ap_filter_rec_t *) ap_register_output_filter(const char *name,
                                           ap_out_filter_func filter_func,
                                           ap_init_filter_func filter_init,
                                           ap_filter_type ftype)

ap_register_input_filter will look like:

AP_DECLARE(ap_filter_rec_t *) ap_register_input_filter(const char *name,
                      ap_in_filter_func filter_func,
                      ap_init_filter_func filter_init,
                      ap_filter_type ftype);

For both functions, the filter_init may be NULL.

Then, in ap_invoke_handler(), we walk all of the filter chains and
iff the filter_rec has a init function, we call it.  This way only
the filters that are involved in the request are called and they
have a chance to tweak the request_rec (or anything else) before the
actual data is generated.

Now, the question remains what do we do here in this init function.
My case is that we could in theory move the context-initialization
code for most filters to this new function.  (The case where !f->ctx
is handled is either too complicated or incorrect in a lot of our
modules.  This would simplify the core filter operations for a lot
of modules.)

However, this isn't required and current modules can just add NULL to
their parameter list, but it allows for modules like php and
mod_include to set r->no_local_copy so that the handler can call
ap_meets_conditions() before any data is generated and the 304 case
will be ignored properly.  So, mod_include's register line could be:

ap_register_output_filter("INCLUDES", includes_filter, includes_setup,
                           AP_FTYPE_RESOURCE);

where the bare minimum includes_setup function is:

apr_status_t includes_setup(ap_filter_t *f) {
    f->r->no_local_copy = 1; 
}

(I would also move the context-initialization code, but it isn't
strictly required.)

This is the best and cleanest solution I can come up with.  Does
anyone see any problems with this?  I will most likely take a pass
at implementing this tonight and post a patch for review.  -- justin

Re: Extending ap_register_*_filter

[Justin Erenkrantz <je...@apache.org>]

On Sat, Jun 08, 2002 at 01:02:32AM -0700, Greg Stein wrote:
> This feels totally extraneous to me. The 'insert_filter' hook is defined to
> run *right before* the handler is called. It is the absolute latest hook. It
> would seem there is no difference between using that one and any kind of
> init-filter hook.

Nope, the insert_filter hook would be run unconditionally even if
the filter isn't present.  Therefore, unless each module were to walk
through the filter chain *itself* to determine if its filter was in
the chain or not, there is a lot of wasted time.  Also, AIUI, hooks
are only valid one per file, so we couldn't have separate setup funcs
per input and output filters.  

mod_include wouldn't want to set r->no_local_copy for *every* request.
It'd only want to do so when it is present and we're about to serve
the page.

> (in fact, filter initialization is/can also be done as the filter is
>  inserted. the caller can initialize the context for use by the filter)

The issue is that if we were to call this before the handlers were
invoked, then it could potentially be called too early or too often.
I'm thinking of subrequests here - if the data isn't going to be
generated, then why bother initializing the context?

> Just what is the benefit of this relative to inserting a filter with a
> specific context, and/or by using the insert_filter hook.

How can mod_include when it is only present as a filter indicate
that the request is dynamic and can't honor If-Modified-Since et al
requests?  In this case, it has to set r->no_local_copy = 1.  And,
it must do so before the handlers begin because they can call
ap_meets_conditions() anywhere.  If you have any ideas how to solve
this, I'd love to hear it.  -- justin

Re: Extending ap_register_*_filter

[Greg Stein <gs...@lyra.org>]

On Fri, Jun 07, 2002 at 01:51:21PM -0700, Justin Erenkrantz wrote:
> In discussions about how to properly handle 304 (not modified), it
> seems to me that we need a way for a filter to explicitly run some
> code before the handler (and hence any data is generated) is
> executed.
>...
> This is the best and cleanest solution I can come up with.  Does
> anyone see any problems with this?  I will most likely take a pass
> at implementing this tonight and post a patch for review.  -- justin

This feels totally extraneous to me. The 'insert_filter' hook is defined to
run *right before* the handler is called. It is the absolute latest hook. It
would seem there is no difference between using that one and any kind of
init-filter hook.

(in fact, filter initialization is/can also be done as the filter is
 inserted. the caller can initialize the context for use by the filter)

Just what is the benefit of this relative to inserting a filter with a
specific context, and/or by using the insert_filter hook.

Cheers,
-g

-- 
Greg Stein, http://www.lyra.org/

Re: Extending ap_register_*_filter

[Justin Erenkrantz <je...@apache.org>]

On Fri, Jun 07, 2002 at 07:07:17PM -0400, Cliff Woolley wrote:
> +1.  Seems like the best way to do it.  My only question is: what about
> the AP_MODE_INIT flag on ap_get_brigade()?  Couldn't that go away now in
> favor of the new ap_register_input_filter() parameter?...

Yes, AP_MODE_INIT could be removed.  I don't know know if I'll get to
that tonight as my priority is fixing the 304 showstopper.

But, yes, the infrastructure to do so would definitely be in place.
There's a method to my madness.  =)  -- justin

Re: Extending ap_register_*_filter

[Cliff Woolley <jw...@virginia.edu>]

On Fri, 7 Jun 2002, Justin Erenkrantz wrote:

> In discussions about how to properly handle 304 (not modified), it
> seems to me that we need a way for a filter to explicitly run some
> code before the handler (and hence any data is generated) is
> executed.
> So, I propose adding another argument to the register functions -
> typedef int (*ap_init_filter_func)(ap_filter_t *f);
> ap_register_output_filter will look like:
> AP_DECLARE(ap_filter_rec_t *) ap_register_output_filter(const char *name,
>                                            ap_out_filter_func filter_func,
>                                            ap_init_filter_func filter_init,
>                                            ap_filter_type ftype)
> ap_register_input_filter will look like:
> AP_DECLARE(ap_filter_rec_t *) ap_register_input_filter(const char *name,
>                       ap_in_filter_func filter_func,
>                       ap_init_filter_func filter_init,
>                       ap_filter_type ftype);


+1.  Seems like the best way to do it.  My only question is: what about
the AP_MODE_INIT flag on ap_get_brigade()?  Couldn't that go away now in
favor of the new ap_register_input_filter() parameter?  It always seemed
like a bit of a hack to me, and by shifting that burden over to
ap_r_i_f(), we make the input and output filters behave more alike.  For
example, another way to solve this problem would have been to add a
AP_MODE_INIT-style parameter to ap_pass_brigade() so the output filters
could do what the input filters do, but that seems a bit hokey.  The
alternative is to get rid of AP_MODE_INIT and do it all through
ap_register_*_filter.

Or we could just have both, but I don't see what the point of that would
be.

Thoughts?

--Cliff