> 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).
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?
- 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, 9:22 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, 9:22 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
>
>
