Review Request 119110: Release Blocker - KProtocolManager: Fix double mutex locking on a non recursive mutex

Fri Jul 4 22:12:35 UTC 2014

> On July 4, 2014, 7:22 p.m., David Faure wrote:
> > recursive mutexes are more expensive. Can you try this patch instead?
> > http://www.davidfaure.fr/2014/kprotocolmanager.cpp.diff
> 
> David Faure wrote:
>     Actually, nothing beats a unittest :) Added, fix pushed.
>     
>     http://commits.kde.org/kio/842bc8008f5eed03698e8ec67351e791b65ad2b1
>     
>     I'll repack kio after dinner.
> 
> David Faure wrote:
>     Done (v4.100.0-rc2). You can discard this RR.
> 
> Vishesh Handa wrote:
>     Just out of curiosity, what's so expensive about Recursive mutexes?
> 
> Milian Wolff wrote:
>     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.

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.

- David

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/119110/#review61614
-----------------------------------------------------------

On July 4, 2014, 9:58 p.m., Vishesh Handa wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/119110/
> -----------------------------------------------------------
> 
> (Updated July 4, 2014, 9:58 p.m.)
> 
> 
> Review request for KDE Frameworks and David Faure.
> 
> 
> Repository: kio
> 
> 
> Description
> -------
> 
>     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
> 
> 
> Diffs
> -----
> 
>   src/core/kprotocolmanager.cpp 63baa6d 
> 
> Diff: https://git.reviewboard.kde.org/r/119110/diff/
> 
> 
> Testing
> -------
> 
> This can be tested by running `khotnewstuff "plasmoids.knsrc"` in knewstuff/tests/. Without this patch it just blocks.
> 
> 
> Thanks,
> 
> Vishesh Handa
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20140704/8e985e28/attachment-0001.html>