Review Request 121134: make KGlobal reference counting threadsafe
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121134/ --- 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. Thanks, René J.V. Bertin
Re: Review Request 121134: make KGlobal reference counting threadsafe
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121134/#review70441 --- 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). kdecore/kernel/kglobal.cpp https://git.reviewboard.kde.org/r/121134/#comment49264 why did you change the conditional? use int oldRefCount = s_refCount.fetchAndAddOrdered(-1) if (oldRefCount = 1 s_allowQuit) - Milian Wolff On Nov. 16, 2014, 2:12 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, 2:12 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. Thanks, René J.V. Bertin
Re: Review Request 121134: make KGlobal reference counting threadsafe
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). 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. - 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, 3:12 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, 3:12 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. Thanks, René J.V. Bertin
Re: Review Request 121134: make KGlobal reference counting threadsafe
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121134/ --- (Updated Nov. 16, 2014, 4:53 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 (updated) - kdecore/kernel/kglobal.cpp cf003a4 Diff: https://git.reviewboard.kde.org/r/121134/diff/ Testing --- On OS X 10.6.8 only for now. Thanks, René J.V. Bertin
Re: Review Request 121134: make KGlobal reference counting threadsafe
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121134/#review70444 --- Do you have a backtrace of the condition? If there's trouble w/ ONE client onnly, i'd rather bet on a client bug (and KGlobal doesn't mention thread-safetyness) What you probably can do (as it's reproducible somehow) is to add a QMutex. ::tryLock() before and ::unlock() after altering the value. If ::tryLock() ever returns false you can abort(); (or just yell a warning ;-) and got your confirmation - Thomas Lübking On Nov. 16, 2014, 3:53 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, 3:53 nachm.) 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. Thanks, René J.V. Bertin
Re: Review Request 121134: make KGlobal reference counting threadsafe
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121134/#review70448 --- kdecore/kernel/kglobal.cpp https://git.reviewboard.kde.org/r/121134/#comment49267 remove this. - Milian Wolff On Nov. 16, 2014, 3:53 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, 3:53 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. Thanks, René J.V. Bertin
Re: Review Request 121134: make KGlobal reference counting threadsafe
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. 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. - Milian --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121134/#review70441 --- On Nov. 16, 2014, 3:53 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, 3:53 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. Thanks, René J.V. Bertin
Re: Review Request 121134: make KGlobal reference counting threadsafe
On Nov. 16, 2014, 5:28 p.m., Thomas Lübking wrote: Do you have a backtrace of the condition? If there's trouble w/ ONE client onnly, i'd rather bet on a client bug (and KGlobal doesn't mention thread-safetyness) What you probably can do (as it's reproducible somehow) is to add a QMutex. ::tryLock() before and ::unlock() after altering the value. If ::tryLock() ever returns false you can abort(); (or just yell a warning ;-) and got your confirmation I've been trying to get backtraces of the condition for a very long time. Eventually I had the idea to break in QCoreApplication::exit, and when that fired I saw I got there through the ~KJob of a job I had already been suspecting. Next step was breaking conditionally on KGlobal::deref when s_refCount = 1, and I had the luck of getting a hit almost immediately (on Linux, what's more). I have indeed been amazed that this happens only with KDevelop, but there is the `allowQuit` function that could be used by other applications. I see that KDE PIM sets it to true (`KGlobal::setAllowQuit(true)`; default is false, which prevents the whole issue) only in the various agents but not in the KMail, Kontact etc. I cannot find that call in KDevelop's code base so it's probably a library they use that sets toggles the setting. Setting a QMutex and checking if concurrent access ever occurs might work but still won't tell us if it's indeed the cause of the quitting issue. And it could be it never trips because the extra code could add enough overhead that I never stumble upon a locked mutex when calling ref or deref ... Again: a feature 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. I regret haven given the background information now ... - René J.V. --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121134/#review70444 --- On Nov. 16, 2014, 4:53 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, 4:53 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. Thanks, René J.V. Bertin
Re: Review Request 121134: make KGlobal reference counting threadsafe
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. 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. --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121134/#review70441 --- On Nov. 16, 2014, 4:53 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, 4:53 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
Re: Review Request 121134: make KGlobal reference counting threadsafe
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).
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)
Re: Review Request 121134: make KGlobal reference counting threadsafe
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
Re: Review Request 121134: make KGlobal reference counting threadsafe
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
Re: Review Request 121134: make KGlobal reference counting threadsafe
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121134/ --- (Updated Nov. 16, 2014, 7:55 p.m.) Review request for KDE Software on Mac OS X and kdelibs. Changes --- New version of the diff. I've left in the off-main-thread debug output for when compiling with `-DQT_DEBUG` though cross-thread usage ought no longer be an issue of course. 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 (updated) - kdecore/kernel/kglobal.cpp cf003a4 Diff: https://git.reviewboard.kde.org/r/121134/diff/ Testing --- On OS X 10.6.8 only for now. Thanks, René J.V. Bertin
Re: Review Request 121134: make KGlobal reference counting threadsafe
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121134/ --- (Updated Nov. 16, 2014, 8:01 p.m.) Review request for KDE Software on Mac OS X and kdelibs. Changes --- Updated the Testing Done text. 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 (updated) --- 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
Re: Review Request 121134: make KGlobal reference counting threadsafe
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121134/#review70463 --- kdecore/kernel/kglobal.cpp https://git.reviewboard.kde.org/r/121134/#comment49274 QAtomicInt is not a POD, so it is not safe to be used this way. You rather need to use the semi-private QBasicAtomicInt, which is not a POD, like: static QBasicAtomicInt s_allowQuit = Q_BASIC_ATOMIC_INITIALIZER(0); The API should be the same. - Pino Toscano On Nov. 16, 2014, 7:01 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:01 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
Re: Review Request 121134: make KGlobal reference counting threadsafe
On Nov. 16, 2014, 8:05 p.m., Pino Toscano wrote: kdecore/kernel/kglobal.cpp, lines 323-324 https://git.reviewboard.kde.org/r/121134/diff/3/?file=328834#file328834line323 QAtomicInt is not a POD, so it is not safe to be used this way. You rather need to use the semi-private QBasicAtomicInt, which is not a POD, like: static QBasicAtomicInt s_allowQuit = Q_BASIC_ATOMIC_INITIALIZER(0); The API should be the same. Personal education: what does POD stand for in this context? - René J.V. --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121134/#review70463 --- On Nov. 16, 2014, 8:01 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:01 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
Re: Review Request 121134: make KGlobal reference counting threadsafe
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121134/ --- (Updated Nov. 16, 2014, 8: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 (updated) - 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
Re: Review Request 121134: make KGlobal reference counting threadsafe
On Nov. 16, 2014, 7:05 p.m., Pino Toscano wrote: kdecore/kernel/kglobal.cpp, lines 323-324 https://git.reviewboard.kde.org/r/121134/diff/3/?file=328834#file328834line323 QAtomicInt is not a POD, so it is not safe to be used this way. You rather need to use the semi-private QBasicAtomicInt, which is not a POD, like: static QBasicAtomicInt s_allowQuit = Q_BASIC_ATOMIC_INITIALIZER(0); The API should be the same. René J.V. Bertin wrote: Personal education: what does POD stand for in this context? plain-old-data. - Milian --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121134/#review70463 --- 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
Re: Review Request 121134: make KGlobal reference counting threadsafe
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121134/#review70466 --- kdecore/kernel/kglobal.cpp https://git.reviewboard.kde.org/r/121134/#comment49277 please no, just remove it. Now with QAtomicInt it's just fine to use KJob from different threads. So no need to spam anything to the console, nor to complicate the code this way. remove this. kdecore/kernel/kglobal.cpp https://git.reviewboard.kde.org/r/121134/#comment49278 see above, this is just useless - remove this (also below). - Milian Wolff 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
Re: Review Request 121134: make KGlobal reference counting threadsafe
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
Re: Review Request 121134: make KGlobal reference counting threadsafe
--- 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 (updated) - 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
Re: Review Request 121134: make KGlobal reference counting threadsafe
On Nov. 16, 2014, 8:05 p.m., Pino Toscano wrote: kdecore/kernel/kglobal.cpp, lines 323-324 https://git.reviewboard.kde.org/r/121134/diff/3/?file=328834#file328834line323 QAtomicInt is not a POD, so it is not safe to be used this way. You rather need to use the semi-private QBasicAtomicInt, which is not a POD, like: static QBasicAtomicInt s_allowQuit = Q_BASIC_ATOMIC_INITIALIZER(0); The API should be the same. René J.V. Bertin wrote: Personal education: what does POD stand for in this context? Milian Wolff wrote: plain-old-data. Ah. When in doubt in this kind of situation I often rely (first) on the compiler to insult my ignorance ... which it didn't here :) - René J.V. --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121134/#review70463 --- 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
Re: Review Request 121134: make KGlobal reference counting threadsafe
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121134/#review70468 --- kdecore/kernel/kglobal.cpp https://git.reviewboard.kde.org/r/121134/#comment49279 *cough* kdecore/kernel/kglobal.cpp https://git.reviewboard.kde.org/r/121134/#comment49281 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? - Thomas Lübking On Nov. 16, 2014, 7:19 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, 7:19 nachm.) 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
Re: Review Request 121134: make KGlobal reference counting threadsafe
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
Re: Review Request 121134: make KGlobal reference counting threadsafe
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? 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. - 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, 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
Re: Review Request 121134: make KGlobal reference counting threadsafe
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
Re: Review Request 121134: make KGlobal reference counting threadsafe
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121134/ --- (Updated Nov. 16, 2014, 8: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 (updated) - 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
Re: Review Request 121134: make KGlobal reference counting threadsafe
On Nov. 16, 2014, 7: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) 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). - David --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121134/#review70468 --- On Nov. 16, 2014, 8: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, 8: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
Re: Review Request 121134: make KGlobal reference counting threadsafe
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
Re: Review Request 121134: make KGlobal reference counting threadsafe
On Nov. 16, 2014, 7: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).
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?
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 ;)
- David
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/121134/#review70468
---
On Nov. 16, 2014, 8: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, 8: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
Re: Review Request 121134: make KGlobal reference counting threadsafe
On Nov. 16, 2014, 9:29 p.m., David Faure wrote: kdecore/kernel/kglobal.cpp, line 332 https://git.reviewboard.kde.org/r/121134/diff/6/?file=328847#file328847line332 Would s_allowQuit.load() compile? I forgot the Qt4 API since it changed in Qt5. It would be slightly better imho, for readability. Nope, sorry. - René J.V. --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121134/#review70475 --- 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
Re: Review Request 121134: make KGlobal reference counting threadsafe
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121134/ --- (Updated Nov. 16, 2014, 10:14 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 (updated) - 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
Re: Review Request 121134: make KGlobal reference counting threadsafe
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
Re: Review Request 121134: make KGlobal reference counting threadsafe
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).
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 ;)
Thomas Lübking wrote:
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
David Faure wrote:
Qt typically warns rather than aborting when doing something like this
(e.g. sendPostedEvents on object living in another thread, stopping timers
from wrong thread, etc.). Otherwise akonadi and plasma wouldn't run very
long, ahem ;)
I think the warning is good enough, the app can proceed anyhow, making
users happier.
I couldn't agree more.
After all, this whole exercise started for me because of an application
quitting for no (user-)apparent reason. ;)
- 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, 10:14 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, 10:14 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
Re: Review Request 121134: make KGlobal reference counting threadsafe
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121134/ --- (Updated Nov. 16, 2014, 9:39 p.m.) Status -- This change has been marked as submitted. 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
