Hi Joerg,
Op 15-06-11 10:00, Joerg-Cyril.Hoehle@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