Re: [PATCH] request_header_add adds a memory leak

On 21/07/2012 11:06 a.m., Alex Rousskov wrote:
> On 07/19/2012 08:40 PM, 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).
>> 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.
>>
>>> 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.
>> 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.
>
> Attached is the patch implementing those API changes. They are minor,
> and I even kept the old fwdStart() call so that fewer files like Asn.cc
> and htcp.cc have to be modified and exposed to AccessLogEntry.h. These
> changes give AccessLogEntry to the server side, but this is only a tiny
> step towards supporting a true MasterXact object.
>
> The patch removes HttpRequest::al member, breaking the refcounting loop.
> Squid does not seem to leak with these changes.
>
> Any objections to this patch going into trunk?

+1. Looks fine.

Amos
Received on Sat Jul 21 2012 - 01:01:15 MDT