You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by "William A. Rowe, Jr." <wr...@covalent.net> on 2002/03/01 22:12:27 UTC
Baffled by the negotation fix...
I believe the attached .patch file should resolve our issues with the
subrequest
negotiation. Essentially, we want the sub request to build a new filter
chain if
we plan on using the subrequest as a 'fast internal redirect'.
This patch does just that, when we fast redirect, we replace the original
request
filters with the new request filters. In preparing the request, we go back
to our
favorite r->connection filters to start, when we are making the subrequest
with
a NULL next_filter.
But the patch is broken. Free beer at ApacheCon 2002 to the first hacker
who
untangles the misbehavior that fouls up this patch. Make that hard stuff if
you
discover it's a simple bug in my patch :)
One hint. I think our whole "Either Connection or Request" idea of filters
is
borked. It really seems that "Either Connection or Protocol or Body" is a
much
better model - this bug seems to prove that once again. I think we really
wanted
to grab r->http_input_filters and r->protocol_output_filters when we set up
the
subreq of next_filter==NULL. But there doesn't appear to be such a thing.
Bill
RE: Baffled by the negotation fix...
Posted by Ryan Bloom <rb...@covalent.net>.
> On Fri, Mar 01, 2002 at 09:15:46PM -0800, Ryan Bloom wrote:
> > You are thinking of this wrong. Filters that are stored on
> > r->protocol_filters wouldn't survive a single request, they would be
> > required for the lifetime of the request. The idea is that anything
> > that lives in r->output_filters would be allowed to be removed by
the
> > server code at will. Anything on r->protocol_filters would be
static
> > for the lifetime
> > of a request, and c->output_filters survives a request.
>
> We're essentially creating a new request here. I'm not sure why
We aren't creating a new request. The fact that we are simply copying
the features of the request_rec to another request proves that this
isn't a new request.
> you think we have to preseve the old protocol filters. They simply
> aren't valid anymore. They are for the old request - we want a *new*
> instance of these filters. The old request filters aren't valid for
> this type of sub-request - the contexts shouldn't be valid. The
> only filters with a valid context are the connection-filters.
That is BS. Those filters are completely valid. We are talking about
moving them from the original request to the redirected request, just
like we move other stuff. The protocol level filters are just as valid
as the connection filters, the only real problem is that we don't have a
structure that sits between the request and the connection. If we did,
those filters would go there.
> > > What's wrong with a simple hook that lets the protocol engines add
> > > filters to a request? -- justin
> >
> > Because the more hooks we add, the more complex it is to write a
module,
> > and the slower the server actually goes. Also, why should this be a
> > hook? There are a set of filters that absolutely must survive for
the
> > lifetime of a request and no longer. Those are protocol filters,
and
> > they shouldn't be treated equivalently to content filters.
>
> How do you know which filters to add? There are number of places
> in the HTTP modules where they manually add CONTENT_LENGTH and
> HTTP_HEADER. My point is just to remove all of the duplicate code
> and allow modules to reset the filters. I can't believe that this
> will cause the server to be that much slower. By making it a hook,
> you abstract out the protocol - only the protocol engine knows
> which filters are required. -- justin
Have you tried to write a module recently Justin? I am constantly
trying to explain to people what each hook does. Adding another hook is
a PITA for module authors, and I am against adding hooks that are
absolutely not necessary. As for which filters to add, any filter that
has an AP_HTTP_HEADER_TYPE type would be static for the request.
The abstraction still exists, it is just that it is done with filter
type instead of a hook. It still removes all of the duplicate code. It
also makes the filter categories mean something other than just the
order of the filter. The lifetime of the filter is the tied directly to
the type of the filter.
Ryan
Re: Baffled by the negotation fix...
Posted by Justin Erenkrantz <je...@ebuilt.com>.
On Fri, Mar 01, 2002 at 09:15:46PM -0800, Ryan Bloom wrote:
> You are thinking of this wrong. Filters that are stored on
> r->protocol_filters wouldn't survive a single request, they would be
> required for the lifetime of the request. The idea is that anything
> that lives in r->output_filters would be allowed to be removed by the
> server code at will. Anything on r->protocol_filters would be static
> for the lifetime
> of a request, and c->output_filters survives a request.
We're essentially creating a new request here. I'm not sure why
you think we have to preseve the old protocol filters. They simply
aren't valid anymore. They are for the old request - we want a *new*
instance of these filters. The old request filters aren't valid for
this type of sub-request - the contexts shouldn't be valid. The
only filters with a valid context are the connection-filters.
> > What's wrong with a simple hook that lets the protocol engines add
> > filters to a request? -- justin
>
> Because the more hooks we add, the more complex it is to write a module,
> and the slower the server actually goes. Also, why should this be a
> hook? There are a set of filters that absolutely must survive for the
> lifetime of a request and no longer. Those are protocol filters, and
> they shouldn't be treated equivalently to content filters.
How do you know which filters to add? There are number of places
in the HTTP modules where they manually add CONTENT_LENGTH and
HTTP_HEADER. My point is just to remove all of the duplicate code
and allow modules to reset the filters. I can't believe that this
will cause the server to be that much slower. By making it a hook,
you abstract out the protocol - only the protocol engine knows
which filters are required. -- justin
RE: Baffled by the negotation fix...
Posted by Ryan Bloom <rb...@covalent.net>.
> On Fri, Mar 01, 2002 at 04:50:16PM -0800, Ryan Bloom wrote:
> > I don't understand how this is working. The original code from Will
> > came from a discussion that he and I had over the phone. My
question is
> > how were the protocol specific filters removed in this case?
> >
> > Oh, damn, the problem is that these filters are request specific
instead
> > of connection specific. What we really need, is a filter type that
is
> > request specific, but that isn't specifically tied to the request.
> > Basically another field in the request_rec, which would store all
store
> > all HTTP_HEADER filters. That way, they wouldn't be lost for
> > sub-requests and internal redirects.
>
> Yes, but the concept of the protocol is intertwined with the concept
> of a request. A request should be one "transaction" of a protocol
> interaction. A protocol filter shouldn't survive successive
> iterations of a "request" - we saw that didn't work for HTTP.
You are thinking of this wrong. Filters that are stored on
r->protocol_filters wouldn't survive a single request, they would be
required for the lifetime of the request. The idea is that anything
that lives in r->output_filters would be allowed to be removed by the
server code at will. Anything on r->protocol_filters would be static
for the lifetime
of a request, and c->output_filters survives a request.
> What's wrong with a simple hook that lets the protocol engines add
> filters to a request? -- justin
Because the more hooks we add, the more complex it is to write a module,
and the slower the server actually goes. Also, why should this be a
hook? There are a set of filters that absolutely must survive for the
lifetime of a request and no longer. Those are protocol filters, and
they shouldn't be treated equivalently to content filters.
Ryan
Re: Baffled by the negotation fix...
Posted by Justin Erenkrantz <je...@ebuilt.com>.
On Fri, Mar 01, 2002 at 04:50:16PM -0800, Ryan Bloom wrote:
> I don't understand how this is working. The original code from Will
> came from a discussion that he and I had over the phone. My question is
> how were the protocol specific filters removed in this case?
>
> Oh, damn, the problem is that these filters are request specific instead
> of connection specific. What we really need, is a filter type that is
> request specific, but that isn't specifically tied to the request.
> Basically another field in the request_rec, which would store all store
> all HTTP_HEADER filters. That way, they wouldn't be lost for
> sub-requests and internal redirects.
Yes, but the concept of the protocol is intertwined with the concept
of a request. A request should be one "transaction" of a protocol
interaction. A protocol filter shouldn't survive successive
iterations of a "request" - we saw that didn't work for HTTP.
What's wrong with a simple hook that lets the protocol engines add
filters to a request? -- justin
RE: Baffled by the negotation fix...
Posted by Ryan Bloom <rb...@covalent.net>.
> On Fri, Mar 01, 2002 at 03:12:27PM -0600, William A. Rowe, Jr. wrote:
> > I believe the attached .patch file should resolve our issues with
the
> > subrequest
> > negotiation. Essentially, we want the sub request to build a new
filter
> > chain if
> > we plan on using the subrequest as a 'fast internal redirect'.
> >
> > This patch does just that, when we fast redirect, we replace the
> original
> > request
> > filters with the new request filters. In preparing the request, we
go
> back
> > to our
> > favorite r->connection filters to start, when we are making the
> subrequest
> > with
> > a NULL next_filter.
> >
> > But the patch is broken. Free beer at ApacheCon 2002 to the first
> hacker
> > who
> > untangles the misbehavior that fouls up this patch. Make that hard
> stuff if
> > you
> > discover it's a simple bug in my patch :)
> >
> > One hint. I think our whole "Either Connection or Request" idea of
> filters
> > is
> > borked. It really seems that "Either Connection or Protocol or
Body" is
> a
> > much
> > better model - this bug seems to prove that once again. I think we
> really
> > wanted
> > to grab r->http_input_filters and r->protocol_output_filters when we
set
> up
> > the
> > subreq of next_filter==NULL. But there doesn't appear to be such a
> thing.
>
> Yup, you're right. We need to add protocol-specific filters that
> can be arbitrarily reset. Hint: this is just what reset_filters
> does, but its static. Notice that we do this when we are about
> to serve an HTTP error - the same concept of a subreq applies to
> the error condition.
>
> To prove that, this patch works for me. So, what I think we should
> do is add a protocol hook that says, "add all of your required
> filters if this is your protocol." How does that sound? -- justin
I don't understand how this is working. The original code from Will
came from a discussion that he and I had over the phone. My question is
how were the protocol specific filters removed in this case?
Oh, damn, the problem is that these filters are request specific instead
of connection specific. What we really need, is a filter type that is
request specific, but that isn't specifically tied to the request.
Basically another field in the request_rec, which would store all store
all HTTP_HEADER filters. That way, they wouldn't be lost for
sub-requests and internal redirects.
Ryan
Re: Baffled by the negotation fix...
Posted by Justin Erenkrantz <je...@ebuilt.com>.
On Fri, Mar 01, 2002 at 03:12:27PM -0600, William A. Rowe, Jr. wrote:
> I believe the attached .patch file should resolve our issues with the
> subrequest
> negotiation. Essentially, we want the sub request to build a new filter
> chain if
> we plan on using the subrequest as a 'fast internal redirect'.
>
> This patch does just that, when we fast redirect, we replace the original
> request
> filters with the new request filters. In preparing the request, we go back
> to our
> favorite r->connection filters to start, when we are making the subrequest
> with
> a NULL next_filter.
>
> But the patch is broken. Free beer at ApacheCon 2002 to the first hacker
> who
> untangles the misbehavior that fouls up this patch. Make that hard stuff if
> you
> discover it's a simple bug in my patch :)
>
> One hint. I think our whole "Either Connection or Request" idea of filters
> is
> borked. It really seems that "Either Connection or Protocol or Body" is a
> much
> better model - this bug seems to prove that once again. I think we really
> wanted
> to grab r->http_input_filters and r->protocol_output_filters when we set up
> the
> subreq of next_filter==NULL. But there doesn't appear to be such a thing.
Yup, you're right. We need to add protocol-specific filters that
can be arbitrarily reset. Hint: this is just what reset_filters
does, but its static. Notice that we do this when we are about
to serve an HTTP error - the same concept of a subreq applies to
the error condition.
To prove that, this patch works for me. So, what I think we should
do is add a protocol hook that says, "add all of your required
filters if this is your protocol." How does that sound? -- justin
Index: server/request.c
===================================================================
RCS file: /home/cvs/httpd-2.0/server/request.c,v
retrieving revision 1.99
diff -u -r1.99 request.c
--- server/request.c 15 Feb 2002 07:43:20 -0000 1.99
+++ server/request.c 1 Mar 2002 22:26:32 -0000
@@ -1492,15 +1492,20 @@
/* start with the same set of output filters */
if (next_filter) {
+ /* no input filters for a subrequest */
rnew->output_filters = next_filter;
+ ap_add_output_filter_handle(ap_subreq_core_filter_handle,
+ NULL, rnew, rnew->connection);
}
else {
+ /* If NULL - we are expecting to be internal_fast_redirect'ed
+ * to this subrequest - or this request will never be invoked.
+ */
+ rnew->input_filters = r->input_filters;
rnew->output_filters = r->output_filters;
+ ap_add_output_filter("CONTENT_LENGTH", NULL, rnew, rnew->connection);
+ ap_add_output_filter("HTTP_HEADER", NULL, rnew, rnew->connection);
}
- ap_add_output_filter_handle(ap_subreq_core_filter_handle,
- NULL, rnew, rnew->connection);
-
- /* no input filters for a subrequest */
ap_set_sub_req_protocol(rnew, r);
Index: modules/http/http_request.c
===================================================================
RCS file: /home/cvs/httpd-2.0/modules/http/http_request.c,v
retrieving revision 1.125
diff -u -r1.125 http_request.c
--- modules/http/http_request.c 25 Jan 2002 01:11:46 -0000 1.125
+++ modules/http/http_request.c 1 Mar 2002 22:26:33 -0000
@@ -441,6 +441,8 @@
r->err_headers_out);
r->subprocess_env = apr_table_overlay(r->pool, rr->subprocess_env,
r->subprocess_env);
+ r->output_filters = rr->output_filters;
+ r->input_filters = rr->input_filters;
}
AP_DECLARE(void) ap_internal_redirect(const char *new_uri, request_rec *r)