Make the emulation stop asynchronous to prevent deadlocks. #515
Conversation
|
I forgot to say I am in full support of this build. Tons of on close crashes can happen when I'm being stupid. Actually tested it, none of the normal tricks that have the potential for crashing dolphin work any more. I tried closing while loading a savestate, closing while loading a bad savestate, closing while switching internal resolutions, closing while saving (figured it was worth a try,) closing with the button and escape at the same time. Honestly, it seems rock solid. Even if there are problems with this, I'm guessing it fixes more than it breaks. |
|
@JMC47 That means there were probably more components than just the VideoBackend that would deadlock if the GUI thread is blocked. |
|
To prevent potential problems we should probably disable much of the user interface while we're in the For now I've added functionality to update the GUI and disable the play button to show the stopping state. |
|
Looks good to me |
|
I did not notice any regressions while testing. Hopefully it can be merged so that work continues on exclusive fullscreen. There's only so much I can test for closing the emulator. |
| @@ -81,4 +82,8 @@ void UpdateTitle(); | |||
| // the return value of the first call should be passed in as the second argument of the second call. | |||
| bool PauseAndLock(bool doLock, bool unpauseOnUnlock=true); | |||
|
|
|||
| // for calling back into UI code without introducing a dependency on it in core | |||
| typedef void(*CallbackFunc)(void); | |||
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
|
Welp.. I'd be happier if the whole startup/shutdown code were just rewritten from scratch, but since the net effect of this branch seems to be fixing a lot of dumb crashes rather than introducing new instabilities, it's fine with me. Other than the comments I added, LGTM. |
| @@ -83,6 +83,7 @@ bool g_bStarted = false; | |||
| void *g_pWindowHandle = nullptr; | |||
| std::string g_stateFileName; | |||
| std::thread g_EmuThread; | |||
| static CallbackFunc g_onAfterStopCb = nullptr; | |||
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This may expose bugs which relied on the Main Thread to be suspended in the stopping state.
…lized. This properly handles the stopping state and more accurately represents the intended check.
…ted yet. Fixes a null pointer exception when the user starts the recording during a state transition.
|
What has changed in the new series of commits? Was it just addressing review comments? |
|
@neobrain Sorry about that, I addressed your comments and I updated some more state checks in the db7e746 commit. If you could do another review of that commit it'd be great. And I think commit 4df00ae should probably be cherry-picked for PR #587, it was something I found during my search for state checks but it is more relevant for that PR than this one. |
|
@Armada651: This comment grants you the permission to merge this pull request whenever you think it is ready. After addressing the remaining comments, click this link to merge. If all you did was addressing my comments and adding more stability, this should be fine to merge. Well, your call anyway ;) @dolphin-emu-bot allowmerge |
|
@dolphin-emu-bot rebuild |
Make the emulation stop asynchronous to prevent deadlocks.
This PR moves the synchronisation to the last possible moment when we're already certain that the EmuThread has been shut down.
It fixes a potential deadlock where the VideoBackend is waiting for the GUI thread and vice versa. This deadlock seems to only be triggered by exclusive fullscreen, thus this PR is required for PR #506.
This PR has the potential to expose race conditions in code that does not properly check the emulation state so it should be tested thoroughly.