From e67e646ca52320ea72e84b37b3e451a59b3f389f Mon Sep 17 00:00:00 2001 From: CasualPokePlayer <50538166+CasualPokePlayer@users.noreply.github.com> Date: Thu, 26 Jan 2023 07:12:54 -0800 Subject: [PATCH] fix deadlock when loading state with hotkeys and RAIntegration is active hotkeys don't go through messages so unlike other methods of loading a state this caused a deadlock (load state implies memory peeking to restore state) --- src/BizHawk.Client.EmuHawk/MainForm.cs | 7 +- .../RetroAchievements/RAIntegration.cs | 18 +++-- .../RetroAchievements.Memory.cs | 80 +++++++++++++------ 3 files changed, 74 insertions(+), 31 deletions(-) diff --git a/src/BizHawk.Client.EmuHawk/MainForm.cs b/src/BizHawk.Client.EmuHawk/MainForm.cs index c838be0bcf..cf34f0af75 100644 --- a/src/BizHawk.Client.EmuHawk/MainForm.cs +++ b/src/BizHawk.Client.EmuHawk/MainForm.cs @@ -810,11 +810,16 @@ namespace BizHawk.Client.EmuHawk Render(); + // HACK: RAIntegration might peek at memory during messages + // we need this to allow memory access here, otherwise it will deadlock var raMemHack = (RA as RAIntegration)?.ThisIsTheRAMemHack(); + raMemHack?.Enter(); CheckMessages(); - if (RA is not null) raMemHack?.Dispose(); + // RA == null possibly due MainForm Dispose disposing RA (which case Exit is not valid anymore) + // RA != null possibly due to RA object being created (which case raMemHack is null, as RA was null before) + if (RA is not null) raMemHack?.Exit(); if (_exitRequestPending) { diff --git a/src/BizHawk.Client.EmuHawk/RetroAchievements/RAIntegration.cs b/src/BizHawk.Client.EmuHawk/RetroAchievements/RAIntegration.cs index 87a227b339..67e026e936 100644 --- a/src/BizHawk.Client.EmuHawk/RetroAchievements/RAIntegration.cs +++ b/src/BizHawk.Client.EmuHawk/RetroAchievements/RAIntegration.cs @@ -45,7 +45,11 @@ namespace BizHawk.Client.EmuHawk // Memory may be accessed by another thread (mainly rich presence, some other things too) // and peeks for us are not thread safe, so we need to guard it - private readonly RAMemGuard _memGuard = new(); + private readonly ManualResetEventSlim _memLock = new(false); + private readonly SemaphoreSlim _memSema = new(1); + private readonly object _memSync = new(); + private readonly RAMemGuard _memGuard; + private readonly RAMemAccess _memAccess; private bool _firstRestart = true; @@ -110,6 +114,9 @@ namespace BizHawk.Client.EmuHawk Func getConfig, ToolStripItemCollection raDropDownItems, Action shutdownRACallback) : base(mainForm, inputManager, tools, getConfig, raDropDownItems, shutdownRACallback) { + _memGuard = new(_memLock, _memSema, _memSync); + _memAccess = new(_memLock, _memSema, _memSync); + RA.InitClient(_mainForm.Handle, "BizHawk", VersionInfo.GetEmuVersion()); _isActive = () => !Emu.IsNull(); @@ -146,6 +153,7 @@ namespace BizHawk.Client.EmuHawk HandleHardcoreModeDisable("Loading savestates is not allowed in hardcore mode."); } + using var access = _memAccess.EnterExit(); RA.OnLoadState(path); } @@ -228,7 +236,7 @@ namespace BizHawk.Client.EmuHawk public override void Update() { - using var access = _memGuard.GetAccess(); + using var access = _memAccess.EnterExit(); if (RA.HardcoreModeIsActive()) { @@ -260,7 +268,7 @@ namespace BizHawk.Client.EmuHawk public override void OnFrameAdvance() { - using var access = _memGuard.GetAccess(); + using var access = _memAccess.EnterExit(); var input = _inputManager.ControllerOutput; if (input.Definition.BoolButtons.Any(b => (b.Contains("Power") || b.Contains("Reset")) && input.IsPressed(b))) @@ -283,7 +291,7 @@ namespace BizHawk.Client.EmuHawk } // FIXME: THIS IS GARBAGE - public RAMemGuard.AccessWrapper? ThisIsTheRAMemHack() - => _memGuard.GetAccess(); + public IMonitor ThisIsTheRAMemHack() + => _memAccess; } } diff --git a/src/BizHawk.Client.EmuHawk/RetroAchievements/RetroAchievements.Memory.cs b/src/BizHawk.Client.EmuHawk/RetroAchievements/RetroAchievements.Memory.cs index b95298cff6..363b44a2d3 100644 --- a/src/BizHawk.Client.EmuHawk/RetroAchievements/RetroAchievements.Memory.cs +++ b/src/BizHawk.Client.EmuHawk/RetroAchievements/RetroAchievements.Memory.cs @@ -11,56 +11,86 @@ namespace BizHawk.Client.EmuHawk { public abstract partial class RetroAchievements { - public class RAMemGuard : IMonitor, IDisposable + protected class RAMemGuard : IMonitor, IDisposable { - private readonly ManualResetEventSlim MemLock = new(false); - private readonly SemaphoreSlim MemSema = new(1); - private readonly object MemSync = new(); + private readonly ManualResetEventSlim _memLock; + private readonly SemaphoreSlim _memSema; + private readonly object _memSync; + + public RAMemGuard(ManualResetEventSlim memLock, SemaphoreSlim memSema, object memSync) + { + _memLock = memLock; + _memSema = memSema; + _memSync = memSync; + } public void Enter() { - lock (MemSync) + lock (_memSync) { - MemLock.Wait(); - MemSema.Wait(); + _memLock.Wait(); + _memSema.Wait(); } } public void Exit() { - MemSema.Release(); + _memSema.Release(); } public void Dispose() { - MemLock.Dispose(); - MemSema.Dispose(); + _memLock.Dispose(); + _memSema.Dispose(); + } + } + + protected class RAMemAccess : IMonitor + { + private readonly ManualResetEventSlim _memLock; + private readonly SemaphoreSlim _memSema; + private readonly object _memSync; + private int _refCount; + + public RAMemAccess(ManualResetEventSlim memLock, SemaphoreSlim memSema, object memSync) + { + _memLock = memLock; + _memSema = memSema; + _memSync = memSync; + _refCount = 0; } - // can't be a ref struct due to ThisIsTheRAMemHack :( - public readonly struct AccessWrapper : IDisposable + public void Enter() { - private readonly RAMemGuard _guard; - - internal AccessWrapper(RAMemGuard guard) + if (_refCount == 0) { - _guard = guard; - _guard.MemLock.Set(); + _memLock.Set(); } - public void Dispose() + _refCount++; + } + + public void Exit() + { + switch (_refCount) { - lock (_guard.MemSync) + case <= 0: + throw new InvalidOperationException($"Invalid {nameof(_refCount)}"); + case 1: { - _guard.MemLock.Reset(); - _guard.MemSema.Wait(); - _guard.MemSema.Release(); + lock (_memSync) + { + _memLock.Reset(); + _memSema.Wait(); + _memSema.Release(); + } + + break; } } - } - public AccessWrapper GetAccess() - => new(this); + _refCount--; + } } [UnmanagedFunctionPointer(CallingConvention.Cdecl)]