[KDE/Mac] Review Request 121134: make KGlobal reference counting threadsafe

[René J.V. Bertin]

> On Nov. 16, 2014, 8:22 p.m., Thomas Lübking wrote:
> > kdecore/kernel/kglobal.cpp, line 343
> > <https://git.reviewboard.kde.org/r/121134/diff/5/?file=328836#file328836line343>
> >
> >     Does this actually make sense?
> >     The result is "undefined" when used from multiple threads anyway.
> >     
> >     Should one enforce this to be called from the main thread only instead?
> 
> René J.V. Bertin wrote:
>     How do you mean, undefined? The result will be the value set by whoever called the function last.
>     
>     You have a point that this doesn't make a lot of sense, there's something to say that the main (master...) thread ought to be the one deciding whether or not it can be quit by the reference counter.
> 
> Thomas Lübking wrote:
>     > How do you mean, undefined?
>     
>     It's a write-only flag. If written from two threads with disagree, none has an idea about the current state - and there's no getter either.
>     
>     Since the client code somehow has to find agreement on the outcome, it needs a local mutex - or define who's in position to write this ("the one deciding whether or not it can be quit by the reference counter")
> 
> David Faure wrote:
>     A local mutex around that one line would be completely pointless. The effect will be exactly the same: no data race (atomic operations don't participate in data races), but what I call a "semantic" race, since the last writer wins.
>     
>     However, let's not invent problems. setAllowQuit is always called by the main thread, and only with true as argument (i.e. this is about moving from the initial "don't quit yet it's too early" to the "ok, normal operations start here").
>     The only reason for an atomic int here is so that the reading by secondary threads doesn't lead to a data race (with the write by the main thread).
> 
> René J.V. Bertin wrote:
>     Indeed.
>     
>     So how about
>     
>     ```C++
>     void KGlobal::setAllowQuit(bool allowQuit)
>     {
>         if (QThread::currentThread() == qApp->thread()) {
>             s_allowQuit.fetchAndStoreOrdered(int(allowQuit));
>         }
>         else {
>             qWarning() << "KGlobal::setAllowQuit may only be called from the main thread";
>         }
>     }
>     ```
>     
>     but then if the action can be refused without feedback to the caller we would maybe need a getter?
> 
> David Faure wrote:
>     I don't see the need for a getter (what would the app do with the value...).
>     
>     About the main-thread-check... well, can't hurt, but it doesn't matter because the current code doesn't do that AFAIK (unless kdevelop is being really funky), and this whole KGlobal refcount feature is gone (deprecated) in KF5, replaced with QEventLoopQuitLocker. IOW I'm OK with this if it appeases your fears of API misuse ;)
> 
> Thomas Lübking wrote:
>     I meant "local, in the client code" - so the client would still have a deterministic idea on the value.
>     
>     > The only reason for an atomic int here
>     
>     Ah, the ::deref() check (against late -and idempotent- writes). Makes sense, yes.
>     
>     > we would maybe need a getter?
>     
>     one could probably also just abort. The function *should* apparently not be even called before any off-thread or nested ref/deref starts
> 
> David Faure wrote:
>     Qt typically warns rather than aborting when doing something like this (e.g. sendPostedEvents on object living in another thread, stopping timers from wrong thread, etc.). Otherwise akonadi and plasma wouldn't run very long, ahem ;)
>     
>     I think the warning is good enough, the app can proceed anyhow, making users happier.

I couldn't agree more.

After all, this whole exercise started for me because of an application quitting for no (user-)apparent reason. ;)


- René J.V.


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/121134/#review70468
-----------------------------------------------------------


On Nov. 16, 2014, 10:14 p.m., René J.V. Bertin wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/121134/
> -----------------------------------------------------------
> 
> (Updated Nov. 16, 2014, 10:14 p.m.)
> 
> 
> Review request for KDE Software on Mac OS X, kdelibs and David Faure.
> 
> 
> Repository: kdelibs
> 
> 
> Description
> -------
> 
> I have been experiencing unexpected exits from KDevelop that were not due to any kind of error in the KDevelop code; it was as if someone told the application to exit normally. This happens mostly on OS X, but also sometimes on Linux.
> I finally traced this to `KGlobal::deref` reaching a zero reference count and invoking `QCoreApplication::quit` when called from one of KDevelop's KJob dtors. There does not appear to be a reference counting mismatch in the code, so the issue might be due to a race condition in KGlobal::ref/deref.
> 
> This patch introduces thread-safety to KGlobal's reference counting by turning the simple global `static int s_refCount` into a `static QAtomicInt s_refCount`. I consider this an important bug fix regardless of whether it corrects the issue I have with KDevelop.
> 
> 
> Diffs
> -----
> 
>   kdecore/kernel/kglobal.cpp cf003a4 
> 
> Diff: https://git.reviewboard.kde.org/r/121134/diff/
> 
> 
> Testing
> -------
> 
> On OS X 10.6.8 only for now.
> 
> NB: Qt itself uses the QAtomicInt's parent class (QBasicAtomicInt) for its own reference counter used for implicit sharing. This is thus well-tested code that is unlikely to introduce regressions to a core KDE feature.
> 
> 
> Thanks,
> 
> René J.V. Bertin
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-mac/attachments/20141116/2d345661/attachment.html>