[squid-dev] [PATCH] Fix for bug 4190

[Alex Rousskov]

On 09/04/2015 10:50 PM, Kinkie wrote:
> Hi all,
>    the attached patch is a proposed fix for bug 4190 (and then some).
> It transforms the global hash_table proxy_auth_username_cache into
> four per-authentication-scheme std::unordered_map-backed caches,
> leveraging the RegisteredRunner API and SBuf.


> + * \returns a pointer to cached credentials, or nullptr if none found
> + */
> +class UserNameCache : public RegisteredRunner

Stale comment? A C++ class cannot return something.


> +/** Cache of Auth::User::Pointer, keyed by Auth::User::userKey
...
> +class UserNameCache : public RegisteredRunner

Why is this called UserNameCache? It looks like this is a _user_ cache
(with all the authentication information we want to keep about the
user), not user _name_ cache (with just the user name).


> +class UserNameCache : public RegisteredRunner

Please separate cache operation (the UserNameCache class) and
external-to-caches cache object creation/cleanup/destruction/etc.
initiation code (creating a dedicated child of RegisteredRunner for
that). This is a good idea in general because it simplifies code, but it
is especially desirable here because you have multiple caches but need
only one Runner object to initiate their creation/cleanup/destruction/etc.


> +    UserNameCache() = delete;

Do not delete something that does not exist.


> +    /** obtain sorted list of usernames
> +     *
> +     */
> +    std::vector<Auth::User::Pointer> sortedUsersList() const;

Sorted how? Please check other occurrences of new std::vectors in the
patch. Always define how they are indexed or sorted. It is much easier
to supply that info now than correctly restore it later.

Please typedef std::vector<Auth::User::Pointer> as Auth::Users or
similar and avoid repeating that expression many times.


> +    ~UserNameCache() = default;

This should have "virtual" keyword in the current patch. May not be
necessary after RegisteredRunner parent is removed.


> +    // c_str() raw pointer is used in event. std::string must not reallocate
> +    // after ctor and until shutdown
> +    // must be unique
> +    std::string cacheCleanupEventName;

The troubles with this "pray that it works" design are not worth the
gains IMHO. If you think the event name must be "User cache cleanup:
digest" instead of just "digest", "Digest user cache", or similar, then
pass a beautiful event name as the second UserNameCache constructor
parameter, supplied by UserNameCache kids.


> +void
> +UserNameCache::Cleanup(void *data)
> +{
> +    debugs(29, 5, "checkpoint");
> +    if (!cbdataReferenceValid(data))
> +        return;

I believe you are using eventAdd() with cbdata protection so the
reference has to be valid. If I have not missed some case where data may
be invalid, please remove this if statement.


> +    eventDelete(&UserNameCache::Cleanup, this);

This call will lead to debug_trap() if the event callback happens to be
scheduled already (and, hence, out of the event queue but not fired
yet). There is currently no safe way to delete events with arguments,
especially long-lived ones like cleanup. Do not delete them if possible.

If the cache is no longer needed, delete the cache object itself. This
will prevent the cbdata-protected event from reaching the cache object.
If you cannot delete the cache object because of the bugs elsewhere,
perhaps you can use cleared store_ as a sign that the event should not
be processed any more?

Or, alternatively, perhaps you can let the event fire (and just not
schedule the next one if there is nothing to cleanup)? Which brings me to:


> +UserNameCache::UserNameCache(const char *name) :
> +    cachename(name), cacheCleanupEventName("User cache cleanup: ")
> +{
> +    debugs(29, 5, "initializing " << name << " username cache");
> +    cacheCleanupEventName.append(name);
> +    eventAdd(cacheCleanupEventName.c_str(), &UserNameCache::Cleanup,
> +             this, ::Config.authenticateGCInterval, 1);

It may be better to avoid scheduling cache cleanups until there is
something to cleanup. This will require storing "we have scheduled a
cleanup" boolean, but it may make help folks that deal with Squids which
are idle for extended periods of time but still wake up every second [to
do nothing].


> +    eventAdd(self->cacheCleanupEventName.c_str(), &UserNameCache::Cleanup,
> +             self, ::Config.authenticateGCInterval, 1);

> +    eventAdd(cacheCleanupEventName.c_str(), &UserNameCache::Cleanup,
> +             this, ::Config.authenticateGCInterval, 1);

Please avoid code duplication.


> +void
> +UserNameCache::Cleanup(void *data)
> +{
> +    debugs(29, 5, "checkpoint");
...
> +    // data is this in disguise
> +    UserNameCache *self = static_cast<UserNameCache *>(data);

And now it is time to call self->cleanup(), to avoid doing a lot of
stuff inside the static method when you have a perfectly good object
available.


HTH,

Alex.