Opened 3 years ago
Closed 3 years ago
#12649 closed defect (fixed)
SCUMM: Deadlock in Player_AD on quitting
| Reported by: | eriktorbjorn | Owned by: | eriktorbjorn |
|---|---|---|---|
| Priority: | normal | Component: | Engine: SCUMM |
| Version: | Keywords: | ||
| Cc: | Game: |
Description
Reproduced with Indiana Jones and the Last Crusade, AdLib music, "nuked" OPL emulator.
Steps to reproduce: Start the game, then quit during the intro.
If you are "lucky", the game will deadlock. Judging by debug messages I added, the main thread gets stuck in Player_ADL::~Player_AD() while it's deleting _opl2. The audio thread appears to get stuck in Player_ADL::onTimer(), waiting for the lock to be released.
I don't know if any of the other SCUMM players have the same problem.
Change History (5)
comment:1 by , 3 years ago
comment:2 by , 3 years ago
Do you mean something like I just did in https://github.com/eriktorbjorn/scummvm/commits/public-mixer-mutex ?
(I haven't given this much thought at all, I just did this as an experiment. It may be ridiculously, horrifyingly wrong.)
comment:3 by , 3 years ago
I was more about improving the common code in common/mutex.h/.cpp. But your changes are completely fine for this situation here. So I am all for pushing these commits...
I have done some testing with Loom EGA. Managed to reproduce the issue right away. The SDL audio thread got trapped in Player_AD::onTimer(), the main thread got trapped in MixerImpl::stopHandle(). So that seems exactly what you got...
With your 2 commits on top of that the issue seems to be fixed. In my test the SDL audio thread was held in MixerImpl::mixCallback() during the ~Player_AD destruction after the mutex lock, so that seems to be exactly as it should...
I have some other sound drivers which use the mixer thread and have their own mutexes. After you push your commits I'll try to get rid of some of these, too.
comment:4 by , 3 years ago
I made a pull request of it - https://github.com/scummvm/scummvm/pull/3081 - so that others can chime in too.
comment:5 by , 3 years ago
| Owner: | set to |
|---|---|
| Resolution: | → fixed |
| Status: | new → closed |
The pull request has been merged. I believe this issue is fixed.
It will be the same thing as usually: the mixer thread and the main thread locking each other up in different mutexes (here: Player_AD::_mutex and MixerImpl::_mutex).
Whether something like this may happen in other SCUMM players will depend on whether they have mutexes of their own or not. If the player does not have an extra mutex you should be safe. But in case there is one you have to work around the issue carefully. Or even better: rewrite the mutex code, so the mutexes become globally accessible and the SCUMM AD player can simply use the MixerImpl::_mutex instead of its own thing.