[squid-dev] [PATCH] TidyPointer removal

[Alex Rousskov]

On 07/08/2016 12:44 AM, Amos Jeffries wrote:
> On 8/07/2016 11:24 a.m., Alex Rousskov wrote:
>> We may also want to hide the constructor behind two static methods for
>> similar reasons -- whether X is properly destructed in the following
>> example depends on createX() details, and it really should not:
>>
>>   {
>>       LockingPointer lp(createX());
>>   }
>>

> In the context of how we agreed to shrink the library specific #if
> wrapped parts down to specific action nuggets as much as possible. eg
> how the session resume functons are defined:
> 
> I have been thinking the abstract API should use Pointer instead of Ptr
> so the minimal #if part can do that library-specific initialization of
> the Pointer and avoid constructing these things in generic code.

Your conclusion may be correct, but for different and much simpler
reasons: If parts of code need smart pointers to object X, then all code
must use smart pointers to object X. It is virtually impossible to write
sane and safe code that mixes dumb and smart pointers to the same
object. This is a universal principle.


>>> +    void resetWithoutLocking(T *t) {
>>> +        unlock();
>>> +        raw = t;
>>>      }
>>>  
>>>      void resetAndLock(T *t) {
>>> +        if (t != get()) {
>>> +            resetWithoutLocking(t);
>>> +            lock(t);
>>> +        }
>>> +    }
>>
>> We should probably add self-reset protection to both cases or to none.


> Strange as it seems doing that would violate the non-locking guarantee
> on the first one.

How does the following code violate the non-locking guarantee?

    void resetWithoutLocking(T *t) {
        if (t != get() {
            unlock();
            raw = t;
        }
    }


> Both reset*() methods (and assignments) guarantee that the UnLocker is
> called on any previously stored value.

Self-assignment and self-reset is a gray area. Each
assignment/reset-handling class can define what should happen under
those circumstances. The possible sane definitions are:

  1. Do nothing (side effects lead to leaks/crashes in non-benign cases)
  2. assertion/exception (explicit error in all cases)
  3. Do the usual (side effects lead to leaks/crashes in most cases)

Which definition to pick is our choice, but we should be _consistent_ in
that choice. I recommend #1, but it is a weak recommendation.

In the latest patch I have seen, resetAndLock() was using #1 but
resetWithoutLocking() was using #3.



> If that means the raw-pointer is
> about to go invalid then its a caller bug for using that method of the
> API. Because resetWithoutLocking() fully delegates lock control to the
> caller this is not something we can make assumptions about in the
> resetWithoutLocking() variant.

Self-assignment and self-reset is always a caller problem and is always
a gray area -- logical reasoning cannot arrive at a single "correct" or
"best" solution here.

A caller has delegated control of the already managed object locks to
us. Now the caller wants us to import the same object that was allegedly
pre-locked for our import. It is our decision whether to unlock/free the
object (#3), assert/throw (#2), or do nothing (#1).

I doubt we should pick #3, but that is not what I am complaining about
right now. I am complaining about inconsistency. If you are sure that
crashing the program in most cases is the best solution in the
LockingPointer context, then you should use that solution for all
reset*() and assignment methods.


> Doing a self-check to retain raw-pointer validity would behave the same
> as if it had locked the pointer given.

You cannot know what self-check side effects will be because they depend
on the caller. Thus, any assertion that "do nothing" would behave the
same as "do something" is wrong.

For example, the following benign code triggers self-reset but has no
side effects at all if self-reset is handled using the "do nothing"
approach:

    p.reset(p->get()); // p = p


> the non-TLS builds UnLocker will do
> nothing and T will just be a void or int placeholder.

That does not sound quite right to me: If we want TLS-incapable code to
use LockingPointer for TLS stuff that cannot exist in such code, then
UnLocker and lock() ought to assert! In other words, only nil
LockingPointers should be allowed in that special case.


>> I do not recall any remaining in-scope LockingPointer issues except that
>> the constructor does not force the caller to pick the right importing
>> route. What issues did I forget about?

> The absence of locks for GnuTLS 

GnuTLS support is out of this change scope, I hope.


> and the need to use LockingPointer without actual locking 

Inside GnuTLS builds, you mean? Or is that a separate problem from the
insufficient GnuTLS support in Squid? If it is the latter, please detail it.


> Okay. Build testing the final version underway and will merged to trunk
> when thats done.

Please resolve self-assignment/reset consistency, before or after that
merge.


Thank you,

Alex.