Review Request 121134: make KGlobal reference counting threadsafe
Milian Wolff
mail at milianw.de
Sun Nov 16 19:18:17 GMT 2014
> 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.
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.
- Milian
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/121134/#review70441
-----------------------------------------------------------
On Nov. 16, 2014, 7:13 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:13 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
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20141116/658059c2/attachment.htm>
More information about the kde-core-devel
mailing list