Make mutex lock routines deal with null mutex and general clean up.

[shinrich]

The original change I made was to MUTEX_TRY_LOCK and underlying classes to handle the case when the mutex is null. We were getting some rather convoluted code to deal with scoped lock pointers and the two cases where the mutex was null or not. And ultimately ran into an internal change that messed up the scoped pointer logic introducing a small window locking error. We have been running that logic on our internal build for the past week.

As I was pulling this PR together I noticed the following comment from HttpSM

/* The MUTEX_TRY_LOCK macro was changed so
     that it can't handle NULL mutex'es.  The plugins
     can use null mutexes so we have to do this manually.
    We need to take a smart pointer to the mutex since
    the plugin could release it's mutex while we're on
    the callout
 */

So it appears at some point (before the first commit to git in 2009), the logic to deal with null mutexes was pulled from the MUTEX_TRY_LOCK area. I don't know why. The null check seems like pretty low overhead.

Then I noticed the second part of the comment about needing to take the smart pointer in case the continuation released the mutex along the way. Looking into I_Lock.h it wasn't clear why we had versions that stored the ProxyMutex pointer directly and others that stored the Ptr, so I got rid of the raw pointer version and the spin lock version which doesn't seem to be used. This required some fixes through the code (mostly in HostDB) to stop calling the .get() method on the Ptr before making locking calls.

The one point that needed a direct ProxyMutex * interface was through the TSAPI since that is a C interface. But if we have TSMutexCreate bump the reference count before returning the ProxyMutex * and TSMutexDestroy correspondingly decrement the reference count, we should be able to put the InkAPI provided ProxyMutex * back into the smart pointer for core processing. This was found through the "make check" unit tests.

[SolidWallOfCode]

You should be able to drop the get here and do mutex == old_bucket.mutex. The Ptr class overloads the equality operator to compare the internal pointers.

[SolidWallOfCode]

You should be able to use auto here, e.g.

auto mutex = thread->mutex;

I will have to defer to @zwoop if that's stylistically preferred.

[SolidWallOfCode]

I wonder if we really need this - might it not be reasonable to enable the normal reference counting to work here? In fact, this might obviate the whole point of doing the reference couting in TSMutexLock, the purpose of which is to preserve the mutex over destruction by the plugin. That seems like to assert now.

[shinrich]

Talked with @SolidWallOfCode and indeed, we should not explicitly free here. Rather just decrement the ref count and wait for the smart pointer logic go to 0 and free. If there is some other element of the code holding a smart pointer at this point, it would be wrong to explicitly free here.

[SolidWallOfCode]

Very nice, really cleans up the code.

[shinrich]

Pushed new version to address @SolidWallOfCode's comments.

[shinrich]

[approve ci autest]

[shinrich]

[approve ci autest]

[oknet]

@SolidWallOfCode @shinrich

The plugins which hook on global hooks can use null mutex to handle concurrent request.
Does the plugins are required to have a mutex ?

[shinrich]

Correct, the continuations are only required to have a mutex if they are using the scheduling APIs. If cur_hook->m_cont->mutex is null, the changes to MUTEX_TRY_LOCK will just create a lock object that says it is locked. It will not try to dereference the null mutex.