#5862 closed defect (fixed)
AGOS: Deadlock when quitting Waxworks demo
| Reported by: | lordhoto | Owned by: | eriktorbjorn |
|---|---|---|---|
| Priority: | normal | Component: | Engine: AGOS |
| Version: | Keywords: | ||
| Cc: | Game: | Waxworks |
Description
ScummVM revision 833c692fc96820a47da4ec4068517a85cafb8fc0 Linux/PowerPC (32bit) g++ 4.6.1
Sound driver: AdLib Demo from our demos page.
When exiting the Waxworks Demo at the Accolade logo (via closing the Window) I got a deadlock.
I attached the backtraces for the main thread and the sound thread as files.
Ticket imported from: #3419778. Ticket imported from: bugs/5862.
Attachments (4)
Change History (16)
by , 13 years ago
| Attachment: | thread1.txt added |
|---|
comment:1 by , 13 years ago
This looks like an issue I had with an engine I was working on. It occurs when you try to destruct the midiPlayer/midiDriver while the midiDriver is still assigned to the timerCallback of the player and causes sporadic issues at quit i.e. if the callback is running and locks the mutex.. The code in audio/midiplayer.cpp line 45 i.e. default implementation indicates that it is expected for the destructor of a midiPlayer to unhook the callback prior to driver deletion and player destruction.
comment:2 by , 13 years ago
I have looked at the AGOS code and this seems likely to be the cause. Kirben is going to commit a correction, but hard to know if this is "fixed" as the problem is not reliably replicable anyway.
Lordhoto: It should be noted that all other engines in tree should be quickly reviewed by grep to check that their midiPlayer implementations do unhook the callback in their destructors otherwise they wil likely be affected by similar issues.
comment:3 by , 13 years ago
This bug is nice to get fixed before the release. Raising priority for keeping the track.
comment:4 by , 13 years ago
| Priority: | normal → high |
|---|
comment:5 by , 13 years ago
sev-: Kirben has already committed a correction as d5a763b763a39dbd0873f7eb6e17105a9b1cc958. However, it is not east to test that this is fixed as I indicated earlier i.e. "hard to know if this is "fixed" as the problem is not reliably replicable anyway."
However, the bug was also left open on reviewing the other engines for failure to unhook the MidiDriver timer callbacks. A cursory grep seems to indicate that AGI, CGE, Draci, Hugo, M4, MADE, Parallaction, Prisoner, SAGA, SCI. Tinsel and Touche may need corrections to their MIDIPlayer destructors i.e. missing _driver->setTimerCallback(NULL, NULL); or similar prior to _driver deletion?
comment:6 by , 13 years ago
I was able to reproduce another dead lock in the MIDI code with the recent master in Simon 2. I tried to quit ScummVM via the window close button when the closet appeared in Simon's room in the intro when the deadlock happend.
I attached backtraces from the main and audio thread.
by , 13 years ago
| Attachment: | simon2_main_thread.txt added |
|---|
by , 13 years ago
| Attachment: | simon2_audio_thread.txt added |
|---|
comment:7 by , 7 years ago
| Owner: | changed from to |
|---|---|
| Resolution: | → worksforme |
| Status: | new → closed |
Tried both Waxworks demo and Simon 2 and I couldn't reproduce. Closing this with the assumption that the fix implemented 6 years ago worked
comment:8 by , 7 years ago
| Resolution: | worksforme |
|---|---|
| Status: | closed → new |
This is still a problem, it is just non-deterministic and requires you to get unlucky and have the engine lock its mutex and make a call to the audio mixer after the audio mixer has already locked its mutex but before it has called the engine’s audio callback. With the reduced audio buffer in ScummVM 2.0 this is less likely but the root problem isn’t solved until the double-mutexes are removed, the data members access by both threads are is replaced with lockless atomics (which OSystem does not have), the audio thread stops making callbacks while holding a lock (C++ Core Guidelines CP.22), or the lock attempts are made non-blocking (also not a feature that OSystem has).
comment:9 by , 4 years ago
| Priority: | high → normal |
|---|
comment:10 by , 3 years ago
Is this still relevant? eriktorbjorn recently made a change to the mixer mutex handling which should prevent this mutual thread lockups in different mutexes.
comment:11 by , 3 years ago
| Owner: | changed from to |
|---|---|
| Resolution: | → fixed |
| Status: | new → closed |
This has most probably been fixed with eriktorbjorn's mutex fixes. Assigning this to him
comment:12 by , 3 years ago
I didn't make any changes to the AGOS engine myself, though athrxx did (bug 12617) so I'll trust him here.
Main thread backtrace