Re: [PATCH] Add request_header_add option and [request|reply]_header_* manglers fixes

On 07/06/2012 08:41 AM, Tsantilas Christos wrote:
> On 07/01/2012 02:25 PM, Amos Jeffries wrote:
>> Thank you. In most places the problem is that AccessLogEntry is the
>> closest thing we have to a XactionData object for the whole HTTP
>> transaction. (I still have patches waiting around to polish up and
>> submit for audit creating that master transaction object).
>> AccessLogEntry is available from the ConnStateData / conn() almost
>> everywhere in the code. So it *is* actually available everywhere through
>> the transaction, just not filled out. We have a lot of code cleaning to
>> make it consistent, (but outside this patches scope).
>
> The AccessLogEntry is not available from ConnStateData, it is only
> available from ClientHttpRequest, and this is correct because an
> accessLogEntry is Http request related, not connection related.
>
>>
>> For now it is available via "request->conn()->ale". Or a new temporary
>> one created in local scope for the function doing display output. It may
>> need filling out with pointers to things required (is SSL content, HTTP
>> request, client connection), the code before logging is very
>> inconsistent/non-existent.
>
> The only way to access AccessLogEntry is using the following path:
> request->conn()->getCurrentContext()->http->al
>
> I am not feeling comfortable using the request->conn() although it looks
> that works well in the current squid code. Moreover the
> request->conn()->getCurrentContext()->http path, I think does not
> guarantee that the ClientHttpRequest is the object we need.
>
> Sometimes I am felling the initial design was that a (server side)
> HttpRequest corresponds to more than one connections (more than one
> connections accessing the same object) and of course to more than one
> ClientHttpRequest objects.
>
> Also I believe that the request->conn()->getCurrentContext() may not
> return the ClientSocketContext corresponding to our HttpRequest.
> We can go from ClientSocketContext to HttpRequest
> (ClientSocketContext->http->request) but no the reverse.
>
> Any opinions here?

I agree that getting AccessLogEntry via getCurrentContext() is wrong,
even if it works in most cases. A pipelining connection may have many
requests associated with it and, at the time we log the transaction, our
request may no longer be "current".

Can we add a link from HttpRequest to the corresponding ClientHttpRequest?

Or is it better to add a link from HttpRequest to AccessLogEntry?

Seriously malformed and special requests may not have HttpRequest
assciated with them, so I understand why ClientHttpRequest stores
AccessLogEntry and not HttpRequest, but we need to make a way for the
rest of the code to find AccessLogEntry without relying on
getCurrentContext(). And HttpRequest is available nearly everywhere.

HTH,

Alex.
Received on Fri Jul 06 2012 - 14:55:43 MDT