Re: [xine-devel] [patch] protect stream->gapless_switch with the lock

[Stas S. <st...@ak...>]

Hi.

Thibaut Mattern wrote:
>>> 1. It covers only the code path that my program
>>> uses.
>> Actually, missed a few important cases.
>> The attached one covers some more.
> Good catch. The patch looks ok.
Just a side-note: there was also
---
2. I don't beleive this fix is the best possible
one, it is surely not. I beleive this code have
to be revamped to be thread-safe by desing
---
to which you have not commented, so let me
clarify. I do not find this patch correct.
It is technically correct, but theoretically
it is very wrong.
The culprit is along the lines of:
---
       pthread_rwlock_rdlock(&stream->gapless_switch_lock);
       if( !stream->gapless_switch )
         stream->metronom->handle_audio_discontinuity (stream->metronom, DISC_STREAMSTART, 0);
       pthread_rwlock_unlock(&stream->gapless_switch_lock);
---

That looks perfectly correct, but change it
to:
---
       pthread_rwlock_rdlock(&stream->gapless_switch_lock);
       if( !stream->gapless_switch ) {
         pthread_rwlock_unlock(&stream->gapless_switch_lock);
         stream->metronom->handle_audio_discontinuity (stream->metronom, DISC_STREAMSTART, 0);
       } else
         pthread_rwlock_unlock(&stream->gapless_switch_lock);

---
and it suddently stops working, even though
still looks correct.
That's because, instead of protecting the
stream->gapless_switch here, it secretly
protectes it for another thread, until it
come to the right place and take the lock itself.
This is obviously a hack, and that was the reason
I called the underlying code inherently thread-unsafe.
Such a fix will work, and probably leaving it alone
is not a big deal. But if such a hacks multiple,
this may became an endless source of troubles in the
future, I beleive.