Fix DataShare race condition, and add debug features (#1573)

This commit is contained in:
srkizer 2023-12-17 05:05:13 +09:00 committed by GitHub
parent df1cdff1a5
commit 5998fc687f
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
7 changed files with 545 additions and 127 deletions

View file

@ -1,4 +1,5 @@
using System.Collections.Generic;
using System.Collections.Immutable;
namespace Dalamud.Plugin.Ipc.Internal;
@ -10,11 +11,28 @@ internal class CallGate : IServiceType
{
private readonly Dictionary<string, CallGateChannel> gates = new();
private ImmutableDictionary<string, CallGateChannel>? gatesCopy;
[ServiceManager.ServiceConstructor]
private CallGate()
{
}
/// <summary>
/// Gets the thread-safe view of the registered gates.
/// </summary>
public IReadOnlyDictionary<string, CallGateChannel> Gates
{
get
{
var copy = this.gatesCopy;
if (copy is not null)
return copy;
lock (this.gates)
return this.gatesCopy ??= this.gates.ToImmutableDictionary(x => x.Key, x => x.Value);
}
}
/// <summary>
/// Gets the provider associated with the specified name.
/// </summary>
@ -22,8 +40,34 @@ internal class CallGate : IServiceType
/// <returns>A CallGate registered under the given name.</returns>
public CallGateChannel GetOrCreateChannel(string name)
{
if (!this.gates.TryGetValue(name, out var gate))
gate = this.gates[name] = new CallGateChannel(name);
return gate;
lock (this.gates)
{
if (!this.gates.TryGetValue(name, out var gate))
{
gate = this.gates[name] = new(name);
this.gatesCopy = null;
}
return gate;
}
}
/// <summary>
/// Remove empty gates from <see cref="Gates"/>.
/// </summary>
public void PurgeEmptyGates()
{
lock (this.gates)
{
var changed = false;
foreach (var (k, v) in this.Gates)
{
if (v.IsEmpty)
changed |= this.gates.Remove(k);
}
if (changed)
this.gatesCopy = null;
}
}
}

View file

@ -1,5 +1,5 @@
using System;
using System.Collections.Generic;
using System.Collections.Immutable;
using System.Linq;
using System.Reflection;
@ -14,6 +14,17 @@ namespace Dalamud.Plugin.Ipc.Internal;
/// </summary>
internal class CallGateChannel
{
/// <summary>
/// The actual storage.
/// </summary>
private readonly HashSet<Delegate> subscriptions = new();
/// <summary>
/// A copy of the actual storage, that will be cleared and populated depending on changes made to
/// <see cref="subscriptions"/>.
/// </summary>
private ImmutableList<Delegate>? subscriptionsCopy;
/// <summary>
/// Initializes a new instance of the <see cref="CallGateChannel"/> class.
/// </summary>
@ -31,17 +42,52 @@ internal class CallGateChannel
/// <summary>
/// Gets a list of delegate subscriptions for when SendMessage is called.
/// </summary>
public List<Delegate> Subscriptions { get; } = new();
public IReadOnlyList<Delegate> Subscriptions
{
get
{
var copy = this.subscriptionsCopy;
if (copy is not null)
return copy;
lock (this.subscriptions)
return this.subscriptionsCopy ??= this.subscriptions.ToImmutableList();
}
}
/// <summary>
/// Gets or sets an action for when InvokeAction is called.
/// </summary>
public Delegate Action { get; set; }
public Delegate? Action { get; set; }
/// <summary>
/// Gets or sets a func for when InvokeFunc is called.
/// </summary>
public Delegate Func { get; set; }
public Delegate? Func { get; set; }
/// <summary>
/// Gets a value indicating whether this <see cref="CallGateChannel"/> is not being used.
/// </summary>
public bool IsEmpty => this.Action is null && this.Func is null && this.Subscriptions.Count == 0;
/// <inheritdoc cref="CallGatePubSubBase.Subscribe"/>
internal void Subscribe(Delegate action)
{
lock (this.subscriptions)
{
this.subscriptionsCopy = null;
this.subscriptions.Add(action);
}
}
/// <inheritdoc cref="CallGatePubSubBase.Unsubscribe"/>
internal void Unsubscribe(Delegate action)
{
lock (this.subscriptions)
{
this.subscriptionsCopy = null;
this.subscriptions.Remove(action);
}
}
/// <summary>
/// Invoke all actions that have subscribed to this IPC.
@ -49,9 +95,6 @@ internal class CallGateChannel
/// <param name="args">Message arguments.</param>
internal void SendMessage(object?[]? args)
{
if (this.Subscriptions.Count == 0)
return;
foreach (var subscription in this.Subscriptions)
{
var methodInfo = subscription.GetMethodInfo();
@ -105,7 +148,14 @@ internal class CallGateChannel
var paramTypes = methodInfo.GetParameters()
.Select(pi => pi.ParameterType).ToArray();
if (args?.Length != paramTypes.Length)
if (args is null)
{
if (paramTypes.Length == 0)
return;
throw new IpcLengthMismatchError(this.Name, 0, paramTypes.Length);
}
if (args.Length != paramTypes.Length)
throw new IpcLengthMismatchError(this.Name, args.Length, paramTypes.Length);
for (var i = 0; i < args.Length; i++)
@ -137,7 +187,7 @@ internal class CallGateChannel
}
}
private IEnumerable<Type> GenerateTypes(Type type)
private IEnumerable<Type> GenerateTypes(Type? type)
{
while (type != null && type != typeof(object))
{
@ -148,6 +198,9 @@ internal class CallGateChannel
private object? ConvertObject(object? obj, Type type)
{
if (obj is null)
return null;
var json = JsonConvert.SerializeObject(obj);
try

View file

@ -1,5 +1,3 @@
using System;
#pragma warning disable SA1402 // File may only contain a single type
namespace Dalamud.Plugin.Ipc.Internal;
@ -37,7 +35,7 @@ internal class CallGatePubSub<TRet> : CallGatePubSubBase, ICallGateProvider<TRet
public void InvokeAction()
=> base.InvokeAction();
/// <inheritdoc cref="CallGatePubSubBase.InvokeFunc"/>
/// <inheritdoc cref="CallGatePubSubBase.InvokeFunc{TRet}"/>
public TRet InvokeFunc()
=> this.InvokeFunc<TRet>();
}
@ -75,7 +73,7 @@ internal class CallGatePubSub<T1, TRet> : CallGatePubSubBase, ICallGateProvider<
public void InvokeAction(T1 arg1)
=> base.InvokeAction(arg1);
/// <inheritdoc cref="CallGatePubSubBase.InvokeFunc"/>
/// <inheritdoc cref="CallGatePubSubBase.InvokeFunc{TRet}"/>
public TRet InvokeFunc(T1 arg1)
=> this.InvokeFunc<TRet>(arg1);
}
@ -113,7 +111,7 @@ internal class CallGatePubSub<T1, T2, TRet> : CallGatePubSubBase, ICallGateProvi
public void InvokeAction(T1 arg1, T2 arg2)
=> base.InvokeAction(arg1, arg2);
/// <inheritdoc cref="CallGatePubSubBase.InvokeFunc"/>
/// <inheritdoc cref="CallGatePubSubBase.InvokeFunc{TRet}"/>
public TRet InvokeFunc(T1 arg1, T2 arg2)
=> this.InvokeFunc<TRet>(arg1, arg2);
}
@ -151,7 +149,7 @@ internal class CallGatePubSub<T1, T2, T3, TRet> : CallGatePubSubBase, ICallGateP
public void InvokeAction(T1 arg1, T2 arg2, T3 arg3)
=> base.InvokeAction(arg1, arg2, arg3);
/// <inheritdoc cref="CallGatePubSubBase.InvokeFunc"/>
/// <inheritdoc cref="CallGatePubSubBase.InvokeFunc{TRet}"/>
public TRet InvokeFunc(T1 arg1, T2 arg2, T3 arg3)
=> this.InvokeFunc<TRet>(arg1, arg2, arg3);
}
@ -189,7 +187,7 @@ internal class CallGatePubSub<T1, T2, T3, T4, TRet> : CallGatePubSubBase, ICallG
public void InvokeAction(T1 arg1, T2 arg2, T3 arg3, T4 arg4)
=> base.InvokeAction(arg1, arg2, arg3, arg4);
/// <inheritdoc cref="CallGatePubSubBase.InvokeFunc"/>
/// <inheritdoc cref="CallGatePubSubBase.InvokeFunc{TRet}"/>
public TRet InvokeFunc(T1 arg1, T2 arg2, T3 arg3, T4 arg4)
=> this.InvokeFunc<TRet>(arg1, arg2, arg3, arg4);
}
@ -227,7 +225,7 @@ internal class CallGatePubSub<T1, T2, T3, T4, T5, TRet> : CallGatePubSubBase, IC
public void InvokeAction(T1 arg1, T2 arg2, T3 arg3, T4 arg4, T5 arg5)
=> base.InvokeAction(arg1, arg2, arg3, arg4, arg5);
/// <inheritdoc cref="CallGatePubSubBase.InvokeFunc"/>
/// <inheritdoc cref="CallGatePubSubBase.InvokeFunc{TRet}"/>
public TRet InvokeFunc(T1 arg1, T2 arg2, T3 arg3, T4 arg4, T5 arg5)
=> this.InvokeFunc<TRet>(arg1, arg2, arg3, arg4, arg5);
}
@ -265,7 +263,7 @@ internal class CallGatePubSub<T1, T2, T3, T4, T5, T6, TRet> : CallGatePubSubBase
public void InvokeAction(T1 arg1, T2 arg2, T3 arg3, T4 arg4, T5 arg5, T6 arg6)
=> base.InvokeAction(arg1, arg2, arg3, arg4, arg5, arg6);
/// <inheritdoc cref="CallGatePubSubBase.InvokeFunc"/>
/// <inheritdoc cref="CallGatePubSubBase.InvokeFunc{TRet}"/>
public TRet InvokeFunc(T1 arg1, T2 arg2, T3 arg3, T4 arg4, T5 arg5, T6 arg6)
=> this.InvokeFunc<TRet>(arg1, arg2, arg3, arg4, arg5, arg6);
}
@ -303,7 +301,7 @@ internal class CallGatePubSub<T1, T2, T3, T4, T5, T6, T7, TRet> : CallGatePubSub
public void InvokeAction(T1 arg1, T2 arg2, T3 arg3, T4 arg4, T5 arg5, T6 arg6, T7 arg7)
=> base.InvokeAction(arg1, arg2, arg3, arg4, arg5, arg6, arg7);
/// <inheritdoc cref="CallGatePubSubBase.InvokeFunc"/>
/// <inheritdoc cref="CallGatePubSubBase.InvokeFunc{TRet}"/>
public TRet InvokeFunc(T1 arg1, T2 arg2, T3 arg3, T4 arg4, T5 arg5, T6 arg6, T7 arg7)
=> this.InvokeFunc<TRet>(arg1, arg2, arg3, arg4, arg5, arg6, arg7);
}
@ -341,7 +339,7 @@ internal class CallGatePubSub<T1, T2, T3, T4, T5, T6, T7, T8, TRet> : CallGatePu
public void InvokeAction(T1 arg1, T2 arg2, T3 arg3, T4 arg4, T5 arg5, T6 arg6, T7 arg7, T8 arg8)
=> base.InvokeAction(arg1, arg2, arg3, arg4, arg5, arg6, arg7, arg8);
/// <inheritdoc cref="CallGatePubSubBase.InvokeFunc"/>
/// <inheritdoc cref="CallGatePubSubBase.InvokeFunc{TRet}"/>
public TRet InvokeFunc(T1 arg1, T2 arg2, T3 arg3, T4 arg4, T5 arg5, T6 arg6, T7 arg7, T8 arg8)
=> this.InvokeFunc<TRet>(arg1, arg2, arg3, arg4, arg5, arg6, arg7, arg8);
}

View file

@ -1,5 +1,3 @@
using System;
using Dalamud.Plugin.Ipc.Exceptions;
namespace Dalamud.Plugin.Ipc.Internal;
@ -13,7 +11,7 @@ internal abstract class CallGatePubSubBase
/// Initializes a new instance of the <see cref="CallGatePubSubBase"/> class.
/// </summary>
/// <param name="name">The name of the IPC registration.</param>
public CallGatePubSubBase(string name)
protected CallGatePubSubBase(string name)
{
this.Channel = Service<CallGate>.Get().GetOrCreateChannel(name);
}
@ -54,14 +52,14 @@ internal abstract class CallGatePubSubBase
/// </summary>
/// <param name="action">Action to subscribe.</param>
private protected void Subscribe(Delegate action)
=> this.Channel.Subscriptions.Add(action);
=> this.Channel.Subscribe(action);
/// <summary>
/// Unsubscribe an expression from this registration.
/// </summary>
/// <param name="action">Action to unsubscribe.</param>
private protected void Unsubscribe(Delegate action)
=> this.Channel.Subscriptions.Remove(action);
=> this.Channel.Unsubscribe(action);
/// <summary>
/// Invoke an action registered for inter-plugin communication.

View file

@ -1,5 +1,10 @@
using System;
using System.Collections.Generic;
using System.Diagnostics.CodeAnalysis;
using System.Runtime.ExceptionServices;
using Dalamud.Plugin.Ipc.Exceptions;
using Serilog;
namespace Dalamud.Plugin.Ipc.Internal;
@ -8,10 +13,14 @@ namespace Dalamud.Plugin.Ipc.Internal;
/// </summary>
internal readonly struct DataCache
{
/// <summary> Name of the data. </summary>
internal readonly string Tag;
/// <summary> The assembly name of the initial creator. </summary>
internal readonly string CreatorAssemblyName;
/// <summary> A not-necessarily distinct list of current users. </summary>
/// <remarks> Also used as a reference count tracker. </remarks>
internal readonly List<string> UserAssemblyNames;
/// <summary> The type the data was registered as. </summary>
@ -23,14 +32,83 @@ internal readonly struct DataCache
/// <summary>
/// Initializes a new instance of the <see cref="DataCache"/> struct.
/// </summary>
/// <param name="tag">Name of the data.</param>
/// <param name="creatorAssemblyName">The assembly name of the initial creator.</param>
/// <param name="data">A reference to data.</param>
/// <param name="type">The type of the data.</param>
public DataCache(string creatorAssemblyName, object? data, Type type)
public DataCache(string tag, string creatorAssemblyName, object? data, Type type)
{
this.Tag = tag;
this.CreatorAssemblyName = creatorAssemblyName;
this.UserAssemblyNames = new List<string> { creatorAssemblyName };
this.UserAssemblyNames = new();
this.Data = data;
this.Type = type;
}
/// <summary>
/// Creates a new instance of the <see cref="DataCache"/> struct, using the given data generator function.
/// </summary>
/// <param name="tag">The name for the data cache.</param>
/// <param name="creatorAssemblyName">The assembly name of the initial creator.</param>
/// <param name="dataGenerator">The function that generates the data if it does not already exist.</param>
/// <typeparam name="T">The type of the stored data - needs to be a reference type that is shared through Dalamud itself, not loaded by the plugin.</typeparam>
/// <returns>The new instance of <see cref="DataCache"/>.</returns>
public static DataCache From<T>(string tag, string creatorAssemblyName, Func<T> dataGenerator)
where T : class
{
try
{
var result = new DataCache(tag, creatorAssemblyName, dataGenerator.Invoke(), typeof(T));
Log.Verbose(
"[{who}] Created new data for [{Tag:l}] for creator {Creator:l}.",
nameof(DataShare),
tag,
creatorAssemblyName);
return result;
}
catch (Exception e)
{
throw ExceptionDispatchInfo.SetCurrentStackTrace(
new DataCacheCreationError(tag, creatorAssemblyName, typeof(T), e));
}
}
/// <summary>
/// Attempts to fetch the data.
/// </summary>
/// <param name="callerName">The name of the caller assembly.</param>
/// <param name="value">The value, if succeeded.</param>
/// <param name="ex">The exception, if failed.</param>
/// <typeparam name="T">Desired type of the data.</typeparam>
/// <returns><c>true</c> on success.</returns>
public bool TryGetData<T>(
string callerName,
[NotNullWhen(true)] out T? value,
[NotNullWhen(false)] out Exception? ex)
where T : class
{
switch (this.Data)
{
case null:
value = null;
ex = ExceptionDispatchInfo.SetCurrentStackTrace(new DataCacheValueNullError(this.Tag, this.Type));
return false;
case T data:
value = data;
ex = null;
// Register the access history
lock (this.UserAssemblyNames)
this.UserAssemblyNames.Add(callerName);
return true;
default:
value = null;
ex = ExceptionDispatchInfo.SetCurrentStackTrace(
new DataCacheTypeMismatchError(this.Tag, this.CreatorAssemblyName, typeof(T), this.Type));
return false;
}
}
}

View file

@ -1,9 +1,7 @@
using System;
using System.Collections.Generic;
using System.Diagnostics;
using System.Diagnostics.CodeAnalysis;
using System.Linq;
using System.Reflection;
using Dalamud.Plugin.Ipc.Exceptions;
using Serilog;
@ -16,7 +14,11 @@ namespace Dalamud.Plugin.Ipc.Internal;
[ServiceManager.BlockingEarlyLoadedService]
internal class DataShare : IServiceType
{
private readonly Dictionary<string, DataCache> caches = new();
/// <summary>
/// Dictionary of cached values. Note that <see cref="Lazy{T}"/> is being used, as it does its own locking,
/// effectively preventing calling the data generator multiple times concurrently.
/// </summary>
private readonly Dictionary<string, Lazy<DataCache>> caches = new();
[ServiceManager.ServiceConstructor]
private DataShare()
@ -39,38 +41,15 @@ internal class DataShare : IServiceType
where T : class
{
var callerName = GetCallerName();
Lazy<DataCache> cacheLazy;
lock (this.caches)
{
if (this.caches.TryGetValue(tag, out var cache))
{
if (!cache.Type.IsAssignableTo(typeof(T)))
{
throw new DataCacheTypeMismatchError(tag, cache.CreatorAssemblyName, typeof(T), cache.Type);
}
cache.UserAssemblyNames.Add(callerName);
return cache.Data as T ?? throw new DataCacheValueNullError(tag, cache.Type);
}
try
{
var obj = dataGenerator.Invoke();
if (obj == null)
{
throw new Exception("Returned data was null.");
}
cache = new DataCache(callerName, obj, typeof(T));
this.caches[tag] = cache;
Log.Verbose("[DataShare] Created new data for [{Tag:l}] for creator {Creator:l}.", tag, callerName);
return obj;
}
catch (Exception e)
{
throw new DataCacheCreationError(tag, callerName, typeof(T), e);
}
if (!this.caches.TryGetValue(tag, out cacheLazy))
this.caches[tag] = cacheLazy = new(() => DataCache.From(tag, callerName, dataGenerator));
}
return cacheLazy.Value.TryGetData<T>(callerName, out var value, out var ex) ? value : throw ex;
}
/// <summary>
@ -80,34 +59,36 @@ internal class DataShare : IServiceType
/// <param name="tag">The name for the data cache.</param>
public void RelinquishData(string tag)
{
DataCache cache;
lock (this.caches)
{
if (!this.caches.TryGetValue(tag, out var cache))
{
if (!this.caches.TryGetValue(tag, out var cacheLazy))
return;
}
var callerName = GetCallerName();
lock (this.caches)
{
if (!cache.UserAssemblyNames.Remove(callerName) || cache.UserAssemblyNames.Count > 0)
{
return;
}
if (this.caches.Remove(tag))
{
if (cache.Data is IDisposable disposable)
{
disposable.Dispose();
Log.Verbose("[DataShare] Disposed [{Tag:l}] after it was removed from all shares.", tag);
}
else
{
Log.Verbose("[DataShare] Removed [{Tag:l}] from all shares.", tag);
}
}
cache = cacheLazy.Value;
if (!cache.UserAssemblyNames.Remove(callerName) || cache.UserAssemblyNames.Count > 0)
return;
if (!this.caches.Remove(tag))
return;
}
if (cache.Data is IDisposable disposable)
{
try
{
disposable.Dispose();
Log.Verbose("[DataShare] Disposed [{Tag:l}] after it was removed from all shares.", tag);
}
catch (Exception e)
{
Log.Error(e, "[DataShare] Failed to dispose [{Tag:l}] after it was removed from all shares.", tag);
}
}
else
{
Log.Verbose("[DataShare] Removed [{Tag:l}] from all shares.", tag);
}
}
@ -123,23 +104,14 @@ internal class DataShare : IServiceType
where T : class
{
data = null;
Lazy<DataCache> cacheLazy;
lock (this.caches)
{
if (!this.caches.TryGetValue(tag, out var cache) || !cache.Type.IsAssignableTo(typeof(T)))
{
if (!this.caches.TryGetValue(tag, out cacheLazy))
return false;
}
var callerName = GetCallerName();
data = cache.Data as T;
if (data == null)
{
return false;
}
cache.UserAssemblyNames.Add(callerName);
return true;
}
return cacheLazy.Value.TryGetData(GetCallerName(), out data, out _);
}
/// <summary>
@ -155,27 +127,14 @@ internal class DataShare : IServiceType
public T GetData<T>(string tag)
where T : class
{
Lazy<DataCache> cacheLazy;
lock (this.caches)
{
if (!this.caches.TryGetValue(tag, out var cache))
{
if (!this.caches.TryGetValue(tag, out cacheLazy))
throw new KeyNotFoundException($"The data cache [{tag}] is not registered.");
}
var callerName = Assembly.GetCallingAssembly().GetName().Name ?? string.Empty;
if (!cache.Type.IsAssignableTo(typeof(T)))
{
throw new DataCacheTypeMismatchError(tag, callerName, typeof(T), cache.Type);
}
if (cache.Data is not T data)
{
throw new DataCacheValueNullError(tag, typeof(T));
}
cache.UserAssemblyNames.Add(callerName);
return data;
}
return cacheLazy.Value.TryGetData<T>(GetCallerName(), out var value, out var ex) ? value : throw ex;
}
/// <summary>
@ -186,7 +145,8 @@ internal class DataShare : IServiceType
{
lock (this.caches)
{
return this.caches.Select(kvp => (kvp.Key, kvp.Value.CreatorAssemblyName, kvp.Value.UserAssemblyNames.ToArray()));
return this.caches.Select(
kvp => (kvp.Key, kvp.Value.Value.CreatorAssemblyName, kvp.Value.Value.UserAssemblyNames.ToArray()));
}
}