<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, 8:22 p.m. CET, <b>Thomas Lübking</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="https://git.reviewboard.kde.org/r/121134/diff/5/?file=328836#file328836line343" style="color: black; font-weight: bold; text-decoration: underline;">kdecore/kernel/kglobal.cpp</a>
<span style="font-weight: normal;">
(Diff revision 5)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">QString KGlobal::caption()</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">337</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">s_allowQuit</span><span class="hl"> </span><span class="o"><span class="hl">=</span></span><span class="hl"> </span><span class="n">allowQuit</span><span class="p">;</span></pre></td>
<th bgcolor="#e9eaa8" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">341</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">s_allowQuit</span><span class="p"><span class="hl">.</span></span><span class="n"><span class="hl">fetchAndStoreOrdered</span></span><span class="p"><span class="hl">(</span></span><span class="kt"><span class="hl">int</span></span><span class="p"><span class="hl">(</span></span><span class="n">allowQuit</span><span class="p"><span class="hl">))</span>;</span></pre></td>
</tr>
</tbody>
</table>
<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;">Does this actually make sense?
The result is "undefined" when used from multiple threads anyway.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Should one enforce this to be called from the main thread only instead?</p></pre>
</blockquote>
<p>On November 16th, 2014, 9:08 p.m. CET, <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;">How do you mean, undefined? The result will be the value set by whoever called the function last.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">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.</p></pre>
</blockquote>
<p>On November 16th, 2014, 9:38 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;"><blockquote style="text-rendering: inherit;padding: 0 0 0 1em;border-left: 1px solid #bbb;white-space: normal;margin: 0 0 0 0.5em;line-height: inherit;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">How do you mean, undefined?</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">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.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">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")</p></pre>
</blockquote>
<p>On November 16th, 2014, 9:52 p.m. CET, <b>David Faure</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;">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.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">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).</p></pre>
</blockquote>
<p>On November 16th, 2014, 10:03 p.m. CET, <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;">Indeed.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">So how about</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;"><div class="codehilite" style="background: #f8f8f8"><pre style="line-height: 125%"><span style="color: #B00040">void</span> KGlobal<span style="color: #666666">::</span>setAllowQuit(<span style="color: #B00040">bool</span> allowQuit)
{
<span style="color: #008000; font-weight: bold">if</span> (QThread<span style="color: #666666">::</span>currentThread() <span style="color: #666666">==</span> qApp<span style="color: #666666">-></span><span style="color: #008000; font-weight: bold">thread</span>()) {
s_allowQuit.fetchAndStoreOrdered(<span style="color: #B00040">int</span>(allowQuit));
}
<span style="color: #008000; font-weight: bold">else</span> {
qWarning() <span style="color: #666666"><<</span> <span style="color: #BA2121">"KGlobal::setAllowQuit may only be called from the main thread"</span>;
}
}
</pre></div>
</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">but then if the action can be refused without feedback to the caller we would maybe need a getter?</p></pre>
</blockquote>
<p>On November 16th, 2014, 10:09 p.m. CET, <b>David Faure</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 don't see the need for a getter (what would the app do with the value...).</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">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 ;)</p></pre>
</blockquote>
<p>On November 16th, 2014, 10:15 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;">I meant "local, in the client code" - so the client would still have a deterministic idea on the value.</p>
<blockquote style="text-rendering: inherit;padding: 0 0 0 1em;border-left: 1px solid #bbb;white-space: normal;margin: 0 0 0 0.5em;line-height: inherit;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">The only reason for an atomic int here</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Ah, the ::deref() check (against late -and idempotent- writes). Makes sense, yes.</p>
<blockquote style="text-rendering: inherit;padding: 0 0 0 1em;border-left: 1px solid #bbb;white-space: normal;margin: 0 0 0 0.5em;line-height: inherit;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">we would maybe need a getter?</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">one could probably also just abort. The function <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">should</em> apparently not be even called before any off-thread or nested ref/deref starts</p></pre>
</blockquote>
<p>On November 16th, 2014, 10:21 p.m. CET, <b>David Faure</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;">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 ;)</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I think the warning is good enough, the app can proceed anyhow, making users happier.</p></pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; 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 couldn't agree more.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">After all, this whole exercise started for me because of an application quitting for no (user-)apparent reason. ;)</p></pre>
<br />
<p>- René J.V.</p>
<br />
<p>On November 16th, 2014, 10:14 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, kdelibs and David Faure.</div>
<div>By René J.V. Bertin.</div>
<p style="color: grey;"><i>Updated Nov. 16, 2014, 10:14 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>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">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.</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>