[MPlayer-dev-eng] [PATCH] Fix dead lock in ao_macosx when switch stream format.

[Ulion]

2007/11/5, Reimar Döffinger <Reimar.Doeffinger at stud.uni-karlsruhe.de>:
> Hello,
> On Mon, Nov 05, 2007 at 07:59:25PM +0800, Ulion wrote:
> > Hello,
> >
> > I found a dead lock occur in ao_macosx (that part of code is from my
> > patch based on vlc's source) when switch stream format for use digital
> > passthrough output.
> > The detail is, if the pthread_cond_timedwait timeouted, and detected
> > the stream format already changed, or after 5 times
> > pthread_cond_timedwait timeouted, when removing the stream listener,
> > while on the other hand, the listener callback is blocked to wait the
> > lock since it may be callbacked after pthread_cond_timedwait
> > timeouted.
> > Here's the fix, one thing to make the pthread_cond_timedwait wait
> > longer, another thing to make the callback does not wait the lock,
> > just siginal the wait cond. Now this code will not dead lock any more.
> >
> > If  no objects, I will apply this soon in two days.
>
> This should only be applied after the loads of other bugs and problems
> have been fixed and then it is still necessary.
> Firstly, if AudioStreamRemovePropertyListener fails the lock will not be
> released and not destroyed.

You are right, but this call should not failed if the calling
parameters is correct (never heared it failed). It only block when the
callback tobe removed is just running and after the callback returns
the remove will finished.
I didn't notice this remove call failing caused leak problem before
you said that, but by now I still did not figure a way out to handle
this situation properly. Maybe we can malloc the struct w to avoid the
callback possiblly use the destroyed local variable, but this should
be another patch, it's no matter with the dead lock.

> Secondly, even after your change, the callback might use a condition
> that has already been destroyed.
> Which probably is the thing that causes the deadlock in the first place
> (using a lock that has been destroyed), at least as I understand you
> description.

You got wrong meaning, with gdb to check it on my a little slow
powerpc system, the change stream and callback take a time longer than
0.5 second, but the format detection in the loop found the format
already changed, it then to remove the listener, the remove call is
waiting the listener callback return, but at mean time the listener is
blocking and wait the mutex lock, so dead locked.

So if we remove the wait mutex in the callback, here will be no
deadlock, and after the listener return, the remove call wil finished,
everything is ok.

And if the five loop times finished, format still not changed, the
remove call still can safely end the possibility of the listener using
invalid variables. After remove call, the listener callback will not
be called.

Now the only thing we need to worry is the failing case of remove
call. As I said, it rarely happens, maybe never happened. So if you
think it necessary, I will make another patch to make the struck m
mallocated, or any good ideas?

> And btw. "timeouted" for all I know is not a word (and makes it hard for
> me to read), "timed out" is. The same should apply to "callbacked", though
> a simple "called" sounds better to me anyway.
>

Sorry for this, I will try to remember it.

-- 
Ulion