[prev in list] [next in list] [prev in thread] [next in thread]
List: kde-core-devel
Subject: Re: Review Request 121134: make KGlobal reference counting threadsafe
From: René J.V. Bertin <rjvbertin () gmail ! com>
Date: 2014-11-16 16:56:37
Message-ID: 20141116165637.22979.22129 () probe ! kde ! org
[Download RAW message or body]
--===============8326069729637380502==
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 8bit
> 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
>
>
--===============8326069729637380502==
MIME-Version: 1.0
Content-Type: text/html; charset="utf-8"
Content-Transfer-Encoding: 8bit
<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="12" style="border: 1px #c9c399 \
solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;"> \
<tr> <td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://git.reviewboard.kde.org/r/121134/">https://git.reviewboard.kde.org/r/121134/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <p style="margin-top: 0;">On November 16th, 2014, 5:28 p.m. CET, <b>Thomas \
Lübking</b> wrote:</p> <blockquote style="margin-left: 1em; border-left: 2px solid \
#d0d0d0; padding-left: 10px;"> <pre style="white-space: pre-wrap; white-space: \
-moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: \
break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: \
inherit;white-space: inherit;">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)</p> <p style="padding: 0;text-rendering: inherit;margin: \
0;line-height: inherit;white-space: inherit;">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</p></pre> </blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: \
0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">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).</p> <p style="padding: \
0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I \
have indeed been amazed that this happens only with KDevelop, but there is the <code \
style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: \
0;line-height: inherit;">allowQuit</code> function that could be used by other \
applications. I see that KDE PIM sets it to true (<code style="text-rendering: \
inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: \
inherit;">KGlobal::setAllowQuit(true)</code>; 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.</p> <p style="padding: 0;text-rendering: inherit;margin: \
0;line-height: inherit;white-space: inherit;">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 ...</p> <p style="padding: 0;text-rendering: inherit;margin: 0;line-height: \
inherit;white-space: inherit;">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 ...</p></pre> <br />
<p>- René J.V.</p>
<br />
<p>On November 16th, 2014, 4:53 p.m. CET, René J.V. Bertin wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="12" style="border: \
1px #888a85 solid; border-radius: 6px; -moz-border-radius: 6px; \
-webkit-border-radius: 6px;"> <tr>
<td>
<div>Review request for KDE Software on Mac OS X and kdelibs.</div>
<div>By René J.V. Bertin.</div>
<p style="color: grey;"><i>Updated Nov. 16, 2014, 4:53 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
kdelibs
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" \
style="border: 1px solid #b8b5a0"> <tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: \
-moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: \
break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: \
inherit;white-space: inherit;">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 <code style="text-rendering: \
inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: \
inherit;">KGlobal::deref</code> reaching a zero reference count and invoking <code \
style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: \
0;line-height: inherit;">QCoreApplication::quit</code> 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.</p> <p \
style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: \
inherit;">This patch introduces thread-safety to KGlobal's reference counting by \
turning the simple global <code style="text-rendering: inherit;color: \
#4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">static int \
s_refCount</code> into a <code style="text-rendering: inherit;color: #4444cc;padding: \
0;white-space: normal;margin: 0;line-height: inherit;">static QAtomicInt \
s_refCount</code>. I consider this an important bug fix regardless of whether it \
corrects the issue I have with KDevelop.</p></pre> </td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: \
1px solid #b8b5a0"> <tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: \
-moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: \
break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: \
inherit;white-space: inherit;">On OS X 10.6.8 only for now.</p></pre> </td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>kdecore/kernel/kglobal.cpp <span style="color: grey">(cf003a4)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/121134/diff/" style="margin-left: \
3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>
--===============8326069729637380502==--
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic