> On Nov. 16, 2014, 7:22 nachm., 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 ;)
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 - Thomas ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121134/#review70468 ----------------------------------------------------------- On Nov. 16, 2014, 9:14 nachm., 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:14 nachm.) > > > 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 > >
