> On Nov. 16, 2014, 3:58 p.m., Milian Wolff wrote:
> > as I said before: Did you check whether that is actually the case? Did you
> > add some assertions to see what other thread is calling this code?
> >
> > I'm not sure whether this is supposed to be threadsafe or not. If it must
> > be threadsafe, you'll also need to make s_allowQuit threadsafe
> > (QAtomicBool).
>
> René J.V. Bertin wrote:
> I have indeed looked at this aspect, but not exhaustively and haven't
> found evidence of cross-thread reference counting. Yet.
> It's unclear what you mean with "that" (the thing actually the case). It
> "that" refers to the quitting issue, I can of course never prove that it is
> due to a reference counting race condition because the issue isn't
> reproducible reliably. I can only invalidate the idea that it is due to such
> a race condition, whenever it happens again with properly protected reference
> counting.
>
> I think we have to leave it up to the kdelibs maintainers/authors to
> decide whether or not this has to be thread safe. The doxygen documentation
> for KJob does not mention anything about thread safety. It does however note
> that *KJob and its subclasses is meant to be used in a fire-and-forget way.
> It's deleting itself when it has finished using deleteLater() so the job
> instance disappears after the next event loop run.* . It is not said that a
> KJob cannot use a KJob itself, which could lead to concurrent reference
> (de)counting, nor whether a KJob instance is guaranteed to be deleted in the
> same thread it was created, and I think that a *concurrent processing* class
> that does not explicitly mention it does thread unsafe things can be assumed
> to be thread safe.
>
> There's no QAtomicBool, btw.
>
> Milian Wolff wrote:
> As I told you before in other patches you send in: Please make sure you
> find the actual reason before changing other code in the hope of it improving
> things. If there is no hard evidence that your bug manifests itself because
> of a race in KGlobal::ref/deref, then don't change it. Adding an assertion to
> the code that its only accessed from the mainthread is done easily and should
> be hit reliably (contrary to a race condition or lock contention that Thomas
> proposes).
>
> And also note that a KJob is not neccessarily implemented using threads.
> You can also use e.g. QIODevice or external processes internally to get the
> desired behavior.
>
> René J.V. Bertin wrote:
> KJobs not using threads to implement concurrency will not cause race
> conditions in the reference counting; either they are in separate processes
> or they live in the same thread.
>
> Let me repeat once more what I also wrote in reply to Thomas below: a
> feature (like reference counting) of a framework that is frequently used in
> multithreaded applications can be expected to be thread safe. That's why my
> original description states that I consider the fact that it isn't an
> important bug regardless of whether it's the cause of my issue. Regrettably I
> am a scientist trained to give background anytime I propose something, I keep
> forgetting that there are contexts where that actually turns out
> counterproductive :(
>
> You suggest to use an assert or other detection of doing reference
> counting off the main thread, and doing so imply that reference counting
> should apparently happen only on that thread. There is no evidence that that
> is any more valid an assumption than that it should be thread safe. But I can
> do it ... only it won't tell us if multiple threads try to access the counter
> at the same time.
>
> NB: my initial proposition used about the cheapest way to get interlocked
> counting I could think of, to introduce as little side-effects as possible
> for applications that are not concerned by possible race conditions. I don't
> believe there can be any other side-effect than additional cycles (though by
> using QAtomicInt we have to assume that it is implemented correctly).
>
> René J.V. Bertin wrote:
> A little extract, seconds after relaunching KDevelop with
> ```C++
> void KGlobal::ref()
> {
> s_refCount.fetchAndAddOrdered(1);
> if ( QThread::currentThread() != qApp->thread() ) {
> qWarning() << "KGlobal::ref() called off the main thread" <<
> kBacktrace();
> }
> //kDebug() << "KGlobal::ref() : refCount = " << int(s_refCount);
> }
>
> void KGlobal::deref()
> {
> int prevRefCount = s_refCount.fetchAndAddOrdered(-1);
> if ( QThread::currentThread() != qApp->thread() ) {
> qWarning() << "KGlobal::ref() called off the main thread" <<
> kBacktrace();
> }
> //kDebug() << "KGlobal::deref() : refCount = " << int(s_refCount);
> if (prevRefCount <= 1 && int(s_allowQuit)) {
> QCoreApplication::instance()->quit();
> }
> }
> ```
>
> ```
> kdevelop(68673)/kdevelop (cpp support)
> Cpp::instantiateDeclarationAndContext: Resolved bad base-class
> "QtSharedPointer::ExternalRefCount< T >" "QtSharedPointer::ExternalRefCount<
> T >"
> kdevelop(68673)/kdevelop (cpp support)
> Cpp::instantiateDeclarationAndContext: Resolved bad base-class
> "QtSharedPointer::ExternalRefCount< T >" "QtSharedPointer::ExternalRefCount<
> T >"
> kdevelop(68673)/kdevelop (cpp support)
> Cpp::instantiateDeclarationAndContext: Resolved bad base-class
> "QtSharedPointer::ExternalRefCount< T >" "QtSharedPointer::ExternalRefCount<
> T >"
> KGlobal::ref() called off the main thread "[
> 0: 0 libkdecore.5.dylib 0x00000001000ba123
> _Z14kRealBacktracei + 67
> 1: 1 libkdecore.5.dylib 0x0000000100100d89
> _ZN7KGlobal3refEv + 169
> 2: 2 libkdecore.5.dylib 0x00000001000e49f1
> _ZN4KJobC2EP7QObject + 65
> 3: 3 kdevcmakemanager.so 0x000000012f85f3af
> _ZN21CMakeCommitChangesJobC1ERKN8KDevelop4PathEP12CMakeManagerPNS0_8IProjectE
> + 31
> ]
> "
> KGlobal::ref() called off the main thread "[
> 0: 0 libkdecore.5.dylib 0x00000001000ba123
> _Z14kRealBacktracei + 67
> 1: 1 libkdecore.5.dylib 0x0000000100100d89
> _ZN7KGlobal3refEv + 169
> 2: 2 libkdecore.5.dylib 0x00000001000e49f1
> _ZN4KJobC2EP7QObject + 65
> 3: 3 kdevcmakemanager.so 0x000000012f85f3af
> _ZN21CMakeCommitChangesJobC1ERKN8KDevelop4PathEP12CMakeManagerPNS0_8IProjectE
> + 31
> ]
> "
> KGlobal::ref() called off the main thread "[
> 0: 0 libkdecore.5.dylib 0x00000001000ba123
> _Z14kRealBacktracei + 67
> 1: 1 libkdecore.5.dylib 0x0000000100100d89
> _ZN7KGlobal3refEv + 169
> 2: 2 libkdecore.5.dylib 0x00000001000e49f1
> _ZN4KJobC2EP7QObject + 65
> 3: 3 kdevcmakemanager.so 0x000000012f85f3af
> _ZN21CMakeCommitChangesJobC1ERKN8KDevelop4PathEP12CMakeManagerPNS0_8IProjectE
> + 31
> ]
> "
> KGlobal::ref() called off the main thread "[
> 0: 0 libkdecore.5.dylib 0x00000001000ba123
> _Z14kRealBacktracei + 67
> 1: 1 libkdecore.5.dylib 0x0000000100100d89
> _ZN7KGlobal3refEv + 169
> 2: 2 libkdecore.5.dylib 0x00000001000e49f1
> _ZN4KJobC2EP7QObject + 65
> 3: 3 kdevcmakemanager.so 0x000000012f85f3af
> _ZN21CMakeCommitChangesJobC1ERKN8KDevelop4PathEP12CMakeManagerPNS0_8IProjectE
> + 31
> ]
> "
> KGlobal::ref() called off the main thread "[
> 0: 0 libkdecore.5.dylib 0x00000001000ba123
> _Z14kRealBacktracei + 67
> 1: 1 libkdecore.5.dylib 0x0000000100100d89
> _ZN7KGlobal3refEv + 169
> 2: 2 libkdecore.5.dylib 0x00000001000e49f1
> _ZN4KJobC2EP7QObject + 65
> 3: 3 kdevcmakemanager.so 0x000000012f85f3af
> _ZN21CMakeCommitChangesJobC1ERKN8KDevelop4PathEP12CMakeManagerPNS0_8IProjectE
> + 31
> ]
> "
> KGlobal::ref() called off the main thread "[
> 0: 0 libkdecore.5.dylib 0x00000001000ba123
> _Z14kRealBacktracei + 67
> 1: 1 libkdecore.5.dylib 0x0000000100100d89
> _ZN7KGlobal3refEv + 169
> 2: 2 libkdecore.5.dylib 0x00000001000e49f1
> _ZN4KJobC2EP7QObject + 65
> 3: 3 kdevcmakemanager.so 0x000000012f85f3af
> _ZN21CMakeCommitChangesJobC1ERKN8KDevelop4PathEP12CMakeManagerPNS0_8IProjectE
> + 31
> ]
> "
> KGlobal::ref() called off the main thread "[
> 0: 0 libkdecore.5.dylib 0x00000001000ba123
> _Z14kRealBacktracei + 67
> 1: 1 libkdecore.5.dylib 0x0000000100100d89
> _ZN7KGlobal3refEv + 169
> 2: 2 libkdecore.5.dylib 0x00000001000e49f1
> _ZN4KJobC2EP7QObject + 65
> 3: 3 kdevcmakemanager.so 0x000000012f85f3af
> _ZN21CMakeCommitChangesJobC1ERKN8KDevelop4PathEP12CMakeManagerPNS0_8IProjectE
> + 31
> ]
> "
> KGlobal::ref() called off the main thread "[
> 0: 0 libkdecore.5.dylib 0x00000001000ba123
> _Z14kRealBacktracei + 67
> 1: 1 libkdecore.5.dylib 0x0000000100100d89
> _ZN7KGlobal3refEv + 169
> 2: 2 libkdecore.5.dylib 0x00000001000e49f1
> _ZN4KJobC2EP7QObject + 65
> 3: 3 kdevcmakemanager.so 0x000000012f85f3af
> _ZN21CMakeCommitChangesJobC1ERKN8KDevelop4PathEP12CMakeManagerPNS0_8IProjectE
> + 31
> ]
> "
> KGlobal::ref() called off the main thread "[
> 0: 0 libkdecore.5.dylib 0x00000001000ba123
> _Z14kRealBacktracei + 67
> 1: 1 libkdecore.5.dylib 0x0000000100100d89
> _ZN7KGlobal3refEv + 169
> 2: 2 libkdecore.5.dylib 0x00000001000e49f1
> _ZN4KJobC2EP7QObject + 65
> 3: 3 kdevcmakemanager.so 0x000000012f85f3af
> _ZN21CMakeCommitChangesJobC1ERKN8KDevelop4PathEP12CMakeManagerPNS0_8IProjectE
> + 31
> ]
> "
> KGlobal::ref() called off the main thread "[
> 0: 0 libkdecore.5.dylib 0x00000001000ba123
> _Z14kRealBacktracei + 67
> 1: 1 libkdecore.5.dylib 0x0000000100100d89
> _ZN7KGlobal3refEv + 169
> 2: 2 libkdecore.5.dylib 0x00000001000e49f1
> _ZN4KJobC2EP7QObject + 65
> 3: 3 kdevcmakemanager.so 0x000000012f85f3af
> _ZN21CMakeCommitChangesJobC1ERKN8KDevelop4PathEP12CMakeManagerPNS0_8IProjectE
> + 31
> ]
> "
> KGlobal::ref() called off the main thread "[
> 0: 0 libkdecore.5.dylib 0x00000001000ba123
> _Z14kRealBacktracei + 67
> 1: 1 libkdecore.5.dylib 0x0000000100100d89
> _ZN7KGlobal3refEv + 169
> 2: 2 libkdecore.5.dylib 0x00000001000e49f1
> _ZN4KJobC2EP7QObject + 65
> 3: 3 kdevcmakemanager.so 0x000000012f85f3af
> _ZN21CMakeCommitChangesJobC1ERKN8KDevelop4PathEP12CMakeManagerPNS0_8IProjectE
> + 31
> ]
> "
> KGlobal::ref() called off the main thread "[
> 0: 0 libkdecore.5.dylib 0x00000001000ba123
> _Z14kRealBacktracei + 67
> 1: 1 libkdecore.5.dylib 0x0000000100100d89
> _ZN7KGlobal3refEv + 169
> 2: 2 libkdecore.5.dylib 0x00000001000e49f1
> _ZN4KJobC2EP7QObject + 65
> 3: 3 kdevcmakemanager.so 0x000000012f85f3af
> _ZN21CMakeCommitChangesJobC1ERKN8KDevelop4PathEP12CMakeManagerPNS0_8IProjectE
> + 31
> ]
> "
> KGlobal::ref() called off the main thread "[
> 0: 0 libkdecore.5.dylib 0x00000001000ba123
> _Z14kRealBacktracei + 67
> 1: 1 libkdecore.5.dylib 0x0000000100100d89
> _ZN7KGlobal3refEv + 169
> 2: 2 libkdecore.5.dylib 0x00000001000e49f1
> _ZN4KJobC2EP7QObject + 65
> 3: 3 kdevcmakemanager.so 0x000000012f85f3af
> _ZN21CMakeCommitChangesJobC1ERKN8KDevelop4PathEP12CMakeManagerPNS0_8IProjectE
> + 31
> ]
> "
> kdevelop(68673)/kdevelop (cpp support)
> Cpp::instantiateDeclarationAndContext: Resolved bad base-class "_List_base<
> _Tp, _Alloc >" "_List_base< _Tp, _Alloc >"
> kdevelop(68673)/kdevelop (cpp support)
> Cpp::instantiateDeclarationAndContext: Resolved bad base-class "QVector< T >"
> "QVector< T >"
> KGlobal::ref() called off the main thread "[
> 0: 0 libkdecore.5.dylib 0x00000001000ba123
> _Z14kRealBacktracei + 67
> 1: 1 libkdecore.5.dylib 0x0000000100100d89
> _ZN7KGlobal3refEv + 169
> 2: 2 libkdecore.5.dylib 0x00000001000e49f1
> _ZN4KJobC2EP7QObject + 65
> 3: 3 kdevcmakemanager.so 0x000000012f85f3af
> _ZN21CMakeCommitChangesJobC1ERKN8KDevelop4PathEP12CMakeManagerPNS0_8IProjectE
> + 31
> ]
> "
> KGlobal::ref() called off the main thread "[
> 0: 0 libkdecore.5.dylib 0x00000001000ba123
> _Z14kRealBacktracei + 67
> 1: 1 libkdecore.5.dylib 0x0000000100100d89
> _ZN7KGlobal3refEv + 169
> 2: 2 libkdecore.5.dylib 0x00000001000e49f1
> _ZN4KJobC2EP7QObject + 65
> 3: 3 kdevcmakemanager.so 0x000000012f85f3af
> _ZN21CMakeCommitChangesJobC1ERKN8KDevelop4PathEP12CMakeManagerPNS0_8IProjectE
> + 31
> ]
> "
> KGlobal::ref() called off the main thread "[
> 0: 0 libkdecore.5.dylib 0x00000001000ba123
> _Z14kRealBacktracei + 67
> 1: 1 libkdecore.5.dylib 0x0000000100100d89
> _ZN7KGlobal3refEv + 169
> 2: 2 libkdecore.5.dylib 0x00000001000e49f1
> _ZN4KJobC2EP7QObject + 65
> 3: 3 kdevcmakemanager.so 0x000000012f85f3af
> _ZN21CMakeCommitChangesJobC1ERKN8KDevelop4PathEP12CMakeManagerPNS0_8IProjectE
> + 31
> ]
> "
> KGlobal::ref() called off the main thread "[
> 0: 0 libkdecore.5.dylib 0x00000001000ba123
> _Z14kRealBacktracei + 67
> 1: 1 libkdecore.5.dylib 0x0000000100100d89
> _ZN7KGlobal3refEv + 169
> 2: 2 libkdecore.5.dylib 0x00000001000e49f1
> _ZN4KJobC2EP7QObject + 65
> 3: 3 kdevcmakemanager.so 0x000000012f85f3af
> _ZN21CMakeCommitChangesJobC1ERKN8KDevelop4PathEP12CMakeManagerPNS0_8IProjectE
> + 31
> ]
> "
> KGlobal::ref() called off the main thread "[
> 0: 0 libkdecore.5.dylib 0x00000001000ba123
> _Z14kRealBacktracei + 67
> 1: 1 libkdecore.5.dylib 0x0000000100100d89
> _ZN7KGlobal3refEv + 169
> 2: 2 libkdecore.5.dylib 0x00000001000e49f1
> _ZN4KJobC2EP7QObject + 65
> 3: 3 kdevcmakemanager.so 0x000000012f85f3af
> _ZN21CMakeCommitChangesJobC1ERKN8KDevelop4PathEP12CMakeManagerPNS0_8IProjectE
> + 31
> ]
> "
> KGlobal::ref() called off the main thread "[
> 0: 0 libkdecore.5.dylib 0x00000001000ba123
> _Z14kRealBacktracei + 67
> 1: 1 libkdecore.5.dylib 0x0000000100100d89
> _ZN7KGlobal3refEv + 169
> 2: 2 libkdecore.5.dylib 0x00000001000e49f1
> _ZN4KJobC2EP7QObject + 65
> 3: 3 kdevcmakemanager.so 0x000000012f85f3af
> _ZN21CMakeCommitChangesJobC1ERKN8KDevelop4PathEP12CMakeManagerPNS0_8IProjectE
> + 31
> ]
> "
> KGlobal::ref() called off the main thread "[
> 0: 0 libkdecore.5.dylib 0x00000001000ba123
> _Z14kRealBacktracei + 67
> 1: 1 libkdecore.5.dylib 0x0000000100100d89
> _ZN7KGlobal3refEv + 169
> 2: 2 libkdecore.5.dylib 0x00000001000e49f1
> _ZN4KJobC2EP7QObject + 65
> 3: 3 kdevcmakemanager.so 0x000000012f85f3af
> _ZN21CMakeCommitChangesJobC1ERKN8KDevelop4PathEP12CMakeManagerPNS0_8IProjectE
> + 31
> ]
> "
> ```
>
> René J.V. Bertin wrote:
> A bit more detail:
>
> ```C++
> void KGlobal::ref()
> {
> s_refCount.fetchAndAddOrdered(1);
> if ( QThread::currentThread() != qApp->thread() ) {
> qWarning() << "KGlobal::ref()=" << int(s_refCount) << "called
> from background thread" << QThread::currentThread() << "\n" << kBacktrace();
> }
> //kDebug() << "KGlobal::ref() : refCount = " << int(s_refCount);
> }
>
> void KGlobal::deref()
> {
> int prevRefCount = s_refCount.fetchAndAddOrdered(-1);
> if ( QThread::currentThread() != qApp->thread() ) {
> qWarning() << "KGlobal::deref()=" << int(s_refCount) << "called
> from background thread" << QThread::currentThread() << "\n" << kBacktrace();
> }
> //kDebug() << "KGlobal::deref() : refCount = " << int(s_refCount);
> if (prevRefCount <= 1 && int(s_allowQuit)) {
> QCoreApplication::instance()->quit();
> }
> }
> ```
>
> ```
> "
> KGlobal::ref()= 20 called from background thread QThread(0x121a40e30,
> name = "Thread (pooled)")
> "[
> 0: 0 libkdecore.5.dylib 0x00000001000b9f83
> _Z14kRealBacktracei + 67
> 1: 1 libkdecore.5.dylib 0x0000000100100c4b
> _ZN7KGlobal3refEv + 267
> 2: 2 libkdecore.5.dylib 0x00000001000e4851
> _ZN4KJobC2EP7QObject + 65
> 3: 3 kdevcmakemanager.so 0x000000013015f3af
> _ZN21CMakeCommitChangesJobC1ERKN8KDevelop4PathEP12CMakeManagerPNS0_8IProjectE
> + 31
> ]
> "
> KGlobal::ref()= 21 called from background thread QThread(0x121a40e30,
> name = "Thread (pooled)")
> "[
> 0: 0 libkdecore.5.dylib 0x00000001000b9f83
> _Z14kRealBacktracei + 67
> 1: 1 libkdecore.5.dylib 0x0000000100100c4b
> _ZN7KGlobal3refEv + 267
> 2: 2 libkdecore.5.dylib 0x00000001000e4851
> _ZN4KJobC2EP7QObject + 65
> 3: 3 kdevcmakemanager.so 0x000000013015f3af
> _ZN21CMakeCommitChangesJobC1ERKN8KDevelop4PathEP12CMakeManagerPNS0_8IProjectE
> + 31
> ]
> "
> KGlobal::ref()= 9 called from background thread QThread(0x13ebfb7b0, name
> = "Thread (pooled)")
> "[
> 0: 0 libkdecore.5.dylib 0x00000001000b9f83
> _Z14kRealBacktracei + 67
> 1: 1 libkdecore.5.dylib 0x0000000100100c4b
> _ZN7KGlobal3refEv + 267
> 2: 2 libkdecore.5.dylib 0x00000001000e4851
> _ZN4KJobC2EP7QObject + 65
> 3: 3 kdevcmakemanager.so 0x000000013015f3af
> _ZN21CMakeCommitChangesJobC1ERKN8KDevelop4PathEP12CMakeManagerPNS0_8IProjectE
> + 31
> ]
> "
> KGlobal::ref()= 10 called from background thread QThread(0x11c1b3660,
> name = "Thread (pooled)")
> "[
> 0: 0 libkdecore.5.dylib 0x00000001000b9f83
> _Z14kRealBacktracei + 67
> 1: 1 libkdecore.5.dylib 0x0000000100100c4b
> _ZN7KGlobal3refEv + 267
> 2: 2 libkdecore.5.dylib 0x00000001000e4851
> _ZN4KJobC2EP7QObject + 65
> 3: 3 kdevcmakemanager.so 0x000000013015f3af
> _ZN21CMakeCommitChangesJobC1ERKN8KDevelop4PathEP12CMakeManagerPNS0_8IProjectE
> + 31
> ]
> "
> ```
> ...
> ```
> "
> KGlobal::ref()= 269 called from background thread QThread(0x11c1b3660,
> name = "Thread (pooled)")
> "[
> 0: 0 libkdecore.5.dylib 0x00000001000b9f83
> _Z14kRealBacktracei + 67
> 1: 1 libkdecore.5.dylib 0x0000000100100c4b
> _ZN7KGlobal3refEv + 267
> 2: 2 libkdecore.5.dylib 0x00000001000e4851
> _ZN4KJobC2EP7QObject + 65
> 3: 3 kdevcmakemanager.so 0x000000013015f3af
> _ZN21CMakeCommitChangesJobC1ERKN8KDevelop4PathEP12CMakeManagerPNS0_8IProjectE
> + 31
> ]
> "
> KGlobal::ref()= 270 called from background thread QThread(0x11c1b3660,
> name = "Thread (pooled)")
> "[
> 0: 0 libkdecore.5.dylib 0x00000001000b9f83
> _Z14kRealBacktracei + 67
> 1: 1 libkdecore.5.dylib 0x0000000100100c4b
> _ZN7KGlobal3refEv + 267
> 2: 2 libkdecore.5.dylib 0x00000001000e4851
> _ZN4KJobC2EP7QObject + 65
> 3: 3 kdevcmakemanager.so 0x000000013015f3af
> _ZN21CMakeCommitChangesJobC1ERKN8KDevelop4PathEP12CMakeManagerPNS0_8IProjectE
> + 31
> ]
> "
> KGlobal::ref()= 271 called from background thread QThread(0x13ebfb7b0,
> name = "Thread (pooled)")
> "[
> 0: 0 libkdecore.5.dylib 0x00000001000b9f83
> _Z14kRealBacktracei + 67
> 1: 1 libkdecore.5.dylib 0x0000000100100c4b
> _ZN7KGlobal3refEv + 267
> 2: 2 libkdecore.5.dylib 0x00000001000e4851
> _ZN4KJobC2EP7QObject + 65
> 3: 3 kdevcmakemanager.so 0x000000013015f3af
> _ZN21CMakeCommitChangesJobC1ERKN8KDevelop4PathEP12CMakeManagerPNS0_8IProjectE
> + 31
> ]
> "
> KGlobal::ref()= 273 called from background thread QThread(0x13ebfb7b0,
> name = "Thread (pooled)")
> "[
> 0: 0 libkdecore.5.dylib 0x00000001000b9f83
> _Z14kRealBacktracei + 67
> 1: 1 libkdecore.5.dylib 0x0000000100100c4b
> _ZN7KGlobal3refEv + 267
> 2: 2 libkdecore.5.dylib 0x00000001000e4851
> _ZN4KJobC2EP7QObject + 65
> 3: 3 kdevcmakemanager.so 0x000000013015f3af
> _ZN21CMakeCommitChangesJobC1ERKN8KDevelop4PathEP12CMakeManagerPNS0_8IProjectE
> + 31
> ]
> "
> ```
> (pity so few people set thread names, or use setObjectName...)
>
> I'd say we have at least 2 threads running a cmake manager, and each
> thread *increase* the refcounter repeatedly (and in rather fast succession),
> but for now I haven't found a single `deref` that's not done off the main
> thread. Yet it has been called as can be seen above.
>
> Seems there's nothing in this scenario preventing a background thread to
> `ref()` while the main thread tries to `deref`.
>
> I'll leave it to one of you guys to try this under Linux where the
> backtrace will maybe contain a bit more useful information.
>
> Milian Wolff wrote:
> Great! That information alone is good evidence that the status quo is
> broken. A ref might clash with a deref in another thread and lead to
> undefined behavior, such as the condition reading <= 1 becoming true leading
> to the shutdown.
>
> Looking more at the code, I agree that KGlobal::ref/deref should become
> threadsafe. So if you cleanup your patch (i.e. remove the qWarning), I'm all
> for pushing this in.
>
> René J.V. Bertin wrote:
> Whew ... good thing I'm a stubborn bastard :)
>
> Just to complete the report: my test session has now finished importing
> and parsing a total of 5 projects, and to be exhaustive I requested a reload
> of one of them. Not a single deref was done off the main thread.
>
> Albert just emailed me that this ref counting mechanism is in place since
> 2006 . Surprises me a bit. True, multi-processor machines weren't that common
> by then (though I got my first in 2005, a G5 Powermac) and I guess that the
> race condition is unlikely to occur when you do multithreading on a single
> CPU core. But that situation has changed (and I've seen "my issue" occur even
> on a dual-core netbook :)).
>
> I'll clean out the code (again :)), update the diff, and then wait a bit
> for others to react before pushing it in.
>
> Milian Wolff wrote:
> You keep forgetting/confusing different things. You can use multiple
> cores / threads in an application just fine without ever noticing the issue
> at hand. It only manifests when you actually use KGlobal::(de)ref from
> different threads. Something that apparently noone ever did - besides
> KDevelop which constructs KJob's in a background thread.
>
> Albert Astals Cid wrote:
> Thus maybe what needs to happen is fix KDevelop?
Of course it only manifests itself when using KGlobal::(de)ref from different
threads, but even then there should be no problem when the actual access to the
same memory location is really concurrent, which cannot AFAIK be the case if
you the threads run on a single core.
Apparently indeed no one ever ran into this issue. It's possible that KDevelop
only does because it stresses the refcounting mechanism, running multiple
short-lived jobs concurrently.
As to "fixing KDevelop": I'll let Milian reply to that.
- René J.V.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/121134/#review70441
-----------------------------------------------------------
On Nov. 16, 2014, 8:19 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, 8:19 p.m.)
>
>
> Review request for KDE Software on Mac OS X and kdelibs.
>
>
> 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
>
>
