Re: [PATCH2/2] winmm: PlaySound concurrency cleanup.

[Eric Pouech]

sharing a global variable between threads without proper sync protection or
atomic operation is the wrong thing to do
moreover you're doing several unrelated changes in the same patch, please
split up

A+

2011/3/28 <Joerg-Cyril.Hoehle(a)t-systems.com>
> Hi,
>
> - PlaySound_Alloc does not call _Free anymore, so it does not use WINMM_cs
>  anymore, which makes it easier to reason about use of critical sections.
> - psStopEvent was never used like an event rather than a boolean.
> - No need to muck with WHDR_DONE.
> - No need to keep the unused thread handle object around.
> - Abort the playloop in case of error, like I did for mciwave.  The error
>  may not be transient.
>
> - Free waveHdr and hEvent after closing hWave.  Either you believe that
>  WAVERR_STILLPLAYING can happen, then obviously waveHdr is in use and must
>  not yet be freed, or you believe checking for STILLPLAYING is a waste of
>  code because waveOutReset must have done its job.
>
> I should have added an ERR() message.
>
> One should think again about what to do in such a case.  Why/when would
>  sleep help?  User-friendlier might be not to wait, not to free WaveHdr
>  and accept to leak resources in such a case.
>
> Actually, waveOutClose can fail with STILLPLAYING because waveOutReset
>  can fail.  If you look at winealsa, you'll see it uses CreateEvent to
>  synchronously wait for acknowledge of the message.  Now guess what
>  happens if CreateEvent fails because memory is tight?
>  What happens in the app when waveOutClose's CreateEvent fails?
>  It's always tough to reason about the many error paths.
>
> Regards,
>        Jörg Höhle

-- 
-- 
Eric Pouech

[PATCH2/2] winmm: PlaySound concurrency cleanup.

Eric,
> sharing a global variable between threads without proper
> sync protection or atomic operation is the wrong thing to do
> Which one?
> bPlaySoundStop is set and reset within the scope of WINMM_cs.

Actually, there's one error: I should have put it at the exact
same location of the Re/SetEvent(psStopEvent) it replaces,
not outside the loop.  This became apparent as I rethought of
the scenario where 2 concurrent invocations of PlaySound need
to interrupt an already running third one.

I'll resubmit ASAP.
> moreover you're doing several unrelated changes
> in the same patch, please split up
> I initially had that as 4-5 patches, then merged them
> for not being of significant value individually.

Regards,
       Jörg Höhle