Re: dsound: Make sure we're holding the lock on Drop And Stop , try 2
Maarten Lankhorst schrieb:
This fixes an interesting deadlock with the alsa driver:
DSOUND_Stop is called, message is added to thread queue. DSOUND_Release is called, message is added to thread queue.
Both are fast messages, so second is added before first.
But this only works if DSOUND_Stop and DSOUND_Release are called from different threads, right?
thread reads: We're going down! And terminates.
DSOUND_Stop's message is never read and it waits till eternity, so starting/stopping never occurs any more.
Isn't the real problem that when ALSA_DestroyRingMessage(*) is called from IDsDriverBuffer_Release it doesn't check if there are still any "fast messages" in the queue which wait for their message to be handled? Just from a quick look it seems you could theoretically trigger the same deadlock with the WODM_RESET & WODM_CLOSE messages when using the waveout system. (*) probably affects all other sound drivers as well which use this (horrible complex) message ring buffer system.
Peter Beutner schreef:
But this only works if DSOUND_Stop and DSOUND_Release are called from different threads, right? It is during shutdown, DSOUND has an internal timer/thread called the mixer, it calls DSOUND_Stop because we signalled it to stop playbacking, then the program calls idirectsoundbuffer_release which calls DSOUND_Release. Since DSOUND_release also unsets hwbuf, the timer would then continue to use an uninitialized waveout interface, which could result in something even uglier. Isn't the real problem that when ALSA_DestroyRingMessage(*) is called from IDsDriverBuffer_Release it doesn't check if there are still any "fast messages" in the queue which wait for their message to be handled? Just from a quick look it seems you could theoretically trigger the same deadlock with the WODM_RESET & WODM_CLOSE messages when using the waveout system. I agree it's horribly complex, I hope to improve that in alsa during summer. However when calling IDsDriverbuffer_Release dsound says it no longer has anything that uses that interface. Because IDsDriverBuffer_Stop hasn't returned yet this is in fact untrue: it is still using idsdriverbuffer, so to make sure it isn't, dsound has to hold the mixer lock so it waits until the mixer is done playing around.
You are _NOT_ allowed to call Release on a interface if you still have something that uses the interface. The bug is definitely in dsound, it just resulted in winealsa being unresponsive. Maarten
Maarten Lankhorst schrieb:
Peter Beutner schreef:
But this only works if DSOUND_Stop and DSOUND_Release are called from different threads, right? It is during shutdown, DSOUND has an internal timer/thread called the mixer, it calls DSOUND_Stop because we signalled it to stop playbacking, then the program calls idirectsoundbuffer_release which calls DSOUND_Release. Since DSOUND_release also unsets hwbuf, the timer would then continue to use an uninitialized waveout interface, which could result in something even uglier. Isn't the real problem that when ALSA_DestroyRingMessage(*) is called from IDsDriverBuffer_Release it doesn't check if there are still any "fast messages" in the queue which wait for their message to be handled? Just from a quick look it seems you could theoretically trigger the same deadlock with the WODM_RESET & WODM_CLOSE messages when using the waveout system. I agree it's horribly complex, I hope to improve that in alsa during summer. That's great ;)
However when calling IDsDriverbuffer_Release dsound says it no longer has anything that uses that interface. Because IDsDriverBuffer_Stop hasn't returned yet this is in fact untrue: it is still using idsdriverbuffer, so to make sure it isn't, dsound has to hold the mixer lock so it waits until the mixer is done playing around.
You are _NOT_ allowed to call Release on a interface if you still have something that uses the interface. The bug is definitely in dsound, it just resulted in winealsa being unresponsive. ok that makes sense. Definitely something wrong in dsound.
But I still think that ALSA_DestroyRingMessage should signal any message left in the queue. Imo it just calls for trouble to expect that messages always are added in the right order.
Peter Beutner schreef:
ok that makes sense. Definitely something wrong in dsound.
But I still think that ALSA_DestroyRingMessage should signal any message left in the queue. Imo it just calls for trouble to expect that messages always are added in the right order. DestroyRingMessage is called in methods or functions similar to free(). If you still use the interface/driver then you shouldn't call free(). If you do then that's not a bug in the driver but in your code. So no.. everything is shutdown and there should be no messages left. Period!
Maarten
Maarten Lankhorst schrieb:
Peter Beutner schreef:
ok that makes sense. Definitely something wrong in dsound.
But I still think that ALSA_DestroyRingMessage should signal any message left in the queue. Imo it just calls for trouble to expect that messages always are added in the right order. DestroyRingMessage is called in methods or functions similar to free(). If you still use the interface/driver then you shouldn't call free(). If you do then that's not a bug in the driver but in your code. So no.. So we expect every application out there to be bug free? :p It is exactly the same thing you do with your FORCE_SHUTDOWN proposal. Even if the application does something stupid we want to make sure that we can cleanup and exit properly. I would rather see a big fat error message than a deadlocked application. But the chance that something happens here is relative small anyway so it's probably not worth fighting for it.
participants (2)
-
Maarten Lankhorst -
Peter Beutner