Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RFC: AUDIO: Add missing (?) mutex locking to MidiDriver_MT32GM::isReady() #3038

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

@eriktorbjorn
Copy link
Member

@eriktorbjorn eriktorbjorn commented Jun 1, 2021

While looking into why Lure of the Temptress takes so long to start with MT-32 audio (turns out that it's because it is setting up a lot of custom sounds?), I noticed what to me looks like a missing mutex lock in MidiDriver_MT32GM::isReady().

Lure of the Temptress calls isRead() to see if the driver has finished processing all the SysEx data. This checks if the queue is empty, but it does so without waiting for the mutex to unlock. Doesn't that mean the queue can be accessed simultaneously from two different threads?

Of course, such problems are notoriously hard to debug. I added some debug() calls, and it looked like it happened a couple of times... assuming I can count on such debug messages to be printed in the correct order.

I tried asking around, but there was no one there who could answer me. So now I'm creating this pull request to see if there's any feedback.

At least it seems to me that the way e.g. Lure of the Temptress calls
isReady() to see if the driver has finished processing all the custom
sounds, _sysExQueue can be accessed by two threads simultaneously.

Which seems like a bad thing to me!
@NMIError
Copy link
Contributor

@NMIError NMIError commented Jun 1, 2021

I posted a reply a little while ago in Discord, but that might have gotten overlooked in the traffic...

The use case for the SysEx queue is:

  1. Put SysExes on the queue
  2. Check isReady while an SDL timer sends the SysExes (possibly using another thread)

isReady uses Queue::isEmpty, which is a read-only operation, so it does not affect the queue in any way. The only possible issue I can see is if Queue::isEmpty returns true when the queue is not actually empty due to some concurrent modification, but I don't think it does that.

@eriktorbjorn
Copy link
Member Author

@eriktorbjorn eriktorbjorn commented Jun 2, 2021

Maybe you're right. Threads and mutexes scare me, so I figured it was better to report it and ask for feedback than to just keep quiet and worry.

@NMIError
Copy link
Contributor

@NMIError NMIError commented Jun 2, 2021

Sure, thanks for taking an interest!

Maybe another dev can add an opinion on possible concurrency issues between Queue::pop and Queue::isEmpty?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants