[squid-dev] [PATCH] TidyPointer removal

[Alex Rousskov]

On 07/07/2016 01:40 PM, Amos Jeffries wrote:
> On 4/07/2016 5:39 a.m., Alex Rousskov wrote:
>>> +    /// Reset raw pointer - delete last one and save new one.
>>> +    void reset(T *t) {
>>> +        deletePointer();
>>> +        raw = t;
>>>      }


>> I do not think we can have a reset() method like this because the
>> standard shared_ptr::reset() has a very different semantics. Let's call
>> this absorb(). This is like a raw pointer assignment operator, but we
>> want to keep it "explicit" because we think this is a dangerous operation.


> What different semantics do you see in there?

Good question! This issue is confusing for me, so I apologize if this
explanation is messy:

Facts:

* resetWithoutLocking(x) does not increment the reference counter for x.
* resetAndLock(x) increments the reference counter for x.

What does the standard shared_ptr::reset(x) method do?

* If shared_ptr counts all references to x, internal and external, then
shared_ptr::reset(x) is like our resetAndLock(x) -- it increments the
counter.

* If standard shared_ptr only counts external references, then
shared_ptr::reset(x) is like our resetWithoutLocking(x) -- it does not
increment the counter.

This logic is totally flawed, on several levels, but this trap might
help illustrating why I find this issue confusing.

To fix our logic, we should not misrepresent what shared_ptr does
internally, but look at its API instead. The standard API guarantees
that the following sequence leads to object X destruction:

  {
      std::shared_ptr s;
      X *x = createX();
      s.reset(x);
      // s is destroyed upon exiting its scope
  }

Which of our two LockingPointer::reset*() methods provides the same
guarantee?

The correct answer is: None! In our case, the reset*() caller has to
know createX() details to call the right reset*() method, even though
createX() returns a "raw" pointer that knows nothing about our
LockingPointer. Similarly, the caller must know createX() details to
decide whether it is safe to call the LockingPointer constructor!

This mess happens because, in some cases, OpenSSL effectively returns an
already managed pointer, even though it does not know about our
LockingPointer. Or, in other words, it happens because we [have to]
share the pointer management mechanism with OpenSSL, but that mechanism
does not come in a form of a smart pointer -- it is hidden inside the X
object itself.

With a standard API, there is only one reset(x). In our case, we have to
have two. Thus, none of them can be called reset()! We have to force the
caller to make a choice, depending on the context (i.e., the source of x).

BTW, your decision to name both methods reset*() is superior to my
earlier suggestion to name them absorb() and reset(). Let's leave your
reset*() names intact.

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());
  }



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


> +    void unlock() {
> +        if (raw)
> +            UnLocker(raw);
> +        raw = nullptr;
> +    }

Let's move assignment inside the if-statement to save a few CPU cycles.


> +    T *raw; ///< pointer to T object or nullptr

That description adds virtually no information. How about something like
this: "Normally, no other code will have this raw pointer. However,
OpenSSL may keep raw pointers and manage their reference counts. When
OpenSSL shares a pointer with us, the caller must not use LockingPointer
constructor and must call resetAndLock()."


> +    explicit LockingPointer(T *t = nullptr): raw(t) {}

I recommend calling resetWithoutLocking() here even though it could
waste a few CPU cycles. This is the only dangerous part remaining in the
LockingPointer interface, and it may help being explicit here.


>  typedef void* SessionPtr;
> +CtoCpp1(xfree, SessionPtr);

xfree() is already a C++ function and does not need wrapping, right?




>> Please also add a public clear(void) method that reset(t) will call
>> instead of lock() when t is nil. Alternatively, add a reset(void) method
>> with the same semantics as clear(void) -- that is what std::shared_ptr does.
> 
> I do not see any clear() method in shared_ptr documentation.
> <http://en.cppreference.com/mwiki/index.php?title=Special%3ASearch&search=clear%28%29>

Yes, I know. As I said, they provide a reset(void) instead. If you think
we should add reset(void) instead of clear(), let's add that, but please
do add one instead of [ab]using reset*(nullptr).


> + * The lock() method increments Object's reference counter.
> + *
> + * The unlock() method decrements Object's reference counter and destroys
> + * the object when the counter reaches zero.

Please remove or move/merge these descriptions with their corresponding
private methods descriptions. No need to place this private info in the
class description. Sorry I did not mention that earlier.


>> Please note that the originally proposed commit message no longer
>> applies to your changes so we need a new one.
>>
> 
> "
> This replaces TidyPointer with std::unique_ptr and re-implements the
> part of TidyPointer which LockingPointer was using. Removing the
> inheritence in the process and updating methods names for clarity.

No serious objections, but I suggest:

-----------
Replaced TidyPointer with std::unique_ptr.

LockingPointer is now a stand-alone class that is understood (and
documented) as a typical shared pointer with OpenSSL-friendly object
importing methods. Replaced its TidyPointer::reset() abuse with an
explicit resetWithoutLocking() method after reviewing that all such
calls needed no locking indeed.
-----------


> The LockingPointer still has some of the issues which led us down the
> path to getting here. I've chosen to submit for review at this point to
> ensure we still have a fully working Squid before going further down
> this trail.

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?


All of the above changes do not require another review round IMO.


Thank you,

Alex.