Hi Joerg, Op 15-06-11 10:00, joerg-cyril.hoe...@t-systems.com schreef: > Hi, > > please try and convince me that a deadlock cannot happen. > > http://source.winehq.org/source//dlls/winealsa.drv/mmdevdrv.c > > AudioClient_Start calls > CreateTimerQueueTimer > which creates a new thread periodically invoking the timer callback. > > The callback invokes > alsa_push_buffer_data() > which ceases the audio client object lock (This->lock). > > AudioClient_Stop performs > EnterCriticalSection(&This->lock); then > DeleteTimerQueueTimer(g_timer_q, This->timer, INVALID_HANDLE_VALUE); > > Now imagine Stop is invoked, EnterCriticalSection executed then the thread is > preempted. > Suppose the timer callback kicks in: alsa_push_buffer_data will block on > This->lock. > When the thread is resumed, DeleteTimerQueue will hang forever waiting for > all timer callbacks to come to an end. > > My opinion is that critical sections are great to prevent concurrent > execution while > things are running, but they are hell to tear down properly. > > IMHO, LeaveCriticalSection + Re-EnterCriticalSection around DeleteTimerQueue > is not a solution. > I consider using Leave+EnterCS around anything that can wait/block an > anti-pattern > (there are a few places like that in Wine). I believe a "restart op" is > needed in general > (like a "restart transaction if it failed" in databases) as anything could > have happened while waiting. You're right, This->timer should be set to NULL inside the lock, and DeleteTimerQueueTimer should be run afterwards.
~Maarten
