> On nov. 16, 2014, 2: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.
Thus maybe what needs to happen is fix KDevelop?
- Albert
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/121134/#review70441
-----------------------------------------------------------
On nov. 16, 2014, 7: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, 7: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
>
>
