[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: "Milian Wolff" <mail () milianw ! de>
Date: 2014-11-16 16:36:49
Message-ID: 20141116163649.22979.39362 () probe ! kde ! org
[Download RAW message or body]
--===============5460689541783123098==
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 8bit
> 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
>
>
--===============5460689541783123098==
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, 2:58 p.m. UTC, <b>Milian \
Wolff</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;">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?</p> <p style="padding: 0;text-rendering: inherit;margin: 0;line-height: \
inherit;white-space: inherit;">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).</p></pre> </blockquote>
<p>On November 16th, 2014, 3:53 p.m. UTC, <b>René J.V. Bertin</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;">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.</p> <p style="padding: 0;text-rendering: inherit;margin: 0;line-height: \
inherit;white-space: inherit;">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 <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: \
inherit;white-space: normal;">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.</em> . 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 <em style="padding: 0;text-rendering: \
inherit;margin: 0;line-height: inherit;white-space: normal;">concurrent \
processing</em> class that does not explicitly mention it does thread unsafe things \
can be assumed to be thread safe.</p> <p style="padding: 0;text-rendering: \
inherit;margin: 0;line-height: inherit;white-space: inherit;">There's no QAtomicBool, \
btw.</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;">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).</p> <p style="padding: 0;text-rendering: \
inherit;margin: 0;line-height: inherit;white-space: inherit;">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.</p></pre> <br />
<p>- Milian</p>
<br />
<p>On November 16th, 2014, 3:53 p.m. UTC, 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, 3: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>
--===============5460689541783123098==--
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic