Re: ACLChecklist reuse

On 20/05/11 06:35, Alex Rousskov wrote:
> Hello,
>
> While investigating a bug report, I have realized that ACLChecklist
> objects are currently meant to be used only once (for a either a fast or
> slow check). This limitation comes from a self-destructing nature of the
> check() method which can be sketched like this (Sketch A):
>
>> void
>> ACLChecklist::check()
>> {
>> while (accessList != NULL) {
>> if (matched)
>> return ...
>> accessList = next in accessList;
>> }
>> return ...
>> }
>
> Since accessList is a data member of ACLChecklist, the checking loop
> removes information with every iteration when accessList is set to the
> next list item.
>
> I do not know why ACLChecklist::check() and other similar ACL code was
> written like this, but I suspect it was just easier than preserving
> accessList items during the check and cleaning up only when the object
> is destroyed. Here is how that more complex code would look like (Sketch B):
>
>> void
>> ACLChecklist::check()
>> {
>> for (current = accessList; current; current = next) {
>> matched = ...
>> if (matched)
>> return ...
>> }
>> return ...
>> }
>
>
> I am omitting cbdata locking steps in both cases. While Sketch B
> destructor would need to unlock both accessList and current (if set), I
> do not think this is relevant to this discussion.
>
>
> I found Squid code that, under certain conditions, reuses an
> ACLChecklist object, with bad consequences. Our options are:

Where? What consequences?

>
> 1) Continue to use Sketch A design. Preserve/lock enough information
> about the request, response, connection, etc. to be able to create a new
> ACLChecklist object when it is needed in the rather low-level context
> where that object is being [re]used now.
>
> The primary problem with this approach is that we have to create and
> maintain a new "pre-ACLChecklist" object that is used only to create
> ACLChecklist objects, which already have the necessary code to
> preserve/lock information used in the checks.
>
> 2) Switch to Sketch B design so that any ACLChecklist object can be
> reused if needed.
>
>
> 3) Add an ACLChecklist member to switch between sketch A and sketch B
> behavior. The primary problem with this approach is that it is more
> complex than either #1 or #2. It would be more difficult to implement it
> correctly without changing a lot of current ACLChecklist class
> implementation at least a little.
>
>
> I would like to go with option #2 (Sketch B) because it is simple and
> yet allows ACLChecklist object reuse. I think it can be implemented
> without many changes by adding a initialAccessList or a similar data
> member so that the existing accessList member can still represent the
> current check and be re-initialized in the beginning of each check.
>
> Am I missing some problems with option #2? Do you see better solutions?
>
>
> Thank you,
>
> Alex.

ACLChecklist is a walker state. Walks over some caller determined
SquidConfig::accessList list over a series of async callbacks and time
delays.

As I understand it the intended form of re-use was to have one
ACLFilledChecklist which gets its accessList set to the start of each
*_access list at the beginning of a test.
   "Re-use" actually just being saved/cached lookup results in
ACLFilledChecklist data fields.
  When the test is run the caller may set a *new*
SquidConfig::accessList into the ACLChecklist and run a new test.

If the caller code needs it to scan the same access list twice then its
the callers responsibility to reset the accessList and run the second
check from scratch. Though why this would happen is beyond me, we never
share one accessList dataset between two directives.

  You will see re-use being done cleanly in the areas such as
header_access, tcp_outgoing_*, reply_body_max_size, access_log where
there are an array of accessList to be tested in series to find if the
directive should be enacted or not.

Amos

-- 
Please be using
   Current Stable Squid 2.7.STABLE9 or 3.1.12
   Beta testers wanted for 3.2.0.7 and 3.1.12.1