From c27422384f66868991814a0fd905c3bada90c271 Mon Sep 17 00:00:00 2001 From: Soreepeong Date: Tue, 20 Feb 2024 15:37:54 +0900 Subject: [PATCH] IGameConfig: fix load-time race condition As some public properties of `IGameConfig` are being set on the first `Framework` tick, there was a short window that those properties were null, which goes against the interface declaration. This commit fixes that, by making those properties block for the full initialization of the class. A possible side effect is that a plugin that is set to block the game from loading until it loads will now hang the game if it tries to access the game configuration from its constructor, instead of throwing a `NullReferenceException`. As it would mean that the plugin was buggy at the first place and it would have sometimes failed to load anyway, it might as well be a non-breaking change. --- Dalamud/Game/Config/GameConfig.cs | 87 ++++++++++++++++++++------ Dalamud/Plugin/Services/IGameConfig.cs | 22 ++++++- Dalamud/Utility/Util.cs | 40 ++++++++++++ 3 files changed, 126 insertions(+), 23 deletions(-) diff --git a/Dalamud/Game/Config/GameConfig.cs b/Dalamud/Game/Config/GameConfig.cs index b82d64f24..94a92a4da 100644 --- a/Dalamud/Game/Config/GameConfig.cs +++ b/Dalamud/Game/Config/GameConfig.cs @@ -1,4 +1,6 @@ -using Dalamud.Hooking; +using System.Threading.Tasks; + +using Dalamud.Hooking; using Dalamud.IoC; using Dalamud.IoC.Internal; using Dalamud.Plugin.Services; @@ -15,6 +17,11 @@ namespace Dalamud.Game.Config; [ServiceManager.BlockingEarlyLoadedService] internal sealed class GameConfig : IServiceType, IGameConfig, IDisposable { + private readonly TaskCompletionSource tcsInitialization = new(); + private readonly TaskCompletionSource tcsSystem = new(); + private readonly TaskCompletionSource tcsUiConfig = new(); + private readonly TaskCompletionSource tcsUiControl = new(); + private readonly GameConfigAddressResolver address = new(); private Hook? configChangeHook; @@ -23,16 +30,32 @@ internal sealed class GameConfig : IServiceType, IGameConfig, IDisposable { framework.RunOnTick(() => { - Log.Verbose("[GameConfig] Initializing"); - var csFramework = FFXIVClientStructs.FFXIV.Client.System.Framework.Framework.Instance(); - var commonConfig = &csFramework->SystemConfig.CommonSystemConfig; - this.System = new GameConfigSection("System", framework, &commonConfig->ConfigBase); - this.UiConfig = new GameConfigSection("UiConfig", framework, &commonConfig->UiConfig); - this.UiControl = new GameConfigSection("UiControl", framework, () => this.UiConfig.TryGetBool("PadMode", out var padMode) && padMode ? &commonConfig->UiControlGamepadConfig : &commonConfig->UiControlConfig); - - this.address.Setup(sigScanner); - this.configChangeHook = Hook.FromAddress(this.address.ConfigChangeAddress, this.OnConfigChanged); - this.configChangeHook.Enable(); + try + { + Log.Verbose("[GameConfig] Initializing"); + var csFramework = FFXIVClientStructs.FFXIV.Client.System.Framework.Framework.Instance(); + var commonConfig = &csFramework->SystemConfig.CommonSystemConfig; + this.tcsSystem.SetResult(new("System", framework, &commonConfig->ConfigBase)); + this.tcsUiConfig.SetResult(new("UiConfig", framework, &commonConfig->UiConfig)); + this.tcsUiControl.SetResult( + new( + "UiControl", + framework, + () => this.UiConfig.TryGetBool("PadMode", out var padMode) && padMode + ? &commonConfig->UiControlGamepadConfig + : &commonConfig->UiControlConfig)); + + this.address.Setup(sigScanner); + this.configChangeHook = Hook.FromAddress( + this.address.ConfigChangeAddress, + this.OnConfigChanged); + this.configChangeHook.Enable(); + this.tcsInitialization.SetResult(); + } + catch (Exception ex) + { + this.tcsInitialization.SetExceptionIfIncomplete(ex); + } }); } @@ -59,13 +82,16 @@ internal sealed class GameConfig : IServiceType, IGameConfig, IDisposable #pragma warning restore 67 /// - public GameConfigSection System { get; private set; } + public Task InitializationTask => this.tcsInitialization.Task; /// - public GameConfigSection UiConfig { get; private set; } + public GameConfigSection System => this.tcsSystem.Task.Result; /// - public GameConfigSection UiControl { get; private set; } + public GameConfigSection UiConfig => this.tcsUiConfig.Task.Result; + + /// + public GameConfigSection UiControl => this.tcsUiControl.Task.Result; /// public bool TryGet(SystemConfigOption option, out bool value) => this.System.TryGet(option.GetName(), out value); @@ -169,6 +195,11 @@ internal sealed class GameConfig : IServiceType, IGameConfig, IDisposable /// void IDisposable.Dispose() { + var ode = new ObjectDisposedException(nameof(GameConfig)); + this.tcsInitialization.SetExceptionIfIncomplete(ode); + this.tcsSystem.SetExceptionIfIncomplete(ode); + this.tcsUiConfig.SetExceptionIfIncomplete(ode); + this.tcsUiControl.SetExceptionIfIncomplete(ode); this.configChangeHook?.Disable(); this.configChangeHook?.Dispose(); } @@ -226,9 +257,16 @@ internal class GameConfigPluginScoped : IDisposable, IServiceType, IGameConfig internal GameConfigPluginScoped() { this.gameConfigService.Changed += this.ConfigChangedForward; - this.gameConfigService.System.Changed += this.SystemConfigChangedForward; - this.gameConfigService.UiConfig.Changed += this.UiConfigConfigChangedForward; - this.gameConfigService.UiControl.Changed += this.UiControlConfigChangedForward; + this.InitializationTask = this.gameConfigService.InitializationTask.ContinueWith( + r => + { + if (!r.IsCompletedSuccessfully) + return r; + this.gameConfigService.System.Changed += this.SystemConfigChangedForward; + this.gameConfigService.UiConfig.Changed += this.UiConfigConfigChangedForward; + this.gameConfigService.UiControl.Changed += this.UiControlConfigChangedForward; + return Task.CompletedTask; + }).Unwrap(); } /// @@ -243,6 +281,9 @@ internal class GameConfigPluginScoped : IDisposable, IServiceType, IGameConfig /// public event EventHandler? UiControlChanged; + /// + public Task InitializationTask { get; } + /// public GameConfigSection System => this.gameConfigService.System; @@ -256,9 +297,15 @@ internal class GameConfigPluginScoped : IDisposable, IServiceType, IGameConfig public void Dispose() { this.gameConfigService.Changed -= this.ConfigChangedForward; - this.gameConfigService.System.Changed -= this.SystemConfigChangedForward; - this.gameConfigService.UiConfig.Changed -= this.UiConfigConfigChangedForward; - this.gameConfigService.UiControl.Changed -= this.UiControlConfigChangedForward; + this.InitializationTask.ContinueWith( + r => + { + if (!r.IsCompletedSuccessfully) + return; + this.gameConfigService.System.Changed -= this.SystemConfigChangedForward; + this.gameConfigService.UiConfig.Changed -= this.UiConfigConfigChangedForward; + this.gameConfigService.UiControl.Changed -= this.UiControlConfigChangedForward; + }); this.Changed = null; this.SystemChanged = null; diff --git a/Dalamud/Plugin/Services/IGameConfig.cs b/Dalamud/Plugin/Services/IGameConfig.cs index 8e9b48d83..8249aed76 100644 --- a/Dalamud/Plugin/Services/IGameConfig.cs +++ b/Dalamud/Plugin/Services/IGameConfig.cs @@ -1,14 +1,20 @@ -using System; -using System.Diagnostics; +using System.Diagnostics; +using System.Threading.Tasks; using Dalamud.Game.Config; -using FFXIVClientStructs.FFXIV.Common.Configuration; +using Dalamud.Plugin.Internal.Types; namespace Dalamud.Plugin.Services; /// /// This class represents the game's configuration. /// +/// +/// Avoid accessing configuration from your plugin constructor, especially if your plugin sets +/// to 2 and to true. +/// If property access from the plugin constructor is desired, do the value retrieval asynchronously via +/// ; do not wait for the result right away. +/// public interface IGameConfig { /// @@ -31,6 +37,16 @@ public interface IGameConfig /// public event EventHandler UiControlChanged; + /// + /// Gets a task representing the initialization state of this instance of . + /// + /// + /// Accessing -typed properties such as , directly or indirectly + /// via , + /// , or alike will block, if this task is incomplete. + /// + public Task InitializationTask { get; } + /// /// Gets the collection of config options that persist between characters. /// diff --git a/Dalamud/Utility/Util.cs b/Dalamud/Utility/Util.cs index f5ad8b999..65196b3ee 100644 --- a/Dalamud/Utility/Util.cs +++ b/Dalamud/Utility/Util.cs @@ -10,6 +10,7 @@ using System.Reflection.Emit; using System.Runtime.CompilerServices; using System.Runtime.InteropServices; using System.Text; +using System.Threading.Tasks; using Dalamud.Configuration.Internal; using Dalamud.Data; @@ -697,6 +698,45 @@ public static class Util Marshal.ThrowExceptionForHR(hr.Value); } + /// + /// Calls if the task is incomplete. + /// + /// The task. + /// The exception to set. + internal static void SetExceptionIfIncomplete(this TaskCompletionSource t, Exception ex) + { + if (t.Task.IsCompleted) + return; + try + { + t.SetException(ex); + } + catch + { + // ignore + } + } + + /// + /// Calls if the task is incomplete. + /// + /// The type of the result. + /// The task. + /// The exception to set. + internal static void SetExceptionIfIncomplete(this TaskCompletionSource t, Exception ex) + { + if (t.Task.IsCompleted) + return; + try + { + t.SetException(ex); + } + catch + { + // ignore + } + } + /// /// Print formatted GameObject Information to ImGui. ///