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

 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 Tue Jun 28 2011 - 00:40:21 MDT