Hi,
Andrew Eikum wrote:
I wonder if it's appropriate for code freeze.
You choose: - either 1.4 keeps the old code as is and suffers random dead-locks, or - 1.4 integrates code void of race-conditions on the basis of a solid design.
When working on this patch, were you able to identify specifically where the contention is?
The "10 liner test hack" I mentioned in bug #29657 was simply to remove OSSpinLock inside the callbacks. No more dead-lock. It worked quite well (over hundred successful test iterations to verify the hypothesis), but it was a time bomb and eventually crashed.
Unlike a 10 line hack, a proper solution costs a hundred lines.
The idea of the InTransition state was to handle the lock release in mid-call. Can we add a check for that state in the callbacks to solve this problem less intrusively?
and then do what?
I've never experienced code that works releasing locks mid-call. Quite to the contrary, I'm convinced that such patterns are a sure indicator of a design flaw and bound to cause trouble.
Of the same vein, one liners like EnterCS This-> playing = StateStopped; LeaveCS are suspicious too. What does that guarantee?
You're right that InTransition is no more needed with my patch. A post code-freeze patch can clean up. More specifically, I've kept it so far because of Wait(event) in Stop, outside the lock.
While winealsa would benefit too from a lock-free player as sketched in bug #29531, I'm not considering submitting it for 1.4. The difference is that some apps may suffer audio glitches with winealsa, however any app can suffer a random freeze with winecoreaudio. (Well, perhaps I'll write it meanwhile and if it exhibits significant improvements, submit it anyway).
Next on my list for 1.4 is #28388. There too, a random dead-lock in any MIDI playing app is not acceptable IMHO.
GetPosition is also crucial for 1.4, regardless of code-freeze.
Regards, Jörg Höhle