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

SCI: avoid deadlocks by using only one mutex #3933

Merged
merged 2 commits into from Jun 3, 2022
Merged

Conversation

athrxx
Copy link
Member

@athrxx athrxx commented Jun 1, 2022

Make use of the mixer's mutex instead of creating a new one. Otherwise there can still be lock-ups when the main thread and the mixer thread lock each other up in different mutexes (causing a deadlock/freeze). I just noticed this with KQ5 FM-Towns.

We had the same issue in AGOS, it was fixed by allowing access to the mixer's mutex. We can use the same thing here...

Make use of the mixer's mutex instead of creating a new one. Otherwise there can still be lock-ups when the main thread and the mixer thread lock each other up in different mutexes (causing a deadlock/freeze). I just noticed this with KQ5 FM-Towns.

We had the same issue in AGOS, it was fixed by allowing to access the mixer's mutex. We can use the same thing here...
@@ -260,7 +260,7 @@ class SciMusic : public Common::Serializable {
// MIDI parser and to the MIDI driver/player. Note that guarded code must NOT
// include references to the mixer, otherwise there will probably be situations
// where a deadlock can occur
Common::Mutex _mutex;
Common::Mutex &_mutex;
Copy link
Member

@sluicebox sluicebox Jun 2, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do the comments for _mutex need updating now that it's a reference to the mixer's mutex?

Copy link
Member Author

@athrxx athrxx Jun 2, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, I have updated the comment which has become somewhat incorrect. If you have a ref to the mixer's own mutex you can have as many mixer calls as you want inside the guarded space.

(after turning _mutex into a reference to the mixers's mutex)
@sluicebox
Copy link
Member

@sluicebox sluicebox commented Jun 3, 2022

Since @athrxx has encountered a deadlock with the existing code and confirmed that this fixed it, and that it's a fix that's already successfully been applied, I'm all in favor. I'd like @bluegr to give the third eyeball and confirm this before merging.

@sluicebox sluicebox requested a review from bluegr Jun 3, 2022
@bluegr
Copy link
Member

@bluegr bluegr commented Jun 3, 2022

Well done finding this deadlock! The change makes sense, as it avoids deadlocks between threads.
A straightforward change
Thanks again! Merging

bluegr
bluegr approved these changes Jun 3, 2022
@bluegr bluegr merged commit 2803adc into scummvm:master Jun 3, 2022
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants