<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://git.reviewboard.kde.org/r/119110/">https://git.reviewboard.kde.org/r/119110/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On July 4th, 2014, 7:22 p.m. UTC, <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;">recursive mutexes are more expensive. Can you try this patch instead?
http://www.davidfaure.fr/2014/kprotocolmanager.cpp.diff</pre>
</blockquote>
<p>On July 4th, 2014, 7:27 p.m. UTC, <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;">Actually, nothing beats a unittest :) Added, fix pushed.
http://commits.kde.org/kio/842bc8008f5eed03698e8ec67351e791b65ad2b1
I'll repack kio after dinner.</pre>
</blockquote>
<p>On July 4th, 2014, 9:02 p.m. UTC, <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;">Done (v4.100.0-rc2). You can discard this RR.</pre>
</blockquote>
<p>On July 4th, 2014, 9:58 p.m. UTC, <b>Vishesh Handa</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;">Just out of curiosity, what's so expensive about Recursive mutexes?</pre>
</blockquote>
<p>On July 4th, 2014, 10:10 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;">http://stackoverflow.com/questions/187761/recursive-lock-mutex-vs-non-recursive-lock-mutex reads quite nicely. Generally, there is actually never a reason to use a recursive mutex, except for lazyness imo. And, usually, working with non-recursive mutices actually enforces a style of programming, which often performs much better, as you have to ensure to only hold the lock for the minimal amount of time required. With recursive mutices, you often hold the lock for a long time, potentially with long detours. A common example I saw: emitting a signal and then all directly connected slots get evoked, which in turn calls more code on your object, leading to a reentry and so forth. This should never happen, and thus recursive mutices can be regarded as code-smell, imo. </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;">A recursive QMutex needs a d pointer (and doesn't otherwise), to refcount the locking by the current thread. But that's not the main point.
To me it's the symptom of unclean code.
A proper mutex-protected class should lock the mutex in all public methods, and from there only call private methods (which assume the mutex to be locked).
This keeps things properly separated and clean.
Making the mutex recursive basically means "I don't know how this gets called, so in doubt, I'll just make it recursive". It's the easy way out, admitting that we don't understand the control flow in the code.</pre>
<br />
<p>- David</p>
<br />
<p>On July 4th, 2014, 9:58 p.m. UTC, Vishesh Handa wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('https://git.reviewboard.kde.org/static/rb/images/review_request_box_top_bg.ab6f3b1072c9.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for KDE Frameworks and David Faure.</div>
<div>By Vishesh Handa.</div>
<p style="color: grey;"><i>Updated July 4, 2014, 9:58 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
kio
</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;"> KProtocolManager: Fix double mutex locking on a non recursive mutex
Currently called reparseConfiguration results in an infinite block
because the code tries to lock the mutex twice even though it is not a
recursive mutex.
Stack trace -
KProtocolManager::entryMap -> Tries to lock it
KIO::SlaveConfigPrivate::readGlobalConfig
KIO::SlaveConfig::reset
KProtocolManager::reparseConfiguration -> Holds the lock
This can easily be solved by making the mutex recursive, but that breaks
the asserts as they are using QMutex::tryLock to check if the mutex is
locked. With the mutex being recursive tryLock just results in them
getting locked again.
I'm not sure if this is the correct way of fixing it
Here is the full backtrace if anyone is interested -
#0 0x00007ffff328a359 in syscall () from /usr/lib/libc.so.6
#1 0x00007ffff3e21830 in _q_futex (addr=0x7ffff31a4b00 <(anonymous namespace)::Q_QGS_kProtocolManagerPrivate::innerFunction()::holder>, op=0, val=3, timeout=0x0)
at /home/vishesh/kde5/qt5/qtbase/src/corelib/thread/qmutex_linux.cpp:154
#2 0x00007ffff3e21a04 in lockInternal_helper<false> (d_ptr=..., timeout=-1, elapsedTimer=0x0) at /home/vishesh/kde5/qt5/qtbase/src/corelib/thread/qmutex_linux.cpp:195
#3 0x00007ffff3e21891 in QBasicMutex::lockInternal (this=0x7ffff31a4b00 <(anonymous namespace)::Q_QGS_kProtocolManagerPrivate::innerFunction()::holder>)
at /home/vishesh/kde5/qt5/qtbase/src/corelib/thread/qmutex_linux.cpp:211
#4 0x00007ffff3e2164c in QMutex::lock (this=0x7ffff31a4b00 <(anonymous namespace)::Q_QGS_kProtocolManagerPrivate::innerFunction()::holder>)
at /home/vishesh/kde5/qt5/qtbase/src/corelib/thread/qmutex.cpp:225
#5 0x00007ffff2eed989 in QMutexLocker::QMutexLocker (this=0x7fffffffc6f0, m=0x7ffff31a4b00 <(anonymous namespace)::Q_QGS_kProtocolManagerPrivate::innerFunction()::holder>)
at /opt/qt5/include/QtCore/qmutex.h:136
#6 0x00007ffff2ee7f27 in KProtocolManager::entryMap (group=...) at /home/vishesh/kde5/src/frameworks/kio/src/core/kprotocolmanager.cpp:294
#7 0x00007ffff2ee5748 in KIO::SlaveConfigPrivate::readGlobalConfig (this=0x7b5d10) at /home/vishesh/kde5/src/frameworks/kio/src/core/slaveconfig.cpp:73
#8 0x00007ffff2ee5fa6 in KIO::SlaveConfig::reset (this=0x7b6b20) at /home/vishesh/kde5/src/frameworks/kio/src/core/slaveconfig.cpp:227
#9 0x00007ffff2ee7dfc in KProtocolManager::reparseConfiguration () at /home/vishesh/kde5/src/frameworks/kio/src/core/kprotocolmanager.cpp:278
#10 0x00007ffff2ea37ee in KIO::SessionData::reset (this=0x7b52f0) at /home/vishesh/kde5/src/frameworks/kio/src/core/sessiondata.cpp:136
#11 0x00007ffff2ea3289 in KIO::SessionData::configDataFor (this=0x7b52f0, configData=..., proto=...) at /home/vishesh/kde5/src/frameworks/kio/src/core/sessiondata.cpp:102
#12 0x00007ffff2edb7f9 in KIO::SchedulerPrivate::metaDataFor (this=0x7b52d0, protocol=..., proxyList=..., url=...)
at /home/vishesh/kde5/src/frameworks/kio/src/core/scheduler.cpp:1049
#13 0x00007ffff2edc18b in KIO::SchedulerPrivate::setupSlave (this=0x7b52d0, slave=0x82d3d0, url=..., protocol=..., proxyList=..., newSlave=true, config=0x0)
at /home/vishesh/kde5/src/frameworks/kio/src/core/scheduler.cpp:1094
#14 0x00007ffff2edb737 in setupSlave (slave=0x82d3d0, url=..., protocol=..., proxyList=..., newSlave=true, config=0x0)
at /home/vishesh/kde5/src/frameworks/kio/src/core/scheduler.cpp:1042
#15 0x00007ffff2eda759 in KIO::ProtoQueue::startAJob (this=0x7c11b0) at /home/vishesh/kde5/src/frameworks/kio/src/core/scheduler.cpp:621
#16 0x00007ffff2edd795 in KIO::ProtoQueue::qt_static_metacall (_o=0x7c11b0, _c=QMetaObject::InvokeMetaMethod, _id=0, _a=0x7fffffffcfa0)
at /home/vishesh/kde5/build/frameworks/kio/src/core/moc_scheduler_p.cpp:244
#17 0x00007ffff40879bf in QMetaObject::activate (sender=0x7c1208, signalOffset=3, local_signal_index=0, argv=0x0)
at /home/vishesh/kde5/qt5/qtbase/src/corelib/kernel/qobject.cpp:3680</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;">This can be tested by running `khotnewstuff "plasmoids.knsrc"` in knewstuff/tests/. Without this patch it just blocks.</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>src/core/kprotocolmanager.cpp <span style="color: grey">(63baa6d)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/119110/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>