ToxicMushroom/Dalamud
1
0
Fork 0
mirror of https://github.com/goatcorp/Dalamud.git synced 2025-12-12 18:27:23 +01:00
Code Issues Projects Releases Packages Wiki Activity Actions 3

IGameConfig: fix load-time race condition

Browse source 
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.
This commit is contained in:
Soreepeong 2024-02-20 15:37:54 +09:00
parent ac59f73b59
commit c27422384f
3 changed files with 126 additions and 23 deletions
Download patch file Download diff file Expand all files Collapse all files

87
Dalamud/Game/Config/GameConfig.cs
View file

@ -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");
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<ConfigChangeDelegate>.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<ConfigChangeDelegate>.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
/// <inheritdoc/>
public GameConfigSection System { get; private set; }
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 +195,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();
}
@ -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();
}
/// <inheritdoc/>
@ -243,6 +281,9 @@ internal class GameConfigPluginScoped : IDisposable, IServiceType, IGameConfig
/// <inheritdoc/>
public event EventHandler<ConfigChangeEvent>? UiControlChanged;
/// <inheritdoc/>
public Task InitializationTask { get; }
/// <inheritdoc/>
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;

22
Dalamud/Plugin/Services/IGameConfig.cs
View file

@ -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;
/// <summary>
/// This class represents the game's configuration.
/// </summary>
/// <remarks>
/// 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>
@ -31,6 +37,16 @@ public interface IGameConfig
/// </summary>
public event EventHandler<ConfigChangeEvent> UiControlChanged;
/// <summary>
/// Gets a task representing the initialization state of this instance of <see cref="IGameConfig"/>.
/// </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 this task is incomplete.
/// </remarks>
public Task InitializationTask { get; }
/// <summary>
/// Gets the collection of config options that persist between characters.
/// </summary>

40
Dalamud/Utility/Util.cs
View file

@ -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>