[squid-dev] [RFC] [PREVIEW] LockingPointer round 3.

[Alex Rousskov]

On 07/21/2016 07:58 AM, Amos Jeffries wrote:
>      void resetWithoutLocking(T *t) {
> +#if USE_OPENSSL
> +        assert(!t || t->references > 0);
> +        assert(!raw || raw->references > 0);
> +        if (raw && t == raw) {
> +            assert(raw->references > 1); // us plus caller locks
> +        }
> +#endif
>          unlock();
>          raw = t;
>      }

The first assert() is correct.

The second assert() belongs to unlock() where it is already present.
Remove it from here.

The third assert() comment implies that self-reset is not supported (and
will assert) in some cases, but other self-reset cases are fine. If that
is what you are proposing, you should be explicit about it. Needless to
say, I am against such inconsistent behavior and recommend removing that
third assert.


> If you are referring to the self-reset assert(t != raw) and so adamant
> that is right

I spent many hours discussing why such assert would be wrong [when the
other method has the opposite do-nothing implementation]. And now you
think I may consider it being correct and even call me adamant. Sigh.


> OpenSSL 1.1.0 is making a lot of
> things opaque and that includes the reference counting. We may not be
> able to access t->references for long.

Which is why it is probably a good idea to add MustBeLocked(const T*) or
AssertIfUnlocked(const T*) or similar static method (that does nothing
if it cannot check) instead of sprinkling code with #ifdefs and explicit
accesses to references. If the asserts above are fixed, one such method
would suffice AFAICT.



>>> // XXX: why not set fd_table if that add() succeeded ?

>> The simple answer is that if you set fd_table after cache add()ing
>> succeeded, then both fd_table and the cache will delete the same
>> pointer, crashing Squid. Note that ctx is a raw pointer [to a smart
>> pointer]. However, I doubt this is the correct/best answer.
>>
>> AFAICT, the poor-quality comments in the original code were supposed to
>> answer that question like this: The _only_ reason we add ctx to the
>> table is so that it gets eventually destroyed. The context object is not
>> needed in the table and should not be accessed from there. If we
>> successfully add ctx to the cache, then the cache becomes responsible
>> for its destruction and we do not need to add it to the table.


> So that means the fd_table member can be set *always*

Probably, but this is not my area of the expertise.


> On the other hand. I suspect that the use fd_table is being put to is
> better served by one of the MasterXaction / AccessLogEntry /
> ConnStateData objects relevant to the transaction using that context.

Not MasterXaction or AccessLogEntry -- the context is specific to the
connection, not transaction. ConnStateData is a good candidate but you
may find it difficult to share with those who need the context.

Pretty much any use of fd_table outside Comm code is a bug, but fixing
that bug is difficult.


> +            // fd_table is now responsible for reference counting of the pointer.

This comment is misleading. Both fd_table and the cache (if any) are
responsible. IMO, there is no need to retell what the code already
clearly says so I recommend removing that comment.


> +                    delete tmp; // dont leak tmp on failures

I recommend deleting that comment. It is misleading (more than just tmp
will leak without that delete) and misspelled. You may want to replace
it with "XXX: leaks on exceptions" or, better, use std::unique_ptr (and
release() it on add() success) to prevent those leaks.

> +    /**
> +     * Construct directly from a raw pointer is forbidden.

s/Construct/Constructing/ or s/Construct/Construction/


> +     * Use the appropriate reset method instead.

This does not make sense -- if I need a _constructor_ I cannot use a
regular method.


> +    explicit LockingPointer(T *) = delete;

Did you mean to prohibit all from-pointer construction or just the
explicit one? If it is the former, I suggest removing "explicit".


> How about?
> 
>   static SelfType&& MakeFromLockedOpenSslPointer(T *t) {
>       SelfType out;
>       out.resetWithoutLocking(t);
>       return std::move(out);
>   }
> 
> 
> Theres something I'm not quite understanding going on with the
> return-by-value for template types giving me compiler errors. Thus the
> std::move, it seems to build but I'm not sure if it will run properly at
> all.

I strongly recommend against writing optimization code you do not fully
understand. Add a simple static method that returns a SelfType instead.

This particular method clearly has nothing to do with OpenSsl. Using
OpenSsl in its name but calling it in GnuTLS-compiled code is very
confusing IMO. I recommend removing OpenSsl letters from its name. If
you want to restrict this method use to OpenSSL builds (for now), then
surround it with #ifdefs and document why others should not use it.


The word "Make" does not seem to add any value so I recommend removing
it from the method name:
   CrlPointer::FromLockedOpenSslPointer() is as clear as
   CrlPointer::MakeFromLockedOpenSslPointer()


> identifying this ambiguous use was the point of
> the comment experiment.

It looks like the experiment is still ongoing. If not, please remove
stale experimental comments.


Thank you,

Alex.