You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Bill Stoddard <bi...@wstoddard.com> on 2003/05/28 23:14:53 UTC
Is it a subrequest, a redirect or a fast redirect?
mod_expires (with my latest patch ) is still broken. The expires header
will not be added when the resource is resolved via a DirectoryIndex
(ie, a fast redirect) . mod_expires will not add the expires filter to
what it believes to be a subrequest and a simple check of (r->main !=
NULL) is not granular enough to tell of a request is a subrequest or a
redirect/fast-redirect. I am not about to suggest that redirects
should inherit the filter stack of the main request :-). An alternate
solution is to add a flag to request_rec (or some other indicator) that
a module can check to determine if a request is a true subrequest or a
redirect (fast or normal). Perhaps set the flag in make_sub_req based
on the 'next_filter' argument. Any other ideas?
Bill
Re: Is it a subrequest, a redirect or a fast redirect?
Posted by André Malo <nd...@perlig.de>.
* Bill Stoddard wrote:
> Would you list the PRs that you believe are related to fast redirects
> not working correctly? Perhaps there are some simple changes to get
> fast redirect working and perhaps there are some cases where we simply
> should not be using fast redirect.
Aside from the fact, that the mod_expires problem is solved, I found
currently three reports related to fast_redirect (there may be more):
http://nagoya.apache.org/bugzilla/show_bug.cgi?id=8493 (backwards compat)
http://nagoya.apache.org/bugzilla/show_bug.cgi?id=13211 (worked around so
far)
http://nagoya.apache.org/bugzilla/show_bug.cgi?id=15112 (in conclusion with
mod_rewrite)
nd
--
>>> Muschelflucht-Zusatzeinrichtung.
>> Shell-Escape ist ja noch klar, aber `Zusatzeinrichtung'?
> extension?
Feature. -- gefunden in de.org.ccc
Re: [PATCH] Reworked mod_expires
Posted by Justin Erenkrantz <ju...@erenkrantz.com>.
--On Saturday, May 31, 2003 3:00 PM -0400 Bill Stoddard <bi...@wstoddard.com>
wrote:
> No failures observed so far with this patch, but more testing is needed.
> The best part of all is all the code I was able to remove from mod_expires.
> It would be very cool if someone with the perl-framework setup could test
> this against the mod_expires test cases and report back the results (that
> would be worth a beer on me at the next ac ;-)
Pass the expires tests in perl-framework here.
My only caveat with the patch is to watch the long lines. ;-) -- justin
P.S. mod_expires.c in APACHE_2_0_BRANCH and HEAD are out-of-sync.
[PATCH] Reworked mod_expires
Posted by Bill Stoddard <bi...@wstoddard.com>.
No failures observed so far with this patch, but more testing is
needed. The best part of all is all the code I was able to remove from
mod_expires. It would be very cool if someone with the perl-framework
setup could test this against the mod_expires test cases and report back
the results (that would be worth a beer on me at the next ac ;-)
Bill
Index: mod_expires.c
===================================================================
RCS file: /home/cvs/httpd-2.0/modules/metadata/mod_expires.c,v
retrieving revision 1.39.2.3
diff -u -r1.39.2.3 mod_expires.c
--- mod_expires.c 28 May 2003 14:50:40 -0000 1.39.2.3
+++ mod_expires.c 31 May 2003 18:47:41 -0000
@@ -212,11 +212,6 @@
apr_table_t *expiresbytype;
} expires_dir_config;
-typedef struct {
- int defaulted;
- apr_table_t *expfields;
-} expires_interphase_t;
-
/* from mod_dir, why is this alias used?
*/
#define DIR_CMD_PERMS OR_INDEXES
@@ -434,10 +429,7 @@
apr_time_t expires;
int additional_sec;
char *timestr;
- expires_interphase_t *notes;
- notes = (expires_interphase_t *)
ap_get_module_config(r->request_config,
- &expires_module);
switch (code[0]) {
case 'M':
if (r->finfo.filetype == 0) {
@@ -482,131 +474,81 @@
* according to the content-type of the response -- if it hasn't
* already been set.
*/
-static apr_status_t expires_by_type_filter(ap_filter_t *f,
- apr_bucket_brigade *b)
+static apr_status_t expires_filter(ap_filter_t *f,
+ apr_bucket_brigade *b)
{
request_rec *r;
expires_dir_config *conf;
- expires_interphase_t *notes;
- const char *bytype_expiry;
+ const char *expiry;
apr_table_t *t;
r = f->r;
conf = (expires_dir_config *) ap_get_module_config(r->per_dir_config,
&expires_module);
- notes = (expires_interphase_t *)
ap_get_module_config(r->request_config,
- &expires_module);
/*
- * If this filter is getting called, it *should* mean that
- * the fixup-phase handler ran and set things up for us.
* Check to see which output header table we should use;
* mod_cgi loads script fields into r->err_headers_out,
* for instance.
*/
- bytype_expiry = apr_table_get(r->err_headers_out, "Expires");
- if (bytype_expiry != NULL) {
+ expiry = apr_table_get(r->err_headers_out, "Expires");
+ if (expiry != NULL) {
t = r->err_headers_out;
}
else {
- bytype_expiry = apr_table_get(r->headers_out, "Expires");
+ expiry = apr_table_get(r->headers_out, "Expires");
t = r->headers_out;
}
- if (bytype_expiry == NULL) {
+ if (expiry == NULL) {
/*
* No expiration has been set, so we can apply any managed by
- * this module. Check for one for this content type.
+ * this module. First, check to see if there is an applicable
+ * ExpiresByType directive.
*/
- bytype_expiry = apr_table_get(conf->expiresbytype,
- ap_field_noparam(r->pool,
- r->content_type));
- if (bytype_expiry != NULL) {
- set_expiration_fields(r, bytype_expiry, t);
- }
- else if ((notes != NULL) && notes->defaulted) {
- /*
- * None for this type, but there was a default defined --
- * so use it. Add the Expires header and add or replace the
- * Cache-Control header.
- */
- apr_table_overlap(r->headers_out, notes->expfields,
APR_OVERLAP_TABLES_SET);
+ expiry = apr_table_get(conf->expiresbytype,
ap_field_noparam(r->pool,
+
r->content_type));
+ if (expiry == NULL) {
+ /* Use the ExpiresDefault directive */
+ expiry = conf->expiresdefault;
+ }
+ if (expiry != NULL) {
+ set_expiration_fields(r, expiry, t);
}
}
ap_remove_output_filter(f);
return ap_pass_brigade(f->next, b);
}
-static int add_expires(request_rec *r)
+static void expires_insert_filter(request_rec *r)
{
expires_dir_config *conf;
- expires_interphase_t *notes;
- apr_table_t *rfields;
- char *code;
-
- if (ap_is_HTTP_ERROR(r->status)) {/* Don't add Expires headers to
errors */
- return DECLINED;
- }
- if (r->main != NULL) { /* Say no to subrequests */
- return DECLINED;
+ /* Don't add Expires headers to errors */
+ if (ap_is_HTTP_ERROR(r->status)) {
+ return;
+ }
+ /* Say no to subrequests */
+ if (r->main != NULL) {
+ return;
}
-
conf = (expires_dir_config *) ap_get_module_config(r->per_dir_config,
&expires_module);
- if (conf == NULL) {
- ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r,
- "internal error: %s", r->filename);
- return HTTP_INTERNAL_SERVER_ERROR;
- }
-
- if (conf->active != ACTIVE_ON) {
- return DECLINED;
- }
-
- notes = apr_palloc(r->pool, sizeof(expires_interphase_t));
- notes->defaulted = 0;
- notes->expfields = apr_table_make(r->pool, 4);
- ap_set_module_config(r->request_config, &expires_module, notes);
- /*
- * If there are any ExpiresByType directives for this scope,
- * add the output filter and defer all setting to it. We
- * do make a note of any ExpiresDefault value for its use.
+ /* Check to see if the filter is enabled and if there are any
applicable config
+ * directives for this directory scope
*/
- if (! apr_is_empty_table(conf->expiresbytype)) {
- ap_add_output_filter("MOD_EXPIRES", NULL, r, r->connection);
- rfields = notes->expfields;
+ if (conf->active != ACTIVE_ON ||
+ (apr_is_empty_table(conf->expiresbytype) &&
!conf->expiresdefault)) {
+ return;
}
- else {
- rfields = r->headers_out;
- }
- /*
- * Apply the default expiration if there is one; the filter will
- * narrow it down later if possible.
- */
- code = conf->expiresdefault;
-
- if (code[0] == '\0') {
- return OK;
- }
- else {
- /*
- * Note that we're setting it from the default, so that
- * the output filter (if it runs) knows it can override the
- * value. This allows the by-type filter to be able to
- * tell the difference between a value set by, say, a
- * CGI script and the one we set by default.
- */
- notes->defaulted = 1;
- }
- return set_expiration_fields(r, code, rfields);
+ ap_add_output_filter("MOD_EXPIRES", NULL, r, r->connection);
+ return;
}
-
static void register_hooks(apr_pool_t *p)
{
- ap_register_output_filter("MOD_EXPIRES", expires_by_type_filter, NULL,
+ ap_register_output_filter("MOD_EXPIRES", expires_filter, NULL,
AP_FTYPE_CONTENT_SET);
- ap_hook_fixups(add_expires,NULL,NULL,APR_HOOK_MIDDLE);
+ ap_hook_insert_filter(expires_insert_filter, NULL, NULL,
APR_HOOK_MIDDLE);
}
module AP_MODULE_DECLARE_DATA expires_module =
Re: Is it a subrequest, a redirect or a fast redirect?
Posted by Bill Stoddard <bi...@wstoddard.com>.
Justin Erenkrantz wrote:
> --On Friday, May 30, 2003 8:23 PM -0400 Bill Stoddard
> <bi...@wstoddard.com> wrote:
>
>> I am able to fix the mod_expires problem by doing a test to definitively
>> identify is a request is a subrequest or not (testing r->main is not
>> sufficient). It would be easy to add code to make_sub_request() to
>> set a
>> flag if the SUBREQ_CORE filter is added. This flag would definitively
>> identify if a request is a subrequest or not. Then we could test
>> (r->is_subreq) rather than (r->main) and be assured of correctly
>> identifying
>> a subrequest.
>
>
> Pardon my late entry in this thread.
>
> What about moving this mod_expires hook (add_expires) to insert_filter
> rather than having it in fixups? insert_filter is only invoked when
> the handler is run (see ap_invoke_handler). If the request had been
> processed by fast_redirect, the r->main would be NULL at this point
> (it'd inherit r->main of the original request). And, r->main would
> not be NULL for other types of subreqs.
>
> By the time the handler is invoked, a fast redirect *is* equivalent to
> the normal request. But, fixups is run before this distinction can be
> made cleanly.
>
> This, of course, makes perfect sense (to me) because add_expires *is*
> inserting a filter. Or, am I just missing something completely
> obvious here that would make this not work? -- justin
<hitting forehead saying "Doh">
Thank you for pointing out the obvious . Patch on the way.
Bill
Re: Is it a subrequest, a redirect or a fast redirect?
Posted by Justin Erenkrantz <ju...@erenkrantz.com>.
--On Friday, May 30, 2003 8:23 PM -0400 Bill Stoddard <bi...@wstoddard.com>
wrote:
> I am able to fix the mod_expires problem by doing a test to definitively
> identify is a request is a subrequest or not (testing r->main is not
> sufficient). It would be easy to add code to make_sub_request() to set a
> flag if the SUBREQ_CORE filter is added. This flag would definitively
> identify if a request is a subrequest or not. Then we could test
> (r->is_subreq) rather than (r->main) and be assured of correctly identifying
> a subrequest.
Pardon my late entry in this thread.
What about moving this mod_expires hook (add_expires) to insert_filter rather
than having it in fixups? insert_filter is only invoked when the handler is
run (see ap_invoke_handler). If the request had been processed by
fast_redirect, the r->main would be NULL at this point (it'd inherit r->main
of the original request). And, r->main would not be NULL for other types of
subreqs.
By the time the handler is invoked, a fast redirect *is* equivalent to the
normal request. But, fixups is run before this distinction can be made
cleanly.
This, of course, makes perfect sense (to me) because add_expires *is*
inserting a filter. Or, am I just missing something completely obvious here
that would make this not work? -- justin
Re: Is it a subrequest, a redirect or a fast redirect?
Posted by Bill Stoddard <bi...@wstoddard.com>.
André Malo wrote:
>* William A. Rowe, Jr. wrote:
>
>
>
>>>Looking through the bugreports and the problems we have...
>>>internal_fast_redirect saves some CPU cycles (I guess), but it's finally
>>>(currently) an unclean thing.
>>>
>>>
André,
Would you list the PRs that you believe are related to fast redirects
not working correctly? Perhaps there are some simple changes to get
fast redirect working and perhaps there are some cases where we simply
should not be using fast redirect.
I am able to fix the mod_expires problem by doing a test to definitively
identify is a request is a subrequest or not (testing r->main is not
sufficient). It would be easy to add code to make_sub_request() to set
a flag if the SUBREQ_CORE filter is added. This flag would definitively
identify if a request is a subrequest or not. Then we could test
(r->is_subreq) rather than (r->main) and be assured of correctly
identifying a subrequest.
This nasty hack will make mod_expires behave correctly with
DirectoryIndex responses.
Index: mod_expires.c
===================================================================
RCS file: /home/cvs/httpd-2.0/modules/metadata/mod_expires.c,v
retrieving revision 1.43
diff -u -r1.43 mod_expires.c
--- mod_expires.c 30 May 2003 16:33:57 -0000 1.43
+++ mod_expires.c 31 May 2003 00:05:08 -0000
@@ -547,13 +547,23 @@
return DECLINED;
}
- /* Note: This is broken, DirectoryIndex files will not get an
expires header
- * We should say -yes- to fast-redirects (which are not really
subrequests)
- * but this check is not granular enough to distinguish between a
fast-redirect
- * and a subrequest
+ /* Say no to subrequests but yes to requests about to be fast
redirected
+ * Note: r->main is not a reliable indicator if a request is a
subrequest
+ * or not. The best indicator of a subrequest is the presence of the
+ * SUBREQ_CORE filter (which is an AP_FTYPE_CONTENT_SET filter).
+ * This is a hack to cover us till we get a better mechanism for
distingushing
+ * between subrequests, redirects and fast redirects.
*/
- if (r->main != NULL) { /* Say no to subrequests */
- return DECLINED;
+ if (r->main != NULL) {
+ /* This might be a subrequest */
+ ap_filter_t *f;
+ f = r->output_filters;
+ while (f && f->frec->ftype <= AP_FTYPE_CONTENT_SET) {
+ if (!strcmp(f->frec->name, "subreq_core")) {
+ return DECLINED;
+ }
+ f = f->next;
+ }
}
conf = (expires_dir_config *) ap_get_module_config(r->per_dir_config,
Re: Is it a subrequest, a redirect or a fast redirect?
Posted by André Malo <nd...@perlig.de>.
* William A. Rowe, Jr. wrote:
>>Looking through the bugreports and the problems we have...
>>internal_fast_redirect saves some CPU cycles (I guess), but it's finally
>>(currently) an unclean thing.
>
> This was (in 1.3) an unclean thing in mod_negotiation. Early in 2.0,
> I significantly refactored it to assure we 'initialized things properly'
> between subrequests and fast_internal_redirects. And finally, just
> to ensure it was well tested, moved it into mod_dir and possibly
> other places.
yeah, it was not meant as offense (you probably know that). But it seems the
surrounding concepts are not good enough, e.g. treating the subrequest as
assbackwards without a flag on purpose of the subrequest etc.
(hmm. hard to describe)
> Now if we cannot maintain that code, I strongly suggest we drop that
> feature altogether in Apache 2.2 and revert it's use in mod_dir, and within
> mod_negotiation for 2.0 and 1.3.28.
I agree (though it hurts a bit). Can we maintain the code? I'd say yes -
later, after clearing the environment. We should switch to normal redirects
for now and try to break the problems down, and try to resolve them.
I personally hope to find the peace next time to provide some more details
of my thoughts...
>>IMHO we should drop and discourage the usage fast_redirects until the
>>concept (subreq/filters/data stream/redirects) has reached some more
>>maturity. Normal internal_redirects may be (a bit) slower, but they are
>>approved and work as expected, since they process a complete request.
>
> We must either ditch it or support it, all the way around.
it's a long way to 2.2. Until then it's beta software, so I'd see it not soo
strong. Before dropping the interface entirely let's try to fix it. (but
workaround in stable branches, of course).
nd
--
Ook. Ook. Ook. Ook. Ook. Ook. Ook. Ook. Ook. Ook. Ook. Ook. Ook. Ook.
Ook! Ook? Ook. Ook? Ook. Ook. Ook. Ook. Ook. Ook. Ook. Ook. Ook. Ook.
Ook. Ook. Ook. Ook. Ook? Ook. Ook! Ook! Ook? Ook! Ook. Ook? Ook. Ook.
Ook. Ook. Ook. Ook. Ook! Ook. Ook! Ook! Ook! Ook! Ook! Ook.
Re: Is it a subrequest, a redirect or a fast redirect?
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
At 10:29 AM 5/29/2003, you wrote:
>Looking through the bugreports and the problems we have...
>internal_fast_redirect saves some CPU cycles (I guess), but it's finally
>(currently) an unclean thing.
This was (in 1.3) an unclean thing in mod_negotiation. Early in 2.0,
I significantly refactored it to assure we 'initialized things properly'
between subrequests and fast_internal_redirects. And finally, just
to ensure it was well tested, moved it into mod_dir and possibly
other places.
Now if we cannot maintain that code, I strongly suggest we drop that
feature altogether in Apache 2.2 and revert it's use in mod_dir, and within
mod_negotiation for 2.0 and 1.3.28.
>IMHO we should drop and discourage the usage fast_redirects until the
>concept (subreq/filters/data stream/redirects) has reached some more
>maturity. Normal internal_redirects may be (a bit) slower, but they are
>approved and work as expected, since they process a complete request.
We must either ditch it or support it, all the way around.
Bill
Re: Is it a subrequest, a redirect or a fast redirect?
Posted by André Malo <nd...@perlig.de>.
* Bill Stoddard wrote:
> mod_expires (with my latest patch ) is still broken. The expires header
> will not be added when the resource is resolved via a DirectoryIndex
> (ie, a fast redirect) . mod_expires will not add the expires filter to
> what it believes to be a subrequest and a simple check of (r->main !=
> NULL) is not granular enough to tell of a request is a subrequest or a
> redirect/fast-redirect. I am not about to suggest that redirects
> should inherit the filter stack of the main request :-). An alternate
> solution is to add a flag to request_rec (or some other indicator) that
> a module can check to determine if a request is a true subrequest or a
> redirect (fast or normal). Perhaps set the flag in make_sub_req based
> on the 'next_filter' argument. Any other ideas?
Looking through the bugreports and the problems we have...
internal_fast_redirect saves some CPU cycles (I guess), but it's finally
(currently) an unclean thing.
IMHO we should drop and discourage the usage fast_redirects until the
concept (subreq/filters/data stream/redirects) has reached some more
maturity. Normal internal_redirects may be (a bit) slower, but they are
approved and work as expected, since they process a complete request.
I believe, the main problem of using them in DirectoryIndex is the late
stage (fixup hook) they are invoked. Some important things can not be done
with that concept, since they are not executed in subrequests but already
done in the main. (and some things may be done twice) (e.g.: expires,
cookies, RewriteRules (!) and probably more).
Perhaps the type_checker (which handles Multiviews) is actually the last
stage where fast_redirects make any sense.
Some more thoughts would be really helpful.
nd
--
Das, was ich nicht kenne, spielt stückzahlmäßig *keine* Rolle.
-- Helmut Schellong in dclc