Re: [PATCH] request_header_add adds a memory leak

On 07/20/2012 07:01 PM, Amos Jeffries wrote:
> 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.

Committed to trunk as r12226.

Thank you,

Alex.
Received on Sun Jul 22 2012 - 03:16:07 MDT