Re: [PATCH] ICAP service chains and sets

Alex Rousskov wrote:
> On 06/27/2009 06:28 AM, Amos Jeffries wrote:
>>>> Limit adaptation iterations to adaptation_service_iteration_limit to
>>>> protect
>>>> Squid from infinite adaptation loops caused by ICAP services constantly
>>>> including themselves in the dynamic adaptation chain they request.
>>>> When the
>>>> limit is exceeded, the master transaction fails. The default limit of 16
>>>> should be large enough to not require an explicit configuration in most
>>>> environments yet may be small enough to limit side-effects of loops.
>
>> I see your comment at src/adaptation/AccessCheck.cc: ~156
>> TODO: should we shift and checkCandidates if and only if (!g) ?!
>>
>> If the TODO is about the case of self-reference you mention above then I
>> would say yes we must. To drop such loops into error condition as
>> quickly as possible. I understand the limit needs to be kept anyway for
>> multi-stage loops. But losing the loop early saves a lot of resources.
>
> No, this is different. I have not modified AccessCheck much. Its overall
> logic is as follows:
>
> 1. AccessCheck::check: Identify service groups that may be applicable
> to the virgin message. Start checking candidates by calling
> checkCandidates().
>
> 2. AccessCheck::checkCandidates: Get the top candidate and spawn a
> non-blocking ACL check for it. This ends this call. The non-blocking
> check will call us back in step #3.
>
> 3. AccessCheck::noteAnswer: If there is no match, remove the top
> candidate and go back to step #2. If there is a match, go to step #4.
>
> 4. AccessCheck::do_callback: Check that the top candidate service is
> still there and send it (or a NULL answer) to whoever requested the
> adaptation. Remove the top candidate from the list. Stop the job.
>
> You may have noticed that step #4 always terminates the search. Yet, we
> may not have found a valid service (e.g., due to a reconfiguration) and
> still have candidate services left to check. I added a TODO because it
> smelled fishy to me when I looked at the code, but my focus was not on
> changing how adaptation ACLs are processed so I left the code "as is".
>
> Your question prompted me to investigate this further and I am convinced
> there is a bug. If not everything is peachy in step #4, we must go back
> to step #2 again. I have now fixed this in my sources and polished
> surrounding code a little (there were other small problems). The TODO is
> done. The patch is attached.
>
> This bug is difficult to reproduce because the service must disappear
> while an ACL check is pending and it will only have an effect for checks
> pending before reconfigure; future checks will work OK again. It is
> probably possible to time an appropriate reconfiguration just right, but
> I have not tried to do that.
>
>
>> Also, should we prevent circular loops by way of preventing a filter
>> being run twice over the same request? There have to be few cases where
>> a single adaptation step needs to be repeated exactly.
>
> It would be nice to warn about the same service appearing twice in a set
> or chain (a patch for that is attached and I added it to my sources).
>
> I would not try to forcefully prevent duplicates though because it is
> not illegal and might be used in practice for "try me twice" poor-man
> backup implementations or for services that do different things
> depending on the message state but cannot do it all at once. I do agree
> that these situations are unusual (but very handy for testing! :-).
>

Okay. Fair enough.

>
>> IMO we are finding more and more reasons to add an ERR_*_CONFIG page :)
>
> In most cases, I would just quit (with an error message) if a
> configuration error is discovered during startup. However, I was
> overruled on that by the "start at any cost" crowd.
>
> Errors that are discovered at runtime should probably generate a fairly
> generic response with an error "code" or similar information for admins
> to match to cache.log entries and documentation. Users do not benefit
> from knowing that a Squid cache has a misconfigured ICAP service and
> many admins would not want to disclose that fact to end users anyway.
>

They have a point. We don't _have_ to make every config error fatal. But
we do need some way to make the admin aware of it such that they have
incentive to fix fast. Erros in cache.log are doing a lot but not enough.
Errors in their users faces saying they did wrong is a great incentive
to get it right, and a useful debug tool during pre-rollout tests.

But this is outside your patch scope. I should be starting a different
thread on this...

>
>>>> Fixed "canonical" Request URL maintenance when ICAP clones requests.
>>>> TODO: The urlCanonical() must become HttpRequest::canonical(), hiding
>>>> the
>>>> often out-of-sync canonical data member.
>> IMO: TODO: url cleanup bug needs closing to handle canonical and all
>> other URL display cases properly behind a single URL object. Requires
>> decent string code though or memory duplicates will blow out of
>> proportion to usefulness.
>
> Yes, those are two separate issues: cache consistency (my TODO) and
> cache storage implementation (TODO: add a URL or URI class). My TODO is
> much easier to implement, but was outside of this project scope.
>
>> I took a look at the patch, and decided to take your word that it works :)
>
> Yeah, it is difficult to find macro-level bugs in this code. It does
> work in my tests. I do not think we can get much further without wider
> testing.
>
> Should I interpret your comment as "I am not going to vote on this one"?

Um, well. I wrote that before deciding to read the code again for a
'quick' audit.

>
>> Please reference and close
>> http://www.squid-cache.org/bugs/show_bug.cgi?id=2087
>> in the commit if/when this goes in.
>
> I will try to remember that.
>
>
>> Some stuff I found in the code:
>>
>> in src/HttpRequest.cc please add brackets to make the code more readable:
>> "loggingNeedsHistory = LogfileStatus == LOG_ENABLE &&
>> alLogformatHasAdaptToken;"
>
> Done:
>
> const bool loggingNeedsHistory = (LogfileStatus == LOG_ENABLE) &&
> alLogformatHasAdaptToken;
>
>> 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.

>
>> 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

-- 
Please be using
   Current Stable Squid 2.7.STABLE6 or 3.0.STABLE16
   Current Beta Squid 3.1.0.9