You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Justin Erenkrantz <je...@apache.org> on 2002/04/01 21:56:01 UTC
[PATCH] AddOutputFilterByType issues.
I've been partly out of it lately, but I think there is a problem
with AddOutputFilterByType. Since ap_set_content_type() can
be called arbitrarily many times, it will try to add each filter
as directed by AddOutputFilterByType on each call. For certain
filters, that isn't a terrifically good idea - such as mod_deflate.
For others (say mod_include), this is okay.
We can't compress a file twice (and is causing problems with
Subversion right now), but that's what we are doing now -
mod_deflate is in the chain twice. mod_mime calls
ap_set_content_type twice in its normal execution (I dunno why,
but that's sucky).
I'm using the following patch to minimize how often
ap_add_output_filters_by_type is called. This fixes the symptom,
but perhaps we should think of a better solution? Perhaps only
allow AP_FTYPE_RESOURCE filters to be added multiple times, and
AP_FTYPE_CONTENT_SET or higher can only be added once?
Thoughts? -- justin
Index: modules/http/http_protocol.c
===================================================================
RCS file: /home/cvs/httpd-2.0/modules/http/http_protocol.c,v
retrieving revision 1.406
diff -u -r1.406 http_protocol.c
--- modules/http/http_protocol.c 29 Mar 2002 08:17:22 -0000 1.406
+++ modules/http/http_protocol.c 1 Apr 2002 21:43:07 -0000
@@ -1272,14 +1272,16 @@
AP_DECLARE(void) ap_set_content_type(request_rec *r, const char *ct)
{
- r->content_type = ct;
+ if (!r->content_type || strcmp(r->content_type, ct)) {
+ r->content_type = ct;
- /* Insert filters requested by the AddOutputFiltersByType
- * configuration directive. Content-type filters must be
- * inserted after the content handlers have run because
- * only then, do we reliably know the content-type.
- */
- ap_add_output_filters_by_type(r);
+ /* Insert filters requested by the AddOutputFiltersByType
+ * configuration directive. Content-type filters must be
+ * inserted after the content handlers have run because
+ * only then, do we reliably know the content-type.
+ */
+ ap_add_output_filters_by_type(r);
+ }
}
typedef struct header_filter_ctx {
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
RE: [PATCH] AddOutputFilterByType issues.
Posted by Ryan Bloom <rb...@covalent.net>.
> On Mon, Apr 01, 2002 at 02:05:04PM -0800, Ryan Bloom wrote:
> > I have two conflicting thoughts, so I'll put them both out there for
> > discussion.
> >
> > 1) I agree (mostly) RESOURCE filters are really the only ones that
make
> > sense to add multiple times. We should ensure that no other filters
are
> > added more than once.
> >
> > 2) It is up to the filter to protect against this case. That can
be
> > done by walking the filter chain to ensure that the same filter
isn't in
> > the list already. Of course, walking the chain could be slow,
depending
> > on how many filters there are.
>
> How could the filter itself protect against this case? By the
> time it is called, it is already too late - the chain is created.
> Or am I missing something?
>
> The only thing I can think of is that it it looks at f/f->next
> to make sure that there are no other copies left in the chain that
> haven't been called. I think it would be better to just protect
> against that when we *add* filters rather than when we execute
> them.
>
> I will commit the strcmp check now. -- Justin
All it needs to do is leave a message for itself in the request_rec.
That can be done in either the per_request vector, or the r->notes
table. I wouldn't want to use r->notes, because that could get really
large quickly.
The reality though is that the filter needs to be the one to check this.
There are easily some RESOURCE filters that shouldn't be added more than
once, for example the mod_header_footer filter should only be inserted
once.
There are some other methods that I can think of, but none of them are
really clean. I kind of like the idea of just putting a note in the
per-request vector.
Ryan
Re: [PATCH] AddOutputFilterByType issues.
Posted by Justin Erenkrantz <je...@apache.org>.
On Mon, Apr 01, 2002 at 02:05:04PM -0800, Ryan Bloom wrote:
> I have two conflicting thoughts, so I'll put them both out there for
> discussion.
>
> 1) I agree (mostly) RESOURCE filters are really the only ones that make
> sense to add multiple times. We should ensure that no other filters are
> added more than once.
>
> 2) It is up to the filter to protect against this case. That can be
> done by walking the filter chain to ensure that the same filter isn't in
> the list already. Of course, walking the chain could be slow, depending
> on how many filters there are.
How could the filter itself protect against this case? By the
time it is called, it is already too late - the chain is created.
Or am I missing something?
The only thing I can think of is that it it looks at f/f->next
to make sure that there are no other copies left in the chain that
haven't been called. I think it would be better to just protect
against that when we *add* filters rather than when we execute
them.
I will commit the strcmp check now. -- justin
RE: [PATCH] AddOutputFilterByType issues.
Posted by Ryan Bloom <rb...@covalent.net>.
> I've been partly out of it lately, but I think there is a problem
> with AddOutputFilterByType. Since ap_set_content_type() can
> be called arbitrarily many times, it will try to add each filter
> as directed by AddOutputFilterByType on each call. For certain
> filters, that isn't a terrifically good idea - such as mod_deflate.
> For others (say mod_include), this is okay.
>
> We can't compress a file twice (and is causing problems with
> Subversion right now), but that's what we are doing now -
> mod_deflate is in the chain twice. mod_mime calls
> ap_set_content_type twice in its normal execution (I dunno why,
> but that's sucky).
>
> I'm using the following patch to minimize how often
> ap_add_output_filters_by_type is called. This fixes the symptom,
> but perhaps we should think of a better solution? Perhaps only
> allow AP_FTYPE_RESOURCE filters to be added multiple times, and
> AP_FTYPE_CONTENT_SET or higher can only be added once?
>
> Thoughts? -- justin
I have two conflicting thoughts, so I'll put them both out there for
discussion.
1) I agree (mostly) RESOURCE filters are really the only ones that make
sense to add multiple times. We should ensure that no other filters are
added more than once.
2) It is up to the filter to protect against this case. That can be
done by walking the filter chain to ensure that the same filter isn't in
the list already. Of course, walking the chain could be slow, depending
on how many filters there are.
Regardless, this patch is all good-ness, and should be committed ASAP.
Ryan
RE: [PATCH] AddOutputFilterByType issues.
Posted by Ryan Bloom <rb...@covalent.net>.
> I've been partly out of it lately, but I think there is a problem
> with AddOutputFilterByType. Since ap_set_content_type() can
> be called arbitrarily many times, it will try to add each filter
> as directed by AddOutputFilterByType on each call. For certain
> filters, that isn't a terrifically good idea - such as mod_deflate.
> For others (say mod_include), this is okay.
>
> We can't compress a file twice (and is causing problems with
> Subversion right now), but that's what we are doing now -
> mod_deflate is in the chain twice. mod_mime calls
> ap_set_content_type twice in its normal execution (I dunno why,
> but that's sucky).
>
> I'm using the following patch to minimize how often
> ap_add_output_filters_by_type is called. This fixes the symptom,
> but perhaps we should think of a better solution? Perhaps only
> allow AP_FTYPE_RESOURCE filters to be added multiple times, and
> AP_FTYPE_CONTENT_SET or higher can only be added once?
>
> Thoughts? -- justin
I have two conflicting thoughts, so I'll put them both out there for
discussion.
1) I agree (mostly) RESOURCE filters are really the only ones that make
sense to add multiple times. We should ensure that no other filters are
added more than once.
2) It is up to the filter to protect against this case. That can be
done by walking the filter chain to ensure that the same filter isn't in
the list already. Of course, walking the chain could be slow, depending
on how many filters there are.
Regardless, this patch is all good-ness, and should be committed ASAP.
Ryan
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org