Re: [PATCH] ICAP service chains and sets

On 06/27/2009 11:41 PM, Amos Jeffries wrote:
> Alex Rousskov wrote:
>>> in src/adaptation/AccessCheck.cc you create a ServiceFilter via:
>>> "new AccessCheck( ServiceFilter(method, vp, req, rep), cb, cbdata);"
>>> which takes a copy by & reference. The ServiceFilter locks the given
>>> request and reply but I can't see where the ServiceFilter is deleted (&
>>> references cannot be and the object is not RefCounted.)
>>> - I expect this to lead to memory leaks. Possibly big ones.
>>
>> ServiceFilter instances in this context are not allocated on the heap.
>> Thus, we do not (and cannot) delete them. The compiler generates code to
>> destruct the temporary variable and the class data member. Did I
>> misunderstood your concern?
>
> Ah right. I had forgotten that C++ trick you seem to like using.
> This works then despite the stack changes during AccessCheck callbacks?
>
> Remember we still have not gotten to the bottom of that 10MB/minute
> memory leak near the last one of these I noticed. Personally I'd rather
> go the long way with new/delete and RefCount to be sure of the copies
> than use this trick.

There is no trick. Well, no more of a trick than calling a function with
an integer argument. It is just simple copying of values (which can be
optimized away by the compiler in some cases) or, if references are
used, temporary sharing of values.

In this specific context, we could replace:

  A a;
  foo(a);

with

  A *a = new A;
  foo(a);
  ...
  delete a; // somewhere

But it seems like the top version is less likely to leak and any leaks
are easier to fix, especially when you consider that foo() would also
need to allocate/deallocate.

I did check that the Adaptation::ServiceFilter class has the right set
of methods to deal with message locking/unlocking during creation,
copying, assignment, and destruction.

We could replace everything with refcounting pointers, but it would be
slightly slower, may not save us if the ServiceFilter class is indeed
buggy, and would simply hide the true problem better -- we need to
replace HTTPMSG*LOCK with refcounting pointers. *That* is the true
source of many leaks and fears. But that nice project is outside this
patch scope.

Thank you,

Alex.

>>> in adaptation/icap/ModXact.cc when testing for HDR_X_NEXT_SERVICES I
>>> think it would be worth warning if a non-routable service attempted to
>>> hand back a routing header. How to do it efficiently may be a problem
>>> though.
>>
>> I do not think there would be a critical performance impact if we check
>> using the current code. The difference between a hash and a simple array
>> with linear search ought to be small when you are dealing with less than
>> 10 integers.
>>
>> Another problem, IMO, is how not to warn on every response (which is far
>> more expensive operation than searching). I wanted to port a
>> warn-once-in-a-while facility from Polygraph to Squid for a long time...
>>
>> Added TODO.
>
> Good point.
>
>>
>>
>>> This TODO needs to be enacted rather soon. We are aiming for performance
>>> in 3.2...
>>> // TODO: use HttpMsg::clone()!
>>
>> Agreed. A usable clone() method has been available at least since the
>> eCAP addition. This is not a new TODO or a new problem. This is a nice,
>> small-context project for volunteers :-).
>>
>
> :-)
>
>>
>>> On the side; this has brought to my attention the adaptation/forward.h.
>>> Can we not call that adaptation/Adaptation.h instead? the forward.h
>>> breaks CamelCase and will clash with src/forward.h on those recently
>>> vocal compilers.
>>
>> "Forward" is kind of standard (e.g., <iosfwd>) and more intuitive, IMO:
>> We are providing forward declarations to avoid too many unimportant
>> dependencies via header files. I would rather move or rename
>> src/forward.h :-)
>
> Fair point. It's not currently causing issue for most places. So its out
> of scope here if we go that way.
>
>>
>>> notes.dox ***
>>> \section label takes two parameters: an ID then the text title.
>>
>> Fixed.
>>
>>> mixing \b and the <b></b> equivalents on alternate lines looks a bit
>>> mucky. Can we at least do it consistently within one document?
>>
>> I have no idea where I copied that ugly mix from :-). Replaced all with
>> <b></b>.
>>
>>
>>> Thats it from me. Just ... no more huge project for a few decades,
>>> yes? ;)
>>
>> You call this huge?! I spent more time on StringNg. ;-)
>>
>>
>> Please look at the enhanced logging patch as well. This work depends on
>> that patch. If the changes are approved, the enhanced logging stuff must
>> go into the trunk first.
>>
>
> I'm up to that now. This updated patch is a vote yes from me.
>
> Amos
Received on Sun Jun 28 2009 - 06:59:51 MDT