Re: [PATCH] Remove the HttpStateData::orig_request member

If there is not any objection I will commit this patch to trunk.

On 06/28/2011 03:40 AM, Amos Jeffries wrote:
> On Mon, 27 Jun 2011 16:25:08 +0300, Tsantilas Christos wrote:
>> Hi all,
>>
>> When FwdServer::_peer is set, HttpStateData constructor creates a
>> new special HttpRequest, overwriting the request pointer set in the
>> parent (ServerStateData) constructor to fwd->request.
>>
>> This special HttpRequest sets the proper urlpath (which maybe
>> different from the original HttpRequest), the host
>> (HttpRequest::SetHost/GetHost) to be the peer hostname and inherits
>> flags, protocol and method. Also sets the HttpRequest::flags.proxying.
>>
>> I believe this is originaly done to handle only the differences in
>> urlpath and the host. But this is has as result to have two
>> HttpRequests object in HttpStateData, but their difference is not
>> clear, and this is causes some bugs in http.cc. In patch preamble I am
>> counting 4 possible bugs.
>>
>> This patch removes the HttpStateData::orig_request member and uses
>> always the HttpStateData::request member.
>>
>> If we decide that this patch is in the right direction probably we
>> should remove the ServerStateData::originalRequest() method too.
>>
>> Regards,
>> Christos
>
>
> I've seen one more that you don't list. Debugs() and error pages
> sometimes display the cache_peer hostname as the URL requested domain
> name when going to an origin. Regardless of what the virtual host name
> actually is.
>
> As to point (3), that includes caching of "private" replies as well as
> no-store. Which is just as bad, if not worse. And prevents caching of
> "public" replies when auth is used, not so bad, but annoying.
>
> As to point (4), it *was* a bug, but your change fixes it. The comment
> you added can be erased again.
>
>
> +1.
>
> and Thank You! that HttpStateData() illogic was blocking some of my work :)
>
> Amos
Received on Wed Jun 29 2011 - 20:54:09 MDT