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

[Amos Jeffries]

[kinkie: looks like these may take a while to resolve. If you want to
push the branch to lp:~squid I can pick up the editing now]

On 6/09/2015 4:40 p.m., Alex Rousskov wrote:
> 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.
> 

Should be on the lookup() method, and with s/nullptr/nil/

> 
>> +/** 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).
> 

Legacy: it caches Auth::User keyed by-name.

If we are going to change this it should probably be to
"CredentialsCache" I think.

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

No. We cant have one runner.

The only safe way to check if a Cache() call is usable without creating
a whole new cache as side effect is to check the current scheme config.

With one runner we hit a race condition between the scheme config being
re-configured and the runner needing to decide if the cache is worth
re-configuring or deleting.

It would also mean even more hard-coding of what schemes exist and need
to be managed by the runner, when schemes are supposed to be plug-n-play
components. AIUI the whole point of the runner API was to avoid that
kind of hard-coding.

We can have N split runners controlling a cache each. Just doing nothing
if a CbcPointer they hold is invalid or null. That should work. But not
one runner juggling multiple optional schemes.

Doing the split though leads to direct code duplication. Since the
runner is just calling 1:1 methods on the cache, both have to have
identically matching lifetimes and ctor/dtor sequences need to line up
their actions with inverse dependency :: creating cache creates runner,
deleting runner deletes cache, creating runner early or deleting cache
early causes problems).


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

Squid crashes on shutdown without this. It seems due to the event firing
race you mention below. Though this was fixed before a bug in validating
the cache objects was found, so it may have been a miss-fix.

The above line is what implements the "dont do anything" logic when the
event *does* reach the static callback.

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

We can only delete reliably on final shutdown due to cachemgr things
needing access to Cache(), which will re-allocate it all again uselessly.

Which is part of the reason Cache was inheriting from RunnerRegistry. We
have to have one runner per cache anyway, and the registry default
finshShutdown() takes care of all the deletion and cbdata invalidations
at the correct times without any new code.

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

We were trying to avoid behavour changes as much as possible in this
round. I like though.

> 
>> +    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.
> 
> _______________________________________________
> squid-dev mailing list
> squid-dev at lists.squid-cache.org
> http://lists.squid-cache.org/listinfo/squid-dev
>