[PATCH] subrequests don't inherit bodies

[Greg Ames <gr...@remulak.net>]

A customer reported a problem where their back end app would hang until a read 
timed out.  The main request was a POST which did have a request body which was 
read normally.  The POST response contained an ssi tag that caused a subrequest 
to be created.  The subrequest was forwarded to an app on a back end server. 
This forwarded subrequest contained a Content-Length header so the back end 
server/app was expecting a request body that didn't exist, thus the read timeout.

As it turns out, we clone all of the main request's input headers when we create 
the subrequest, including C-L.  Whacking the subrequest's C-L header fixes the 
hang.  Since the main request's body could also have be chunked, we should 
probably remove the subrequest's Transfer-Encoding header as well.

There could be other headers that don't make sense for subrequests without 
bodies, such as gzip/deflate headers.  But I'm concerned about adding too much 
to a fairly common code path.  a few thoughts for changes to subrequest creation:

1. do a quick test for the main request containing a body.  if true (pretty rare 
case), remove all headers pertaining to request bodies.

2. generate a minimal set of headers from scratch that make sense for the 
subrequest, rather than cloning them from the main request

3. (attached) just remove the headers that say a request body exists.

Thoughts?

Greg

Re: Subrequests should not inherit entity-header fields from the mainline request

[Greg Ames <gr...@remulak.net>]

Paul Querna wrote:
> Bill Stoddard wrote:

>> The problem is that the subrequest is inheriting entity-header fields 
>> from the mainline request (mainline request was a POST). This patch 
>> should be generalised to remove all inherited entity-header fields 
>> from the subrequest.
> 
> 
> Something that popped into my mind is, what about when you actually want 
> to do a POST subrequest?

from ap_set_sub_req_protocol():

     rnew->method          = "GET";
     rnew->method_number   = M_GET;
     rnew->protocol        = "INCLUDED";

...so I say we drive off that bridge when we get to it, to mis-quote Ted Kennedy.

Greg

Re: Subrequests should not inherit entity-header fields from the mainline request

[Joe Schaefer <jo...@sunstarsys.com>]

Paul Querna <ch...@force-elite.com> writes:


[...]

> Something that popped into my mind is, what about when you actually
> want to do a POST subrequest?

This is exactly what I've been struggling with in apreq.
I think the only sane thing apreq can do is to assume that
subrequests will never have bodies attached (so no POST data
will ever be available to a subrequest via apreq).  I think
this is the original intent, and that the input filter cloning
is just there to prevent segfaults (e.g. the subrequest might 
install new filters for itself).
-- 
Joe Schaefer

Re: Subrequests should not inherit entity-header fields from the mainline request

[Paul Querna <ch...@force-elite.com>]

Bill Stoddard wrote:
> Joe Schaefer wrote:
> 
>> Greg Ames <gr...@remulak.net> writes:
>>
>> [...]
>>
>>
>>> As it turns out, we clone all of the main request's input headers when
>>> we create the subrequest, including C-L.  Whacking the subrequest's
>>> C-L header fixes the hang.  Since the main request's body could also
>>> have be chunked, we should probably remove the subrequest's
>>> Transfer-Encoding header as well. 
>>
>>
>>
>> Shouldn't you remove Content-Type also?
>>
> 
> The problem is that the subrequest is inheriting entity-header fields 
> from the mainline request (mainline request was a POST). This patch 
> should be generalised to remove all inherited entity-header fields from 
> the subrequest.

Something that popped into my mind is, what about when you actually want 
to do a POST subrequest?

I think it would be quite hard with how subrequests are currently 
constructed.  I wish the all of the subrequest stuff could be refactored...

-Paul

Re: Subrequests should not inherit entity-header fields from the mainline request

[Greg Ames <gr...@remulak.net>]

Bill Stoddard wrote:
> Joe Schaefer wrote:

>>> As it turns out, we clone all of the main request's input headers when
>>> we create the subrequest, including C-L.  Whacking the subrequest's
>>> C-L header fixes the hang.  Since the main request's body could also
>>> have be chunked, we should probably remove the subrequest's
>>> Transfer-Encoding header as well. 

>> Shouldn't you remove Content-Type also?

> The problem is that the subrequest is inheriting entity-header fields 
> from the mainline request (mainline request was a POST). This patch 
> should be generalised to remove all inherited entity-header fields from 
> the subrequest.

done (rev 159410).  thanks for the tip.

Greg

Subrequests should not inherit entity-header fields from the mainline request

[Bill Stoddard <bi...@wstoddard.com>]

Joe Schaefer wrote:
> Greg Ames <gr...@remulak.net> writes:
> 
> [...]
> 
> 
>>As it turns out, we clone all of the main request's input headers when
>>we create the subrequest, including C-L.  Whacking the subrequest's
>>C-L header fixes the hang.  Since the main request's body could also
>>have be chunked, we should probably remove the subrequest's
>>Transfer-Encoding header as well. 
> 
> 
> Shouldn't you remove Content-Type also?
> 

The problem is that the subrequest is inheriting entity-header fields from the mainline request (mainline 
request was a POST). This patch should be generalised to remove all inherited entity-header fields from the 
subrequest.

Bill

Re: [PATCH] subrequests don't inherit bodies

[Joe Schaefer <jo...@sunstarsys.com>]

Greg Ames <gr...@remulak.net> writes:

[...]

> As it turns out, we clone all of the main request's input headers when
> we create the subrequest, including C-L.  Whacking the subrequest's
> C-L header fixes the hang.  Since the main request's body could also
> have be chunked, we should probably remove the subrequest's
> Transfer-Encoding header as well. 

Shouldn't you remove Content-Type also?

-- 
Joe Schaefer