Merge pull request #15050 from peppy/fix-cross-thread-list-manipulation-skin-source-provider

Fix cross-thread list manipulation in `SkinProvidingContainer`

osu.Game.Tests/Skins/TestSceneSkinProvidingContainer.cs

@@ -6,7 +6,6 @@ using System.Linq;
 using NUnit.Framework;
 using osu.Framework.Audio.Sample;
 using osu.Framework.Bindables;
-using osu.Framework.Extensions.IEnumerableExtensions;
 using osu.Framework.Graphics;
 using osu.Framework.Graphics.OpenGL.Textures;
 using osu.Framework.Graphics.Textures;
@@ -65,10 +64,9 @@ namespace osu.Game.Tests.Skins
 
             public new void TriggerSourceChanged() => base.TriggerSourceChanged();
 
-            protected override void OnSourceChanged()
+            protected override void RefreshSources()
             {
-                ResetSources();
+                SetSources(sources);
-                sources.ForEach(AddSource);
             }
         }
 

osu.Game/Skinning/ISkinSource.cs

@@ -12,6 +12,9 @@ namespace osu.Game.Skinning
     /// </summary>
     public interface ISkinSource : ISkin
     {
+        /// <summary>
+        /// Fired whenever a source change occurs, signalling that consumers should re-query as required.
+        /// </summary>
         event Action SourceChanged;
 
         /// <summary>

osu.Game/Skinning/RulesetSkinProvidingContainer.cs

@@ -58,10 +58,8 @@ namespace osu.Game.Skinning
             return base.CreateChildDependencies(parent);
         }
 
-        protected override void OnSourceChanged()
+        protected override void RefreshSources()
         {
-            ResetSources();
-
             // Populate a local list first so we can adjust the returned order as we go.
             var sources = new List<ISkin>();
 
@@ -91,8 +89,7 @@ namespace osu.Game.Skinning
             else
                 sources.Add(rulesetResourcesSkin);
 
-            foreach (var skin in sources)
+            SetSources(sources);
-                AddSource(skin);
         }
 
         protected ISkin GetLegacyRulesetTransformedSkin(ISkin legacySkin)

osu.Game/Skinning/SkinProvidingContainer.cs

@@ -3,6 +3,7 @@
 
 using System;
 using System.Collections.Generic;
+using System.Linq;
 using JetBrains.Annotations;
 using osu.Framework.Allocation;
 using osu.Framework.Audio.Sample;
@@ -40,10 +41,12 @@ namespace osu.Game.Skinning
 
         protected virtual bool AllowColourLookup => true;
 
+        private readonly object sourceSetLock = new object();
+
         /// <summary>
         /// A dictionary mapping each <see cref="ISkin"/> source to a wrapper which handles lookup allowances.
         /// </summary>
-        private readonly List<(ISkin skin, DisableableSkinSource wrapped)> skinSources = new List<(ISkin, DisableableSkinSource)>();
+        private (ISkin skin, DisableableSkinSource wrapped)[] skinSources = Array.Empty<(ISkin skin, DisableableSkinSource wrapped)>();
 
         /// <summary>
         /// Constructs a new <see cref="SkinProvidingContainer"/> initialised with a single skin source.
@@ -52,7 +55,7 @@ namespace osu.Game.Skinning
             : this()
         {
             if (skin != null)
-                AddSource(skin);
+                SetSources(new[] { skin });
         }
 
         /// <summary>
@@ -168,49 +171,42 @@ namespace osu.Game.Skinning
         }
 
         /// <summary>
-        /// Add a new skin to this provider. Will be added to the end of the lookup order precedence.
+        /// Replace the sources used for lookups in this container.
         /// </summary>
-        /// <param name="skin">The skin to add.</param>
+        /// <remarks>
-        protected void AddSource(ISkin skin)
+        /// This does not implicitly fire a <see cref="SourceChanged"/> event. Consider calling <see cref="TriggerSourceChanged"/> if required.
+        /// </remarks>
+        /// <param name="sources">The new sources.</param>
+        protected void SetSources(IEnumerable<ISkin> sources)
         {
-            skinSources.Add((skin, new DisableableSkinSource(skin, this)));
+            lock (sourceSetLock)
-
-            if (skin is ISkinSource source)
-                source.SourceChanged += TriggerSourceChanged;
-        }
-
-        /// <summary>
-        /// Remove a skin from this provider.
-        /// </summary>
-        /// <param name="skin">The skin to remove.</param>
-        protected void RemoveSource(ISkin skin)
             {
-            if (skinSources.RemoveAll(s => s.skin == skin) == 0)
+                foreach (var skin in skinSources)
-                return;
+                {
-
+                    if (skin.skin is ISkinSource source)
-            if (skin is ISkinSource source)
                         source.SourceChanged -= TriggerSourceChanged;
                 }
 
-        /// <summary>
+                skinSources = sources.Select(skin => (skin, new DisableableSkinSource(skin, this))).ToArray();
-        /// Clears all skin sources.
+
-        /// </summary>
+                foreach (var skin in skinSources)
-        protected void ResetSources()
                 {
-            foreach (var i in skinSources.ToArray())
+                    if (skin.skin is ISkinSource source)
-                RemoveSource(i.skin);
+                        source.SourceChanged += TriggerSourceChanged;
+                }
+            }
         }
 
         /// <summary>
-        /// Invoked when any source has changed (either <see cref="ParentSource"/> or a source registered via <see cref="AddSource"/>).
+        /// Invoked after any consumed source change, before the external <see cref="SourceChanged"/> event is fired.
         /// This is also invoked once initially during <see cref="CreateChildDependencies"/> to ensure sources are ready for children consumption.
         /// </summary>
-        protected virtual void OnSourceChanged() { }
+        protected virtual void RefreshSources() { }
 
         protected void TriggerSourceChanged()
         {
             // Expose to implementations, giving them a chance to react before notifying external consumers.
-            OnSourceChanged();
+            RefreshSources();
 
             SourceChanged?.Invoke();
         }