{"action":"created","issue":{"url":"https://api.github.com/repos/dolphin-emu/dolphin/issues/3794","repository_url":"https://api.github.com/repos/dolphin-emu/dolphin","labels_url":"https://api.github.com/repos/dolphin-emu/dolphin/issues/3794/labels{/name}","comments_url":"https://api.github.com/repos/dolphin-emu/dolphin/issues/3794/comments","events_url":"https://api.github.com/repos/dolphin-emu/dolphin/issues/3794/events","html_url":"https://github.com/dolphin-emu/dolphin/pull/3794","id":150623829,"number":3794,"title":"Core: Add synchronization to state changes (Fix Frame Step and FIFO Player -  Issue 8718)","user":{"login":"EmptyChaos","id":17393426,"avatar_url":"https://avatars.githubusercontent.com/u/17393426?v=3","gravatar_id":"","url":"https://api.github.com/users/EmptyChaos","html_url":"https://github.com/EmptyChaos","followers_url":"https://api.github.com/users/EmptyChaos/followers","following_url":"https://api.github.com/users/EmptyChaos/following{/other_user}","gists_url":"https://api.github.com/users/EmptyChaos/gists{/gist_id}","starred_url":"https://api.github.com/users/EmptyChaos/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/EmptyChaos/subscriptions","organizations_url":"https://api.github.com/users/EmptyChaos/orgs","repos_url":"https://api.github.com/users/EmptyChaos/repos","events_url":"https://api.github.com/users/EmptyChaos/events{/privacy}","received_events_url":"https://api.github.com/users/EmptyChaos/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"milestone":null,"comments":28,"created_at":"2016-04-24T04:22:49Z","updated_at":"2016-05-21T18:51:08Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/dolphin-emu/dolphin/pulls/3794","html_url":"https://github.com/dolphin-emu/dolphin/pull/3794","diff_url":"https://github.com/dolphin-emu/dolphin/pull/3794.diff","patch_url":"https://github.com/dolphin-emu/dolphin/pull/3794.patch"},"body":"<p><del>[Needs PR #3769 as well]</del></p>\r\n\r\nAlternative to #3712 that fixes the underlying race. The basic problem is that `CPU::EnableStepping` is a non-atomic function that was only designed to be called from the Host thread so it races when called from the CPU/GPU or any other system thread. One thread will go through and stop the CPU, mute the audio and pause the FIFO while the other unpauses the CPU, unmutes the audio and unpauses the FIFO resulting in a random final state (e.g. CPU running with no video, CPU and video running with no audio). Other functions which depend on `EnableStepping` like `Core::SetState` and `CPU::Break` inherit those bugs.\r\n\r\nThis PR makes the implicit design choices explicit: `CPU::EnableStepping`, `Core::SetState` and `Core::PauseAndLock` are now Host thread only. `CPU::Break` has been rewritten as a threadsafe way for system threads to do `EnableStepping(true)` only.\r\n\r\n----\r\n\r\n<del>I've removed the anti-deadlock hack (lock timeout) for testing purposes but that needs to go back in before this can be merged.</del> A known source of deadlocks is `wxMsgAlert` (CPU `PanicAlert`s, GUI tries to CORE_PAUSE/PauseAndLock/EnableStepping and deadlocks waiting for the CPU waiting for the GUI waiting for the CPU...), this is somewhat intractable to fix since it requires one of 1) removing wxMsgAlert entirely, 2) shifting Core::SetState/CPU::EnableStepping/Core::PauseAndLock calls onto a dedicated background thread so that the GUI never calls a blocking Core function directly, or 3) spawning an external helper process to display the messages.\r\n\r\n<!-- Reviewable:start -->\r\n---\r\nThis change is [<img src=\"https://reviewable.io/review_button.svg\" height=\"35\" align=\"absmiddle\" alt=\"Reviewable\"/>](https://reviewable.io/reviews/dolphin-emu/dolphin/3794)\r\n<!-- Reviewable:end -->\r\n"},"comment":{"url":"https://api.github.com/repos/dolphin-emu/dolphin/issues/comments/220794304","html_url":"https://github.com/dolphin-emu/dolphin/pull/3794#issuecomment-220794304","issue_url":"https://api.github.com/repos/dolphin-emu/dolphin/issues/3794","id":220794304,"user":{"login":"lioncash","id":712067,"avatar_url":"https://avatars.githubusercontent.com/u/712067?v=3","gravatar_id":"","url":"https://api.github.com/users/lioncash","html_url":"https://github.com/lioncash","followers_url":"https://api.github.com/users/lioncash/followers","following_url":"https://api.github.com/users/lioncash/following{/other_user}","gists_url":"https://api.github.com/users/lioncash/gists{/gist_id}","starred_url":"https://api.github.com/users/lioncash/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/lioncash/subscriptions","organizations_url":"https://api.github.com/users/lioncash/orgs","repos_url":"https://api.github.com/users/lioncash/repos","events_url":"https://api.github.com/users/lioncash/events{/privacy}","received_events_url":"https://api.github.com/users/lioncash/received_events","type":"User","site_admin":false},"created_at":"2016-05-21T18:51:08Z","updated_at":"2016-05-21T18:51:08Z","body":"Alright, given this fixes quite a few races and underlying issues, as well as making stable a much better experience for TASers (and if any regressions related to TASing happens in later dev versions, at least they have a more modern stable release to fall back to as a last resort), I'd like to include this in 5.0. Particularly because it handles a deadlock case more gracefully than what master currently does.\n\nI'll give everyone 24 hours to leave concerns or arguments against doing this.\n\n<details><summary><i>Previously, JMC47 wrote…</i></summary>\n> I could deadlock the GUI thread right now through that.  I run into the bug all the time.  It's not a regression in this Pull Request.\n</details>\n\n\n---\n\nReview status: 2 of 36 files reviewed at latest revision, 1 unresolved discussion.\n\n---\n\n*[Source/Core/Core/HW/CPU.cpp, line 166 \\[r13\\]](https://reviewable.io:443/reviews/dolphin-emu/dolphin/3794#-KIDnogZz7rX0OymfskD:-KIJa5xOGP668et-HV4-:-561118624) ([raw file](https://github.com/dolphin-emu/dolphin/blob/c1944f623b5918ab45152795dea9e49f57878138/Source/Core/Core/HW/CPU.cpp#L166)):*\n<details><summary><i>Previously, EmptyChaos wrote…</i></summary>\n> Like JMC47 said, the `MsgHandler` problem is a pre-existing flaw that I don't see an easy, clean way to fix. IMHO, the best fix is to separate the GUI from the Core using an asynchronous intermediary background thread but implementing that is a large, disruptive patch in itself.\n> \n> Overall, this PR handles the deadlock better than master because master lacks a timeout in `CPU::PauseAndLock` so it freezes permanently whereas this code can recover if you wait 10 seconds for the timeout.\n</details>\nOK, that's more reassuring. I think I'd be more comfortable including this in 5.0 in that case.\n\n---\n\n\n*Comments from [Reviewable](https://reviewable.io:443/reviews/dolphin-emu/dolphin/3794#-:-KIJaKaj2rP_blEQr7nv:-703425026)*\n<!-- Sent from Reviewable.io -->\n"}}
{"actor":{"display_login":"lioncash"}}