[Bug 43239] Tp::SharedPtr tries to be thread-safe, but secretly isn't

[bugzilla-daemon at freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=43239

--- Comment #3 from Andre Moreira Magalhaes <andrunko at gmail.com> 2011-12-02 08:12:08 PST ---
(In reply to comment #2)
> -    QWeakPointer<Connection> conn;
> +    QPointer<Connection> conn;
> 
> What on earth are these? QPointer was wholly and completely deprecated already
> in Qt 4.6 or so. Strange if it wasn't gone completely by the time Qt5 final is
> out. Note that *we can still use QWeakPointer for tracking QObjects getting
> deleted*, or use our own WeakPtr, now reintroduced, for that, whatever.
> 
> Other than the deprecation, there's supposedly a performance advantage to using
> QWeakPointer instead of QPointer. Promotion to strong ref (Tp::SharedPtr) from
> a weak ref is going to be even faster with our own WeakPtr though, but checking
> if the object is still there is going to be faster with QWeakPointer (requires
> an implicit temporary promotion to strong ref with WeakPtr/SharedPtr). And weak
> ptr -> strong ptr is only thread safe with our own, but yeah for pointers which
> are completely hidden inside tp-qt4, that's not an issue.
> 
> So given those considerations, try and decide between QWeakPointer and
> Tp::WeakPtr as appropriate, but never use QPointer.
Yeah, as for QPointer, I don't know what I was thinking, but I should have kept
using QWeakPointer. I will revert this specific changes. 
The thing is that some places, private class constructors mainly, cannot store
a WeakPtr of the "parent" class, as our WeakPtr impl cannot be created from a
bare pointer, it has to be created from a SharedPtr which would unref itself in
the own constructor, thus deleting the object.
So yeah, I checked everything and the places using QPointer now have to be
changed to use QWeakPointer but cannot use WeakPtr. I've changed all other
places that can use WeakPtr safely to properly use it.

> +    }
> +    inline virtual ~RefCounted()
> +    {
> 
> empty line
Will do

> +
> +class TP_QT_EXPORT SharedCount
> +{
> 
> Make that a private class inside RefCounted (with friends as needed) or at
> least somehow hide it from Doxygen. Currently it's an exported public class in
> a public header... not desirable. (It needs to be exported though if it is
> created and destroyed by inline code)
Will see what I can do, but I think I prefer it to be hidden from doxygen, as
well as the public "sc" var from Tp::RefCounted. We need them public as we
cannot friend templates (SharedPtr<T>, WeakPtr<T>) from non-template classes.
Or can we?

> -    explicit inline SharedPtr(const QWeakPointer<T> &o)
> +    explicit inline SharedPtr(const WeakPtr<T> &o)
> 
> Please document separately that we've removed the QWeakPointer constructor
> because it wasn't thread safe and if people are absolutely sure that their code
> doesn't EVER need multithreading for ANYTHING they can still construct
> SharedPtrs from them by getting the raw pointer.
So where to add this documentation? Or you mean a commit log, NEWS, or
something?

> +        SharedCount *sc = o.sc;
> +        if (sc) {
> +            // increase the strongref, but never up from zero
> +            // or less (negative is used on untracked objects)
> +            register int tmp = sc->strongref.fetchAndAddOrdered(0);
> +            while (tmp > 0) {
> +                // try to increment from "tmp" to "tmp + 1"
> +                if (sc->strongref.testAndSetRelaxed(tmp, tmp + 1)) {
> +                    // succeeded
> +                    break;
> +                }
> +                // failed, try again
> +                tmp = sc->strongref.fetchAndAddOrdered(0);
> +            }
> +
> +            if (tmp > 0) {
> +                d = dynamic_cast<T*>(sc->d);
> +                Q_ASSERT(d != NULL);
> +            } else {
> +                d = 0;
> +            }
>          } else {
>              d = 0;
>          }
> 
> You'll have to explain to me if and WHY this works. In some other fashion than
> saying "it's from Qt which has a similar pointer concept". Similar, but not
> identical.
> 
> Have you considered what happens if the corresponding RefCounted destructor or
> SharedCount destructor even more specifically happens to be executed in ALL
> points of the execution of that loop (including its condition)?
> 
> Didn't look at the test thoroughly yet. But as it's not deterministic, I want
> to make sure we first convince ourselves by thinking about the properties of
> the code that it should work. There's only a few lines of (the critical parts
> of) it anyway!
As long as we have at least one WeakPtr that ever pointed to a valid (non-null)
SharedPtr, "WeakPtr::sc" or "o.sc", will exist. So sc will either be null or
non-null during the whole method, won't change due to threads.
If "sc" is non-null, let's see if it is still pointing to a valid data. In
order to check we need to get the number of strongrefs and check if we can
atomically increase from "current value" to "current value + 1", with "current
value" being > 0 (at least one strongref before the test). testAndSetRelaxed
will atomically test if the "current value" is still the same ("tmp") and if so
increment it by 1 returning true, if not, nothing is changed, which means that
on success we hold a strongref and are guaranteed to be pointing to a valid
data.

-- 
Configure bugmail: https://bugs.freedesktop.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA Contact for the bug.