[MPlayer-dev-eng] [PATCH] Prevent ao_win32 from hanging during uninit(0)

[David Bolen]

Reimar Döffinger <Reimar.Doeffinger at gmx.de> writes:

> The whole code is completely wrong: It uses a non-thread-safe buffer in
> a multithreaded context.

Well, to be fair the only thing really written to in both threads are
the integer counters.  The buffers themselves are used safely enough
as long as the full buffer count remains accurate.  (Of course, the
fact that the write routine appears able to reuse a buffer just given
to Windows when presented with a short data block is a problem, but
not so much a threading one).

> More importantly,
>> while(buffered_bytes > 0)usec_sleep(50000);
> optimizes to
>> if (buffered_bytes > 0) while(1) usec_sleep(50000);
> which is an endless loop.
> You'd have to look at the generated assembler to find out if that is
> what causes your issue.

I'll peek a bit, but I definitely have cases in my logs where
buffered_bytes was non-zero and yet the original loop exits just fine.
Also, in testing my patch there were also cases where the loop exited
before exhausting my timeout_cnt, which shouldn't have happened if the
buffered_bytes value wasn't being re-examined.

Perhaps gcc isn't performing that optimization by default.

> By declaring buffered_bytes as volatile this should be fixed, the
> overall code still relies on luck for thread-safety though.

If you mean the risk of compiler optimization, I agree with volatile.
But yeah, we need locking for those cross-thread values.

Any downside to just bounding the loop for now?  I do have a
self-interest in looking into this further but in the meantime, it's a
hang exposure which the patch corrects.  Certainly it's an
improvement, even if imperfect, over the existing code.

-- David