[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

Olli Salli <ollisal at gmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |ollisal at gmail.com

--- Comment #2 from Olli Salli <ollisal at gmail.com> 2011-12-02 07:19:20 PST ---
-    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.

+    }
+    inline virtual ~RefCounted()
+    {

empty line

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

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

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

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