[mythtv] [mythtv-commits] Ticket #6305: Incorrect QWaitCondition & mutex usage in multiple places

[Janne Grunau]

Hi,

On Wednesday 11 March 2009 18:00:45 MythTV wrote:
> #6305: Incorrect QWaitCondition & mutex usage in multiple places
> ---------------------------------------+-----------------------------
>------- Reporter:  tomimo at ncircle.nullnet.fi  |        Owner:  ijr
>      Type:  defect                     |       Status:  closed
>  Priority:  minor                      |    Milestone:  unknown
> Component:  MythTV - General           |      Version:  head
>  Severity:  medium                     |   Resolution:  invalid
>   Mlocked:  0                          |
> ---------------------------------------+-----------------------------
>-------

first, I closed the ticket primarily because I fail to see how the bug 
you reported (status page loading causes deadlock) is related to 
QWaitcondition usage. As far as I can see QWaitCondition usage in TVRec 
is correct even though ugly due to the recursive mutex stateChangeLock.

> Comment(by tomimo at ncircle.nullnet.fi):
>
>  Replying to [comment:2 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 attached patch of course doesn't fix all the places, because I
> noticed that there exists at least 15 locations around the code which
> has the same QWaitCondition-usage flaw and I wanted to test my fixes
> as before committing it all.

The problem with the patch is that it changes the usage sematics of 
QWaitConditions in recorderbase.cpp but doesn't touch the very same 
QWaitcondition in derived classes.

> I did however want to bring some
> attention to this problem right a way.
>
>  I understand very well your comment about the purpose of the
> discussed code, but what I don't understand is that why would you
> want to keep the code which is obviously wrong ? This is a good
> example of QWaitCondition- usage where a wakeOne() or wakeAll()
> signal can be missed by mistake and even though the timedcondwait
> will eventually timeout and fix a potential deadlock situation, why
> shouldn't we fix the code to work properly in the first place ?

I'm not sure that the usage is incorrect. I'm not familiar enough with 
the recorder code to say if we might call PauseAndWait() without calling 
WaitForPause(). If that can happen your patch is as wrong as the current 
code is if WaitForPause() is called at least once before PauseAndWait().

The patch fixes the problem also only if there is at most one thread 
calling WaitForPause().

>> 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.
>
>  I'm little bit amazed to see that you have closed this ticket, even
> though you admit yourself (and in the comments of the code) that the
> code in question is propably not correct. It's not that hard to fix
> the detected errors in order to make the MythTV more stable.

See above. The patch is unrelated to your deadlock and not necessarily 
more correct than the original code.

> I'm sure
> you that you know very well that the small loop wget test proves
> absolutely nothing about the stability of the code,

Well, it proves that the error is not reproduceable on my machine and 
three simultaneously status page request with 100msecs pause for a 
couple of minutes is much more stress than extensive status page usage. 
Nothing more but nothing less.

> especially as
> I've been (unfortunately) able to get the mythbackend hang in two
> different hosts once or twice a day without fixing these
> QWaitCondition cases.

Please attach a full backtrace of all threads to a new ticket.

>  I have no problem in submitting patches to MythTV, but of course I'd
> like to know that you'd be willing to accept them in some form as
> well.

The recorderbase qwaitcondition patch in it's current form probably not. 
Also not  patches "fixing" QWaitCondition usage blindly thorough the 
entire source.

Patches which fixes your deadlock are of course welcome. May them fixing 
some QWaitCondition usage or some other deadlock.

Janne