[mythtv-commits] Ticket #6305: Incorrect QWaitCondition & mutex usage in multiple places
MythTV
mythtv at cvs.mythtv.org
Wed Mar 11 14:59:20 UTC 2009
#6305: Incorrect QWaitCondition & mutex usage in multiple places
---------------------------------------+------------------------------------
Reporter: tomimo at ncircle.nullnet.fi | Owner: ijr
Type: defect | Status: new
Priority: minor | Milestone: unknown
Component: MythTV - General | Version: head
Severity: medium | Resolution:
Mlocked: 0 |
---------------------------------------+------------------------------------
Old description:
> I've noticed a quite frequent hang due to frequent status queries:
>
> Example:
>
> Thread 12 (Thread 0x49396950 (LWP 11152)):
> #0 0x00007fdd7ee1c2d9 in pthread_cond_wait@@GLIBC_2.3.2 () from
> /lib/libpthread.so.0
> #1 0x00007fdd7d4ebb5b in ?? () from /usr/lib/libQtCore.so.4
> #2 0x00007fdd7d4e73cd in QMutex::lock () from /usr/lib/libQtCore.so.4
> #3 0x0000000000438e31 in QMutexLocker::relock (this=0x49393780) at
> /usr/include/qt4/QtCore/qmutex.h:116
> #4 0x000000000043bdd9 in QMutexLocker (this=0x49393780, m=0xa77af8) at
> /usr/include/qt4/QtCore/qmutex.h:98
> #5 0x00007fdd833642a5 in TVRec::IsBusy (this=0xa77a20, busy_input=0x0,
> time_buffer=5) at tv_rec.cpp:2466
> #6 0x00000000004422a7 in EncoderLink::IsBusy (this=0xa9a530,
> busy_input=0x0, time_buffer=5) at encoderlink.cpp:102
> #7 0x00000000004ff7fa in GetCurrentMaxBitrate (encoderList=0x776cc8) at
> backendutil.cpp:47
> #8 0x00000000005012e8 in BackendQueryDiskSpace (strlist=@0x49394ba0,
> encoderList=0x776cc8, consolidated=true, allHosts=true) at
> backendutil.cpp:193
> #9 0x0000000000453e96 in HttpStatus::FillStatusXML (this=0x7fdd70029170,
> pDoc=0x493952d0) at httpstatus.cpp:337
> #10 0x0000000000456fd3 in HttpStatus::GetStatusHTML (this=0x7fdd70029170,
> pRequest=0x7fdd7052ce10) at httpstatus.cpp:134
> #11 0x0000000000457236 in HttpStatus::ProcessRequest
> (this=0x7fdd70029170, pRequest=0x7fdd7052ce10) at httpstatus.cpp:88
> #12 0x00007fdd81a049be in HttpServer::DelegateRequest
> (this=0x7fdd7001d600, pThread=0x7fdd70009170, pRequest=0x7fdd7052ce10) at
> httpserver.cpp:171
> #13 0x00007fdd81a05196 in HttpWorkerThread::ProcessWork
> (this=0x7fdd70009170) at httpserver.cpp:297
> #14 0x00007fdd81a01879 in WorkerThread::WakeForWork (this=0x7fdd70009170)
> at threadpool.cpp:218
> #15 0x00007fdd81a01c88 in WorkerEvent::customEvent (this=0xa968a0,
> e=0x7fdd68830530) at threadpool.cpp:135
> #16 0x00007fdd7d5e3dfd in QObject::event () from /usr/lib/libQtCore.so.4
> #17 0x00007fdd7dd8fc3d in QApplicationPrivate::notify_helper () from
> /usr/lib/libQtGui.so.4
> #18 0x00007fdd7dd979ba in QApplication::notify () from
> /usr/lib/libQtGui.so.4
> #19 0x00007fdd7d5d4d61 in QCoreApplication::notifyInternal () from
> /usr/lib/libQtCore.so.4
> #20 0x00007fdd7d5d59fa in QCoreApplicationPrivate::sendPostedEvents ()
> from /usr/lib/libQtCore.so.4
> #21 0x00007fdd7d5fd4d3 in ?? () from /usr/lib/libQtCore.so.4
> #22 0x00007fdd79e14d3b in g_main_context_dispatch () from
> /usr/lib/libglib-2.0.so.0
> #23 0x00007fdd79e1850d in ?? () from /usr/lib/libglib-2.0.so.0
> #24 0x00007fdd79e186cb in g_main_context_iteration () from
> /usr/lib/libglib-2.0.so.0
> #25 0x00007fdd7d5fd15f in QEventDispatcherGlib::processEvents () from
> /usr/lib/libQtCore.so.4
> #26 0x00007fdd7d5d3682 in QEventLoop::processEvents () from
> /usr/lib/libQtCore.so.4
> #27 0x00007fdd7d5d380d in QEventLoop::exec () from
> /usr/lib/libQtCore.so.4
> #28 0x00007fdd7d4e93f8 in QThread::exec () from /usr/lib/libQtCore.so.4
> #29 0x00007fdd81a0146d in WorkerThread::run (this=0x7fdd70009170) at
> threadpool.cpp:265
> #30 0x00007fdd7d4ec362 in ?? () from /usr/lib/libQtCore.so.4
> #31 0x00007fdd7ee183ea in start_thread () from /lib/libpthread.so.0
> #32 0x00007fdd7ca4acbd in clone () from /lib/libc.so.6
> #33 0x0000000000000000 in ?? ()
>
> It looks there is multiple places where the QWaitCondition.wait/wakeall
> have been implemented incorrectly and therefore the code doesn't work as
> intended.
>
> Example:
>
> // Qt4 requires a QMutex as a parameter...
> // not sure if this is the best solution. Mutex Must be locked before
> wait.
> QMutex mutex;
> mutex.lock();
>
> while (encoding)
> {
> if (request_pause)
> {
> mainpaused = true;
> pauseWait.wakeAll();
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> No locks around this and no predicative set
> to make sure that the signal is detected in the receiver
>
> if (IsPaused() && tvrec)
> tvrec->RecorderPaused();
>
> unpauseWait.wait(&mutex, 100);
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> Uses a bogus mutex, doesn't have a proper locking around
> the wait-call and no predicative checking in order to detect
> if the wake-call has already been sent before we come to the wait()-call.
>
> if (cleartimeonpause)
> gettimeofday(&stm, &tzone);
> continue;
> ------------------------------------------------------------
>
> I'll attach a patch file which shows how I fixed the problem in
> recorderbase.cpp and .h. Because these pauseWait's and unpauseWait's are
> being used in several places, I'll upload a full patch after I've
> verified that my fix doesn't have anmy additional bugs.
New description:
I've noticed a quite frequent hang due to frequent status queries:
Example:
{{{
Thread 12 (Thread 0x49396950 (LWP 11152)):
#0 0x00007fdd7ee1c2d9 in pthread_cond_wait@@GLIBC_2.3.2 () from
/lib/libpthread.so.0
#1 0x00007fdd7d4ebb5b in ?? () from /usr/lib/libQtCore.so.4
#2 0x00007fdd7d4e73cd in QMutex::lock () from /usr/lib/libQtCore.so.4
#3 0x0000000000438e31 in QMutexLocker::relock (this=0x49393780) at
/usr/include/qt4/QtCore/qmutex.h:116
#4 0x000000000043bdd9 in QMutexLocker (this=0x49393780, m=0xa77af8) at
/usr/include/qt4/QtCore/qmutex.h:98
#5 0x00007fdd833642a5 in TVRec::IsBusy (this=0xa77a20, busy_input=0x0,
time_buffer=5) at tv_rec.cpp:2466
#6 0x00000000004422a7 in EncoderLink::IsBusy (this=0xa9a530,
busy_input=0x0, time_buffer=5) at encoderlink.cpp:102
#7 0x00000000004ff7fa in GetCurrentMaxBitrate (encoderList=0x776cc8) at
backendutil.cpp:47
#8 0x00000000005012e8 in BackendQueryDiskSpace (strlist=@0x49394ba0,
encoderList=0x776cc8, consolidated=true, allHosts=true) at
backendutil.cpp:193
#9 0x0000000000453e96 in HttpStatus::FillStatusXML (this=0x7fdd70029170,
pDoc=0x493952d0) at httpstatus.cpp:337
#10 0x0000000000456fd3 in HttpStatus::GetStatusHTML (this=0x7fdd70029170,
pRequest=0x7fdd7052ce10) at httpstatus.cpp:134
#11 0x0000000000457236 in HttpStatus::ProcessRequest (this=0x7fdd70029170,
pRequest=0x7fdd7052ce10) at httpstatus.cpp:88
#12 0x00007fdd81a049be in HttpServer::DelegateRequest
(this=0x7fdd7001d600, pThread=0x7fdd70009170, pRequest=0x7fdd7052ce10) at
httpserver.cpp:171
#13 0x00007fdd81a05196 in HttpWorkerThread::ProcessWork
(this=0x7fdd70009170) at httpserver.cpp:297
#14 0x00007fdd81a01879 in WorkerThread::WakeForWork (this=0x7fdd70009170)
at threadpool.cpp:218
#15 0x00007fdd81a01c88 in WorkerEvent::customEvent (this=0xa968a0,
e=0x7fdd68830530) at threadpool.cpp:135
#16 0x00007fdd7d5e3dfd in QObject::event () from /usr/lib/libQtCore.so.4
#17 0x00007fdd7dd8fc3d in QApplicationPrivate::notify_helper () from
/usr/lib/libQtGui.so.4
#18 0x00007fdd7dd979ba in QApplication::notify () from
/usr/lib/libQtGui.so.4
#19 0x00007fdd7d5d4d61 in QCoreApplication::notifyInternal () from
/usr/lib/libQtCore.so.4
#20 0x00007fdd7d5d59fa in QCoreApplicationPrivate::sendPostedEvents ()
from /usr/lib/libQtCore.so.4
#21 0x00007fdd7d5fd4d3 in ?? () from /usr/lib/libQtCore.so.4
#22 0x00007fdd79e14d3b in g_main_context_dispatch () from
/usr/lib/libglib-2.0.so.0
#23 0x00007fdd79e1850d in ?? () from /usr/lib/libglib-2.0.so.0
#24 0x00007fdd79e186cb in g_main_context_iteration () from
/usr/lib/libglib-2.0.so.0
#25 0x00007fdd7d5fd15f in QEventDispatcherGlib::processEvents () from
/usr/lib/libQtCore.so.4
#26 0x00007fdd7d5d3682 in QEventLoop::processEvents () from
/usr/lib/libQtCore.so.4
#27 0x00007fdd7d5d380d in QEventLoop::exec () from /usr/lib/libQtCore.so.4
#28 0x00007fdd7d4e93f8 in QThread::exec () from /usr/lib/libQtCore.so.4
#29 0x00007fdd81a0146d in WorkerThread::run (this=0x7fdd70009170) at
threadpool.cpp:265
#30 0x00007fdd7d4ec362 in ?? () from /usr/lib/libQtCore.so.4
#31 0x00007fdd7ee183ea in start_thread () from /lib/libpthread.so.0
#32 0x00007fdd7ca4acbd in clone () from /lib/libc.so.6
#33 0x0000000000000000 in ?? ()
}}}
It looks there is multiple places where the QWaitCondition.wait/wakeall
have been implemented incorrectly and therefore the code doesn't work as
intended.
Example:
{{{
// Qt4 requires a QMutex as a parameter...
// not sure if this is the best solution. Mutex Must be locked before
wait.
QMutex mutex;
mutex.lock();
while (encoding)
{
if (request_pause)
{
mainpaused = true;
pauseWait.wakeAll();
}}}
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
No locks around this and no predicative set
to make sure that the signal is detected in the receiver
{{{
if (IsPaused() && tvrec)
tvrec->RecorderPaused();
unpauseWait.wait(&mutex, 100);
}}}
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Uses a bogus mutex, doesn't have a proper locking around
the wait-call and no predicative checking in order to detect
if the wake-call has already been sent before we come to the wait()-call.
{{{
if (cleartimeonpause)
gettimeofday(&stm, &tzone);
continue;
}}}
------------------------------------------------------------
I'll attach a patch file which shows how I fixed the problem in
recorderbase.cpp and .h. Because these pauseWait's and unpauseWait's are
being used in several places, I'll upload a full patch after I've verified
that my fix doesn't have anmy additional bugs.
--
Comment(by janne):
I don't think the patch for Recorderbase is correct. The QWaitconditions
are used as interuptible sleep. There is no need for useful mutexes in
this usage case. Another problem with the patch is that it doesn't change
the usage in derived classes.
the member variables paused and request_pause could probably be guarded by
mutexes which should be used in the wait conditions in recorderbase.
I'm ready to accept that our standard Qt4 port of QWaitCondition with
adding a otherwise unused mutex might create deadlocks in some cases but
recorderbase is a bad example.
--
Ticket URL: <http://svn.mythtv.org/trac/ticket/6305#comment:2>
MythTV <http://www.mythtv.org/>
MythTV
More information about the mythtv-commits
mailing list