Make DtrBar more threadsafe (#1978)

* Changed DtrBar to use ReaderWriterLockSlim so that there exists only one storage of entries, preventing possible desync.
* DtrBarEntry will now hold a reference to the LocalPlugin that created the entry, so that DtrBarPluginScoped can defer plugin related handling to the main service.
* Marked DtrBarEntry class itself to be turned internal in API 11.
* Made IDtrBar.Entries return an immutable copy of underlying list of DtrBar entries, that will be freshly created whenever the list changes.
This commit is contained in:
srkizer 2024-07-28 21:14:37 +09:00 committed by GitHub
parent a7ab3b9def
commit c25f13261d
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
10 changed files with 385 additions and 207 deletions

View file

@ -1,6 +1,7 @@
using System.Collections.Concurrent;
using System.Collections.Generic;
using System.Collections.Immutable;
using System.Linq;
using System.Threading;
using Dalamud.Configuration.Internal;
using Dalamud.Game.Addon.Events;
@ -10,6 +11,7 @@ using Dalamud.Game.Text.SeStringHandling;
using Dalamud.IoC;
using Dalamud.IoC.Internal;
using Dalamud.Logging.Internal;
using Dalamud.Plugin.Internal.Types;
using Dalamud.Plugin.Services;
using FFXIVClientStructs.FFXIV.Client.Graphics;
@ -48,11 +50,13 @@ internal sealed unsafe class DtrBar : IInternalDisposableService, IDtrBar
private readonly AddonLifecycleEventListener dtrPostRequestedUpdateListener;
private readonly AddonLifecycleEventListener dtrPreFinalizeListener;
private readonly ConcurrentBag<DtrBarEntry> newEntries = new();
private readonly List<DtrBarEntry> entries = new();
private readonly ReaderWriterLockSlim entriesLock = new();
private readonly List<DtrBarEntry> entries = [];
private readonly Dictionary<uint, List<IAddonEventHandle>> eventHandles = new();
private ImmutableList<IReadOnlyDtrBarEntry>? entriesReadOnlyCopy;
private Utf8String* emptyString;
private uint runningNodeIds = BaseNodeId;
@ -71,52 +75,108 @@ internal sealed unsafe class DtrBar : IInternalDisposableService, IDtrBar
this.framework.Update += this.Update;
this.configuration.DtrOrder ??= new List<string>();
this.configuration.DtrIgnore ??= new List<string>();
this.configuration.DtrOrder ??= [];
this.configuration.DtrIgnore ??= [];
this.configuration.QueueSave();
}
/// <summary>
/// Event type fired each time a DtrEntry was removed.
/// </summary>
/// <param name="title">The title of the bar entry.</param>
internal delegate void DtrEntryRemovedDelegate(string title);
/// <summary>
/// Event fired each time a DtrEntry was removed.
/// </summary>
internal event DtrEntryRemovedDelegate? DtrEntryRemoved;
/// <inheritdoc/>
public IReadOnlyList<IReadOnlyDtrBarEntry> Entries => this.entries;
/// <inheritdoc/>
public IDtrBarEntry Get(string title, SeString? text = null)
public IReadOnlyList<IReadOnlyDtrBarEntry> Entries
{
if (this.entries.Any(x => x.Title == title) || this.newEntries.Any(x => x.Title == title))
throw new ArgumentException("An entry with the same title already exists.");
var entry = new DtrBarEntry(this.configuration, title, null);
entry.Text = text;
// Add the entry to the end of the order list, if it's not there already.
if (!this.configuration.DtrOrder!.Contains(title))
this.configuration.DtrOrder!.Add(title);
this.newEntries.Add(entry);
return entry;
}
/// <inheritdoc/>
public void Remove(string title)
{
if (this.entries.FirstOrDefault(entry => entry.Title == title) is { } dtrBarEntry)
get
{
dtrBarEntry.Remove();
var erc = this.entriesReadOnlyCopy;
if (erc is null)
{
this.entriesLock.EnterReadLock();
this.entriesReadOnlyCopy = erc = [..this.entries];
this.entriesLock.ExitReadLock();
}
return erc;
}
}
/// <summary>
/// Get a DTR bar entry.
/// This allows you to add your own text, and users to sort it.
/// </summary>
/// <param name="plugin">Plugin that owns the DTR bar, or <c>null</c> if owned by Dalamud.</param>
/// <param name="title">A user-friendly name for sorting.</param>
/// <param name="text">The text the entry shows.</param>
/// <returns>The entry object used to update, hide and remove the entry.</returns>
/// <exception cref="ArgumentException">Thrown when an entry with the specified title exists.</exception>
public IDtrBarEntry Get(LocalPlugin? plugin, string title, SeString? text = null)
{
this.entriesLock.EnterUpgradeableReadLock();
foreach (var existingEntry in this.entries)
{
if (existingEntry.Title == title)
{
existingEntry.ShouldBeRemoved = false;
this.entriesLock.ExitUpgradeableReadLock();
if (plugin == existingEntry.OwnerPlugin)
return existingEntry;
throw new ArgumentException("An entry with the same title already exists.");
}
}
this.entriesLock.EnterWriteLock();
var entry = new DtrBarEntry(this.configuration, title, null) { Text = text, OwnerPlugin = plugin };
this.entries.Add(entry);
// Add the entry to the end of the order list, if it's not there already.
var dtrOrder = this.configuration.DtrOrder ??= [];
if (!dtrOrder.Contains(entry.Title))
dtrOrder.Add(entry.Title);
this.ApplySortUnsafe(dtrOrder);
this.entriesReadOnlyCopy = null;
this.entriesLock.ExitWriteLock();
this.entriesLock.ExitUpgradeableReadLock();
return entry;
}
/// <inheritdoc/>
public IDtrBarEntry Get(string title, SeString? text = null) => this.Get(null, title, text);
/// <summary>
/// Removes a DTR bar entry from the system.
/// </summary>
/// <param name="plugin">Plugin that owns the DTR bar, or <c>null</c> if owned by Dalamud.</param>
/// <param name="title">Title of the entry to remove, or <c>null</c> to remove all entries under the plugin.</param>
/// <remarks>Remove operation is not immediate. If you try to add right after removing, the operation may fail.
/// </remarks>
public void Remove(LocalPlugin? plugin, string? title)
{
this.entriesLock.EnterUpgradeableReadLock();
foreach (var entry in this.entries)
{
if ((title is null || entry.Title == title) && (plugin is null || entry.OwnerPlugin == plugin))
{
if (!entry.Added)
{
this.entriesLock.EnterWriteLock();
this.RemoveEntry(entry);
this.entries.Remove(entry);
this.entriesReadOnlyCopy = null;
this.entriesLock.ExitWriteLock();
}
entry.Remove();
break;
}
}
this.entriesLock.ExitUpgradeableReadLock();
}
/// <inheritdoc/>
public void Remove(string title) => this.Remove(null, title);
/// <inheritdoc/>
void IInternalDisposableService.DisposeService()
{
@ -124,10 +184,17 @@ internal sealed unsafe class DtrBar : IInternalDisposableService, IDtrBar
this.addonLifecycle.UnregisterListener(this.dtrPostRequestedUpdateListener);
this.addonLifecycle.UnregisterListener(this.dtrPreFinalizeListener);
foreach (var entry in this.entries)
this.RemoveEntry(entry);
this.framework.RunOnFrameworkThread(
() =>
{
this.entriesLock.EnterWriteLock();
foreach (var entry in this.entries)
this.RemoveEntry(entry);
this.entries.Clear();
this.entriesReadOnlyCopy = null;
this.entriesLock.ExitWriteLock();
}).Wait();
this.entries.Clear();
this.framework.Update -= this.Update;
if (this.emptyString != null)
@ -137,23 +204,6 @@ internal sealed unsafe class DtrBar : IInternalDisposableService, IDtrBar
}
}
/// <summary>
/// Remove nodes marked as "should be removed" from the bar.
/// </summary>
internal void HandleRemovedNodes()
{
foreach (var data in this.entries)
{
if (data.ShouldBeRemoved)
{
this.RemoveEntry(data);
this.DtrEntryRemoved?.Invoke(data.Title);
}
}
this.entries.RemoveAll(d => d.ShouldBeRemoved);
}
/// <summary>
/// Remove native resources for the specified entry.
/// </summary>
@ -174,7 +224,17 @@ internal sealed unsafe class DtrBar : IInternalDisposableService, IDtrBar
/// </summary>
/// <param name="title">The title to check for.</param>
/// <returns>Whether or not an entry with that title is registered.</returns>
internal bool HasEntry(string title) => this.entries.Any(x => x.Title == title);
internal bool HasEntry(string title)
{
var found = false;
this.entriesLock.EnterReadLock();
for (var i = 0; i < this.entries.Count && !found; i++)
found = this.entries[i].Title == title;
this.entriesLock.ExitReadLock();
return found;
}
/// <summary>
/// Dirty the DTR bar entry with the specified title.
@ -183,24 +243,37 @@ internal sealed unsafe class DtrBar : IInternalDisposableService, IDtrBar
/// <returns>Whether the entry was found.</returns>
internal bool MakeDirty(string title)
{
var entry = this.entries.FirstOrDefault(x => x.Title == title);
if (entry == null)
return false;
var found = false;
entry.Dirty = true;
return true;
this.entriesLock.EnterReadLock();
for (var i = 0; i < this.entries.Count && !found; i++)
{
found = this.entries[i].Title == title;
if (found)
this.entries[i].Dirty = true;
}
this.entriesLock.ExitReadLock();
return found;
}
/// <summary>
/// Reapply the DTR entry ordering from <see cref="DalamudConfiguration"/>.
/// </summary>
internal void ApplySort()
{
this.entriesLock.EnterWriteLock();
this.ApplySortUnsafe(this.configuration.DtrOrder ??= []);
this.entriesLock.ExitWriteLock();
}
private void ApplySortUnsafe(List<string> dtrOrder)
{
// Sort the current entry list, based on the order in the configuration.
var positions = this.configuration
.DtrOrder!
.Select(entry => (entry, index: this.configuration.DtrOrder!.IndexOf(entry)))
.ToDictionary(x => x.entry, x => x.index);
var positions = dtrOrder
.Select(entry => (entry, index: dtrOrder.IndexOf(entry)))
.ToDictionary(x => x.entry, x => x.index);
this.entries.Sort((x, y) =>
{
@ -208,15 +281,13 @@ internal sealed unsafe class DtrBar : IInternalDisposableService, IDtrBar
var yPos = positions.TryGetValue(y.Title, out var yIndex) ? yIndex : int.MaxValue;
return xPos.CompareTo(yPos);
});
this.entriesReadOnlyCopy = null;
}
private AtkUnitBase* GetDtr() => (AtkUnitBase*)this.gameGui.GetAddonByName("_DTR").ToPointer();
private void Update(IFramework unused)
{
this.HandleRemovedNodes();
this.HandleAddedNodes();
var dtr = this.GetDtr();
if (dtr == null || dtr->RootNode == null || dtr->RootNode->ChildNode == null) return;
@ -236,14 +307,27 @@ internal sealed unsafe class DtrBar : IInternalDisposableService, IDtrBar
var runningXPos = this.entryStartPos;
foreach (var data in this.entries)
this.entriesLock.EnterUpgradeableReadLock();
for (var i = 0; i < this.entries.Count; i++)
{
if (!data.Added)
var data = this.entries[i];
if (data.ShouldBeRemoved)
{
data.Added = this.AddNode(data.TextNode);
data.Dirty = true;
this.entriesLock.EnterWriteLock();
this.entries.RemoveAt(i);
this.RemoveEntry(data);
this.entriesReadOnlyCopy = null;
this.entriesLock.ExitWriteLock();
i--;
continue;
}
if (!data.Added)
data.Added = this.AddNode(data);
if (!data.Added || data.TextNode is null) // TextNode check is unnecessary, but just in case.
continue;
var isHide = !data.Shown || data.UserHidden;
var node = data.TextNode;
var nodeHidden = !node->AtkResNode.IsVisible();
@ -290,23 +374,10 @@ internal sealed unsafe class DtrBar : IInternalDisposableService, IDtrBar
data.Dirty = false;
}
this.entriesLock.ExitUpgradeableReadLock();
}
private void HandleAddedNodes()
{
if (!this.newEntries.IsEmpty)
{
foreach (var newEntry in this.newEntries)
{
newEntry.TextNode = this.MakeNode(++this.runningNodeIds);
this.entries.Add(newEntry);
}
this.newEntries.Clear();
this.ApplySort();
}
}
private void FixCollision(AddonEvent eventType, AddonArgs addonInfo)
{
var addon = (AtkUnitBase*)addonInfo.Addon;
@ -316,7 +387,7 @@ internal sealed unsafe class DtrBar : IInternalDisposableService, IDtrBar
var additionalWidth = 0;
AtkResNode* collisionNode = null;
foreach (var index in Enumerable.Range(0, addon->UldManager.NodeListCount))
for (var index = 0; index < addon->UldManager.NodeListCount; index++)
{
var node = addon->UldManager.NodeList[index];
if (node->IsVisible())
@ -382,22 +453,20 @@ internal sealed unsafe class DtrBar : IInternalDisposableService, IDtrBar
private void RecreateNodes()
{
this.runningNodeIds = BaseNodeId;
if (this.entries.Any())
{
this.eventHandles.Clear();
}
this.entriesLock.EnterReadLock();
this.eventHandles.Clear();
foreach (var entry in this.entries)
{
entry.TextNode = this.MakeNode(++this.runningNodeIds);
entry.Added = false;
}
this.entriesLock.ExitReadLock();
}
private bool AddNode(AtkTextNode* node)
private bool AddNode(DtrBarEntry data)
{
var dtr = this.GetDtr();
if (dtr == null || dtr->RootNode == null || dtr->UldManager.NodeList == null || node == null) return false;
if (dtr == null || dtr->RootNode == null || dtr->UldManager.NodeList == null) return false;
var node = data.TextNode = this.MakeNode(++this.runningNodeIds);
this.eventHandles.TryAdd(node->AtkResNode.NodeId, new List<IAddonEventHandle>());
this.eventHandles[node->AtkResNode.NodeId].AddRange(new List<IAddonEventHandle>
@ -420,6 +489,8 @@ internal sealed unsafe class DtrBar : IInternalDisposableService, IDtrBar
dtr->UldManager.UpdateDrawNodeList();
dtr->UpdateCollisionNodeList(false);
Log.Debug("Updated node draw list");
data.Dirty = true;
return true;
}
@ -497,7 +568,15 @@ internal sealed unsafe class DtrBar : IInternalDisposableService, IDtrBar
var addon = (AtkUnitBase*)atkUnitBase;
var node = (AtkResNode*)atkResNode;
if (this.entries.FirstOrDefault(entry => entry.TextNode == node) is not { } dtrBarEntry) return;
DtrBarEntry? dtrBarEntry = null;
this.entriesLock.EnterReadLock();
foreach (var entry in this.entries)
{
if (entry.TextNode == node)
dtrBarEntry = entry;
}
this.entriesLock.ExitReadLock();
if (dtrBarEntry is { Tooltip: not null })
{
@ -541,58 +620,25 @@ internal sealed unsafe class DtrBar : IInternalDisposableService, IDtrBar
#pragma warning disable SA1015
[ResolveVia<IDtrBar>]
#pragma warning restore SA1015
internal class DtrBarPluginScoped : IInternalDisposableService, IDtrBar
internal sealed class DtrBarPluginScoped : IInternalDisposableService, IDtrBar
{
private readonly LocalPlugin plugin;
[ServiceManager.ServiceDependency]
private readonly DtrBar dtrBarService = Service<DtrBar>.Get();
private readonly Dictionary<string, IDtrBarEntry> pluginEntries = new();
/// <summary>
/// Initializes a new instance of the <see cref="DtrBarPluginScoped"/> class.
/// </summary>
internal DtrBarPluginScoped()
{
this.dtrBarService.DtrEntryRemoved += this.OnDtrEntryRemoved;
}
[ServiceManager.ServiceConstructor]
private DtrBarPluginScoped(LocalPlugin plugin) => this.plugin = plugin;
/// <inheritdoc/>
public IReadOnlyList<IReadOnlyDtrBarEntry> Entries => this.dtrBarService.Entries;
/// <inheritdoc/>
void IInternalDisposableService.DisposeService()
{
this.dtrBarService.DtrEntryRemoved -= this.OnDtrEntryRemoved;
foreach (var entry in this.pluginEntries)
{
entry.Value.Remove();
}
this.pluginEntries.Clear();
}
void IInternalDisposableService.DisposeService() => this.dtrBarService.Remove(this.plugin, null);
/// <inheritdoc/>
public IDtrBarEntry Get(string title, SeString? text = null)
{
// If we already have a known entry for this plugin, return it.
if (this.pluginEntries.TryGetValue(title, out var existingEntry)) return existingEntry;
public IDtrBarEntry Get(string title, SeString? text = null) => this.dtrBarService.Get(this.plugin, title, text);
return this.pluginEntries[title] = this.dtrBarService.Get(title, text);
}
/// <inheritdoc/>
public void Remove(string title)
{
if (this.pluginEntries.TryGetValue(title, out var existingEntry))
{
existingEntry.Remove();
this.pluginEntries.Remove(title);
}
}
private void OnDtrEntryRemoved(string title)
{
this.pluginEntries.Remove(title);
}
public void Remove(string title) => this.dtrBarService.Remove(this.plugin, title);
}

View file

@ -1,7 +1,6 @@
using System.Linq;
using Dalamud.Configuration.Internal;
using Dalamud.Configuration.Internal;
using Dalamud.Game.Text.SeStringHandling;
using Dalamud.Plugin.Internal.Types;
using Dalamud.Utility;
using FFXIVClientStructs.FFXIV.Client.System.String;
@ -86,6 +85,7 @@ public interface IDtrBarEntry : IReadOnlyDtrBarEntry
/// <summary>
/// Class representing an entry in the server info bar.
/// </summary>
[Api11ToDo(Api11ToDoAttribute.MakeInternal)]
public sealed unsafe class DtrBarEntry : IDisposable, IDtrBarEntry
{
private readonly DalamudConfiguration configuration;
@ -146,7 +146,7 @@ public sealed unsafe class DtrBarEntry : IDisposable, IDtrBarEntry
}
/// <inheritdoc/>
[Api10ToDo("Maybe make this config scoped to internalname?")]
[Api11ToDo("Maybe make this config scoped to internalname?")]
public bool UserHidden => this.configuration.DtrIgnore?.Contains(this.Title) ?? false;
/// <summary>
@ -160,9 +160,9 @@ public sealed unsafe class DtrBarEntry : IDisposable, IDtrBarEntry
internal Utf8String* Storage { get; set; }
/// <summary>
/// Gets a value indicating whether this entry should be removed.
/// Gets or sets a value indicating whether this entry should be removed.
/// </summary>
internal bool ShouldBeRemoved { get; private set; }
internal bool ShouldBeRemoved { get; set; }
/// <summary>
/// Gets or sets a value indicating whether this entry is dirty.
@ -174,6 +174,11 @@ public sealed unsafe class DtrBarEntry : IDisposable, IDtrBarEntry
/// </summary>
internal bool Added { get; set; }
/// <summary>
/// Gets or sets the plugin that owns this entry.
/// </summary>
internal LocalPlugin? OwnerPlugin { get; set; }
/// <inheritdoc/>
public bool TriggerClickAction()
{