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.
