dead-lock free design

Hi,

there are two guidelines that I identified while trying to prevent
dead-locks with critical sections and access to uninitialiased memory
in Wine.  My latest patch shows both applications.
http://www.winehq.org/pipermail/wine-patches/2011-June/103247.html

1. In callbacks, detect when it should not run anymore and exit *early*.

void CALLBACK mycallback (void* user) {
    struct mycbdata * mycb = user;
    EnterCriticalSection(mycb->lock);
    if(!mycb->started || mycb->shutdown)
        { LeaveCriticalSection; return; }
    /* else do actual work */

This is best done after EnterCS, because the critical section is the
most likely place where the callback thread will have been preempted.

Note that this doesn't prevent mycb from pointing to free'ed memory.
You may only deallocate resources once you've made sure that the
callback cannot be invoked any more.

2. Use two phases to end cleanly.  Delegate part of the work to a later stage.
   Pause is not Stop.
   Stop is not Close.
   Close is not Shutdown.
   Shutdown is not DLL Unload.

My current patch applies this idea partially.
DeleteTimerQueueTimer() is called within a CS, whereas Wait() is
called afterwards (which is safe because the mmdevapi object is not
used any more).  The original deadlock happened because
DeleteTimerQueue and Wait were combined in one call, within the CS.

Waiting for completion is *essential* with the DeleteTimerQueueTimer
API because that's the only means to ensure that callbacks are done.
Then only can you free the memory that mycb above points to.

A variation is to partition the work among AudioClient_Stop and
AudioClient_Release.  In Stop() call DeleteTimerQueueTimer() but Wait
for completion of the callbacks in Release().

Alternatively, in Stop() use ChangeTimerQueueTimer(DueTime=INFINITE)
to prevent firing new callbacks and use DeleteTimerQueue in Release().

I didn't write my patch that way yet because it requires some
attention in AudioClient_Start() when restarting the timer callbacks.

Why does the rule work?  Functions likes Play/Pause/Stop
typically prevent concurrent access (either from the app or from
internal callback threads) using critical sections.  At Close/Release
time, the typical assumption is that there's no more concurrency
-- at least from the side of the application.

Partitioning the work allows for plenty of new possibilities.
E.g. one known bug in MS-Windows is that MCI/winmm's Stop() may take a
lot of time, freezing the application.  However, if Stop() simply
sends a stop signal to the player thread, without waiting for the
thread to actually stop, it can be plenty fast.  Likewise, if
mmdevapi's AudioClient_Stop() simply sets This->started to FALSE, the
callback will still be ticking, but not disturb.  Fast too if it calls
DeleteTimerQueueTimer() but delegates the Wait to AudioClient_Release().

One drawback is that resources will be allocated for longer than with
the naïve approach.

Sometimes, the partitioning decision depends on what state you want to
guarantee after Pause, Stop or Close.  This is rarely documented.
E.g. for MCI's audio player you want to make sure that an immediately
following Open won't fail when Wine's winmm ALSA or OSS drivers only
support opening one device at a time.

Regards,
    Jörg Höhle