Re: request_header_add adds a memory leak

On 07/20/2012 05:40 AM, Amos Jeffries wrote:
> On 20/07/2012 12:34 p.m., Alex Rousskov wrote:
>> Hello,
>>
>> I believe the current trunk is profusely leaking memory. There may
>> be several reasons, but my current suspect is a circular RefCounted
>> reference that allows neither HttpRequest nor AccessLogEntry to be freed:
>>
>> HttpRequest -> AccessLogEntry -> HttpRequest
>>
>> The circle was created by the request_header_add feature during the
>> review process, when it was decided that we should reuse the Format API
>> to expand macros in the added header values. That API needs
>> AccessLogEntry. This is good news -- there is not much code relying on
>> the circle existence!
>>
>> The bad news is that breaking the circle is not trivial. Copying
>> AccessLogEntry to HttpRequest (instead of linking the two) is not a good
>> option because the AccessLogEntry object needs to be populated with
>> values collected throughout the master transaction lifetime -- up until
>> httpHdrAdd() is called on the server side (and much later when we start
>> supporting macros in mangled responses).
>

Have the AccessLogEntry connected to HttpRequest looked good idea
because using this way you can make AccessLogEntry available in all
squid subsystems... But looks that we have problem doing this, they are
using both RefCount-like locking ...

> However, when the transaction reaches termination and logging is
> completed it becomes no longer useful (next transaction must re-fill
> with new details specific to that transaction).
>
> A clear() method on the entry would be good for resetting all its
> pointers after logging completion.

Does not looks good idea... what about cases where we don't log anything
for a reason (eg transaction terminated unexpectedly+bug)?

>
>>
>> The mess stems from the fact that AccessLogEntry is not generally
>> available on the server side. HttpRequest is available there but it
>> cannot be used for AccessLogEntry smuggling (due to the above loop).
>>
>> Unless I hear better ideas, I will extend the FwdState::fwdStart() API
>> to supply an AccessLogEntry (and store it in Server/etc). This may even
>> help with future plans of converting ALE into some kind of MasterXaction
>> class.

Looks the best... But if AccessLogEntry converted to Xaction class, it
should be passed/linked to all squid subsystems (adaptation etc)...

>
> Both would be good loong-term, but up to you which gets chosen. I'm
> inclined to leave the API changes until xaction project can be started
> properly.
>
> Amos
>
Received on Fri Jul 20 2012 - 09:35:53 MDT