Re: [MERGE] Bug 2305 - Auth ref-counting upgrade

Hi Amos,

    If you want to avoid including auth/UserRequest.h and other "heavy"
headers just to get access to AuthUserRequest::Pointer and such,
consider creating auth/forward.h where you can typedef
AuthUserRequestPointer without pulling in full AuthUserRequest
declaration. We already have similar code in src/adaptation/forward.h

* I am concerned that done() is deleting/cleaning stuff in spite of the
object possibly still being in use by others. Elsewhere, this kind of
"early cleaning" approach led to bugs. Consider moving the actual
cleanup to the destructor (but it is not trivial because other code may
test for cleaned up state as an indication that the "end is close and we
should not do much").

* Consider renaming done() to detach() to emphasize that the object may
still be in use for a while.

* In the change below, can you leave the tighter name scope and remove
the extra parenthesis around (request->auth_user_request)?
- if (char const *name = auth->username()) {
+ char const *name = (request->auth_user_request)->username();
+ if (name) {

* A bunch of new files are named with a small first letter and with a
file name prefix duplicating the directory name, violating the file
naming rules. For example:

  === added file 'src/auth/basic/basicUserRequest.cc'
  === added file 'src/auth/digest/digestUserRequest.cc'

Do we want to do that for symmetry with nearby old files?

* There are many whitespace-only changes in the patch. Is that due to
the parent branch being behind on source formatting? This is not a big
deal, of course, but it makes an already large patch even bigger and,
hence, more costly to review. Please keep this in mind for future patches.

It looks like the patch simplified authentication code and may have
fixed a few nasty leaks/loops. I cannot verify all the details, but I
certainly do not object to it (I guess it is +0).

Thank you,

Alex.

On 05/23/2010 06:43 AM, Amos Jeffries wrote:
> This bundle updates the previous submission made on 29th April with a
> few additional fixes found during testing. The code has now been in
> production use for several weeks utilizing the Basic protocol and
> external_acl_type authenticators.
> NTLM, Negotiate, and Digest protocols have been through simple lab
> tests and passed but were not able to be used for live traffic.
>
>
> This branch:
>
> * implements proper RefCounting using the RefCount.h classes for
> almost all auth objects in Squid.
>
> * Restructures auth objects with a simpler structure of duties and scopes.
>
> * Prunes away several circular and indirectly circular pointer loops
>
> * Adds an API to auth config for handling the mainRotate() event. To
> only shutdown helpers, fixing the loss of cached credentials on rotate.
>
> * Adds a username_cache page to cachemgr interface to display the
> current usernames and their TTLs to various revalidation or garbage events.
>
>
> With this we end up with several global pointers for the auth schemes
> which have been built into the current Squid. These are RefCount
> pointers, fixing the leak of schemes on shutdown. Schemes are now also
> permanent structures for the runtime of Squid, fixing leaks on
> reconfigure and rotate actions.
>
> These AuthSchemes are responsible for creating auth Config objects for
> each auth protocol configured in squid.conf. These config objects are
> now also RefCounted as globals, fixing leaks on reconfigure and shutdown.
>
> Each HTTP request authentication attempt generates AuthUserRequest
> objects, which may or may not pointer to an AuthUser set of credentials
> being checked. AuthUserRequest is RefCounted instead of locked, fixing
> several assertion crashes.
>
> AuthUser is now RefCounted instead of locked. It's children inherit
> these properties. This simplifies the object handling a lot and fixes
> several assertions.
> This also means AuthUser no longer needs a back-pointer to all
> AuthUserRequest in order to see if its still needed alive, fixing one
> circular lock loop and a few possible assertions.
> The username cache pointers to only AuthUser objects, fixing a second
> cirular lock loop and potentially leakage. Also simplifying the hash
> cache handling a lot.
>
> Non-Auth code needing a reference to authentication credentials should
> hold a pointer to either an AuthUserRequest or AuthUser object. Not any
> other auth object.
>
>
> FUTURE WORK;
> There is still some conditions leading to auth re-challenge when they
> are not expected.
> A fair chunk of classes and enums have been shuffled into separate
> files to keep the scopes clearer. This could be increased in future when
> building the Auth namespace.
> Potential is now present for simpler TTL handling for all auth types.
>
>
>
> This work was a collaboration between multiple interested parties over
> the last year, with additional developer time funded by Netspace Online
> Systems.
>
> Amos
>
Received on Sun May 23 2010 - 17:26:39 MDT