[mythtv-commits] Ticket #10870: Race condition between StreamHandler::RemoveListener and StreamHandler::AddListener

[MythTV]

#10870: Race condition between StreamHandler::RemoveListener and
StreamHandler::AddListener
----------------------------------------+----------------------------
 Reporter:  roger@…                     |          Owner:
     Type:  Bug Report - Hang/Deadlock  |         Status:  new
 Priority:  minor                       |      Milestone:  unknown
Component:  MythTV - General            |        Version:  0.25-fixes
 Severity:  medium                      |     Resolution:
 Keywords:  AddListener RemoveListener  |  Ticket locked:  0
----------------------------------------+----------------------------

Comment (by Roger James <roger@…>):

 The patch below is solely intended as a work around, and I have put it
 here for information only. I have regression tested it and done some very
 basic testing against the issue I reported. It appears to work. I don't
 like the idea of introducing extra locks to fix a bug, but it is the least
 intrusive change I could think of. A better fix would possibly be the
 implementation of a proper finite state machine with queued event
 handling.

 Roger

 {{{
 $ git diff streamhandler.cpp
 diff --git a/mythtv/libs/libmythtv/streamhandler.cpp
 b/mythtv/libs/libmythtv/str
 index 2b0a7b6..28de368 100644
 --- a/mythtv/libs/libmythtv/streamhandler.cpp
 +++ b/mythtv/libs/libmythtv/streamhandler.cpp
 @@ -133,7 +133,8 @@ void StreamHandler::RemoveListener(MPEGStreamData
 *data)

  void StreamHandler::Start(void)
  {
 -    QMutexLocker locker(&_start_stop_lock);
 +    QMutexLocker
 start_stop_procedure_locker(&_start_stop_procedure_lock);
 +    QMutexLocker start_stop_locker(&_start_stop_lock);

      if (_running)
      {
 @@ -173,7 +174,8 @@ void StreamHandler::Start(void)

  void StreamHandler::Stop(void)
  {
 -    QMutexLocker locker(&_start_stop_lock);
 +    QMutexLocker
 start_stop_procedure_locker(&_start_stop_procedure_lock);
 +    QMutexLocker start_stop_locker(&_start_stop_lock);

      do
      {
 $ git diff streamhandler.h
 diff --git a/mythtv/libs/libmythtv/streamhandler.h
 b/mythtv/libs/libmythtv/strea
 index 1db0584..ae0bc80 100644
 --- a/mythtv/libs/libmythtv/streamhandler.h
 +++ b/mythtv/libs/libmythtv/streamhandler.h
 @@ -45,7 +45,7 @@ class PIDInfo
  typedef QMap<uint,PIDInfo*> PIDInfoMap;

  // locking order
 -// _pid_lock -> _listener_lock -> _start_stop_lock
 +// _pid_lock -> _listener_lock -> _start_stop_procedure_lock ->
 _start_stop_lock

  class StreamHandler : protected MThread, public DeviceReaderCB
  {
 @@ -93,7 +93,7 @@ class StreamHandler : protected MThread, public
 DeviceReaderCB
      virtual void RemoveNamedOutputFile(const QString &filename) {}
      /// At minimum this sets _running_desired, this may also send
      /// signals to anything that might be blocking the run() loop.
 -    /// \note: The _start_stop_lock must be held when this is called.
 +    /// \note: The _start_stop_procedure_lock and the _start_stop_lock
 must be
      virtual void SetRunningDesired(bool desired) { _running_desired =
 desired;

    protected:
 @@ -101,6 +101,7 @@ class StreamHandler : protected MThread, public
 DeviceReader
      bool              _needs_buffering;
      bool              _allow_section_reader;

 +    mutable QMutex    _start_stop_procedure_lock; /// Serialises calls to
 Start
      mutable QMutex    _start_stop_lock;
      volatile bool     _running_desired;
      volatile bool     _error;
 $
 }}}

-- 
Ticket URL: <http://code.mythtv.org/trac/ticket/10870#comment:1>
MythTV <http://code.mythtv.org/trac>
MythTV Media Center