Merge pull request #1673 from Soreepeong/fix/igameconfig-racecon

IGameConfig: fix load-time race condition

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<GameConfigSection> tcsSystem = new();
+    private readonly TaskCompletionSource<GameConfigSection> tcsUiConfig = new();
+    private readonly TaskCompletionSource<GameConfigSection> tcsUiControl = new(); 
+
     private readonly GameConfigAddressResolver address = new();
     private Hook<ConfigChangeDelegate>? configChangeHook;
 
@@ -23,16 +30,32 @@ internal sealed class GameConfig : IServiceType, IGameConfig, IDisposable
     {
         framework.RunOnTick(() =>
         {
-            Log.Verbose("[GameConfig] Initializing");
+            try
-            var csFramework = FFXIVClientStructs.FFXIV.Client.System.Framework.Framework.Instance();
+            {
-            var commonConfig = &csFramework->SystemConfig.CommonSystemConfig;
+                Log.Verbose("[GameConfig] Initializing");
-            this.System = new GameConfigSection("System", framework, &commonConfig->ConfigBase);
+                var csFramework = FFXIVClientStructs.FFXIV.Client.System.Framework.Framework.Instance();
-            this.UiConfig = new GameConfigSection("UiConfig", framework, &commonConfig->UiConfig);
+                var commonConfig = &csFramework->SystemConfig.CommonSystemConfig;
-            this.UiControl = new GameConfigSection("UiControl", framework, () => this.UiConfig.TryGetBool("PadMode", out var padMode) && padMode ? &commonConfig->UiControlGamepadConfig : &commonConfig->UiControlConfig);
+                this.tcsSystem.SetResult(new("System", framework, &commonConfig->ConfigBase));
-        
+                this.tcsUiConfig.SetResult(new("UiConfig", framework, &commonConfig->UiConfig));
-            this.address.Setup(sigScanner);
+                this.tcsUiControl.SetResult(
-            this.configChangeHook = Hook<ConfigChangeDelegate>.FromAddress(this.address.ConfigChangeAddress, this.OnConfigChanged);
+                    new(
-            this.configChangeHook.Enable();
+                        "UiControl",
+                        framework,
+                        () => this.UiConfig.TryGetBool("PadMode", out var padMode) && padMode
+                                  ? &commonConfig->UiControlGamepadConfig
+                                  : &commonConfig->UiControlConfig));
+
+                this.address.Setup(sigScanner);
+                this.configChangeHook = Hook<ConfigChangeDelegate>.FromAddress(
+                    this.address.ConfigChangeAddress,
+                    this.OnConfigChanged);
+                this.configChangeHook.Enable();
+                this.tcsInitialization.SetResult();
+            }
+            catch (Exception ex)
+            {
+                this.tcsInitialization.SetExceptionIfIncomplete(ex);
+            }
         });
     }
 
@@ -58,14 +81,19 @@ internal sealed class GameConfig : IServiceType, IGameConfig, IDisposable
     public event EventHandler<ConfigChangeEvent>? UiControlChanged;
 #pragma warning restore 67
 
-    /// <inheritdoc/>
+    /// <summary>
-    public GameConfigSection System { get; private set; }
+    /// Gets a task representing the initialization state of this class.
+    /// </summary>
+    public Task InitializationTask => this.tcsInitialization.Task;
 
     /// <inheritdoc/>
-    public GameConfigSection UiConfig { get; private set; }
+    public GameConfigSection System => this.tcsSystem.Task.Result;
 
     /// <inheritdoc/>
-    public GameConfigSection UiControl { get; private set; }
+    public GameConfigSection UiConfig => this.tcsUiConfig.Task.Result;
+
+    /// <inheritdoc/>
+    public GameConfigSection UiControl => this.tcsUiControl.Task.Result;
 
     /// <inheritdoc/>
     public bool TryGet(SystemConfigOption option, out bool value) => this.System.TryGet(option.GetName(), out value);
@@ -169,6 +197,11 @@ internal sealed class GameConfig : IServiceType, IGameConfig, IDisposable
     /// <inheritdoc/>
     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();
     }
@@ -220,15 +253,24 @@ internal class GameConfigPluginScoped : IDisposable, IServiceType, IGameConfig
     [ServiceManager.ServiceDependency]
     private readonly GameConfig gameConfigService = Service<GameConfig>.Get();
 
+    private readonly Task initializationTask;
+
     /// <summary>
     /// Initializes a new instance of the <see cref="GameConfigPluginScoped"/> class.
     /// </summary>
     internal GameConfigPluginScoped()
     {
         this.gameConfigService.Changed += this.ConfigChangedForward;
-        this.gameConfigService.System.Changed += this.SystemConfigChangedForward;
+        this.initializationTask = this.gameConfigService.InitializationTask.ContinueWith(
-        this.gameConfigService.UiConfig.Changed += this.UiConfigConfigChangedForward;
+            r =>
-        this.gameConfigService.UiControl.Changed += this.UiControlConfigChangedForward;
+            {
+                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();
     }
     
     /// <inheritdoc/>
@@ -256,9 +298,15 @@ internal class GameConfigPluginScoped : IDisposable, IServiceType, IGameConfig
     public void Dispose()
     {
         this.gameConfigService.Changed -= this.ConfigChangedForward;
-        this.gameConfigService.System.Changed -= this.SystemConfigChangedForward;
+        this.initializationTask.ContinueWith(
-        this.gameConfigService.UiConfig.Changed -= this.UiConfigConfigChangedForward;
+            r =>
-        this.gameConfigService.UiControl.Changed -= this.UiControlConfigChangedForward;
+            {
+                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;

Dalamud/Plugin/Services/IGameConfig.cs

@@ -1,14 +1,23 @@
-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;
 
 /// <summary>
 /// This class represents the game's configuration.
 /// </summary>
+/// <remarks>
+/// Accessing <see cref="GameConfigSection"/>-typed properties such as <see cref="System"/>, directly or indirectly
+/// via <see cref="TryGet(Game.Config.SystemConfigOption,out bool)"/>,
+/// <see cref="Set(Game.Config.SystemConfigOption,bool)"/>, or alike will block, if the game is not done loading.<br />
+/// Therefore, avoid accessing configuration from your plugin constructor, especially if your plugin sets
+/// <see cref="PluginManifest.LoadRequiredState"/> to <c>2</c> and <see cref="PluginManifest.LoadSync"/> to <c>true</c>.
+/// If property access from the plugin constructor is desired, do the value retrieval asynchronously via
+/// <see cref="IFramework.RunOnFrameworkThread{T}(Func{T})"/>; do not wait for the result right away.
+/// </remarks>
 public interface IGameConfig
 {
     /// <summary>

Dalamud/Storage/Assets/DalamudAssetManager.cs

@@ -194,12 +194,14 @@ internal sealed class DalamudAssetManager : IServiceType, IDisposable, IDalamudA
 
                         try
                         {
-                            await using var tempPathStream = File.Open(tempPath, FileMode.Create, FileAccess.Write);
+                            await using (var tempPathStream = File.Open(tempPath, FileMode.Create, FileAccess.Write))
-                            await url.DownloadAsync(
+                            {
-                                this.httpClient.SharedHttpClient,
+                                await url.DownloadAsync(
-                                tempPathStream,
+                                    this.httpClient.SharedHttpClient,
-                                this.cancellationTokenSource.Token);
+                                    tempPathStream,
-                            tempPathStream.Dispose();
+                                    this.cancellationTokenSource.Token);
+                            }
+
                             for (var j = RenameAttemptCount; ; j--)
                             {
                                 try
@@ -265,7 +267,7 @@ internal sealed class DalamudAssetManager : IServiceType, IDisposable, IDalamudA
     /// <inheritdoc/>
     [Pure]
     public IDalamudTextureWrap GetDalamudTextureWrap(DalamudAsset asset) =>
-        ExtractResult(this.GetDalamudTextureWrapAsync(asset));
+        this.GetDalamudTextureWrapAsync(asset).Result;
 
     /// <inheritdoc/>
     [Pure]
@@ -332,8 +334,6 @@ internal sealed class DalamudAssetManager : IServiceType, IDisposable, IDalamudA
         }
     }
 
-    private static T ExtractResult<T>(Task<T> t) => t.IsCompleted ? t.Result : t.GetAwaiter().GetResult();
-
     private Task<TOut> TransformImmediate<TIn, TOut>(Task<TIn> task, Func<TIn, TOut> transformer)
     {
         if (task.IsCompletedSuccessfully)

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);
     }
 
+    /// <summary>
+    /// Calls <see cref="TaskCompletionSource.SetException(System.Exception)"/> if the task is incomplete.
+    /// </summary>
+    /// <param name="t">The task.</param>
+    /// <param name="ex">The exception to set.</param>
+    internal static void SetExceptionIfIncomplete(this TaskCompletionSource t, Exception ex)
+    {
+        if (t.Task.IsCompleted)
+            return;
+        try
+        {
+            t.SetException(ex);
+        }
+        catch
+        {
+            // ignore
+        }
+    }
+
+    /// <summary>
+    /// Calls <see cref="TaskCompletionSource.SetException(System.Exception)"/> if the task is incomplete.
+    /// </summary>
+    /// <typeparam name="T">The type of the result.</typeparam>
+    /// <param name="t">The task.</param>
+    /// <param name="ex">The exception to set.</param>
+    internal static void SetExceptionIfIncomplete<T>(this TaskCompletionSource<T> t, Exception ex)
+    {
+        if (t.Task.IsCompleted)
+            return;
+        try
+        {
+            t.SetException(ex);
+        }
+        catch
+        {
+            // ignore
+        }
+    }
+
     /// <summary>
     /// Print formatted GameObject Information to ImGui.
     /// </summary>