PLAT-12181 make callback collections thread safe

CHANGELOG.md

@@ -12,6 +12,8 @@
 
 - Added a null check to the Bugsnag client to prevent crashes when accessing the client before it has been initialised [#788](https://github.com/bugsnag/bugsnag-unity/pull/788)
 
+- Made all callback collections within the Configuration class thread safe. [#810](https://github.com/bugsnag/bugsnag-unity/pull/810)
+
 ### Dependencies
 
 - Update bugsnag-cocoa to [v6.28.1](https://github.com/bugsnag/bugsnag-cocoa/releases/tag/v6.28.1) [#774](https://github.com/bugsnag/bugsnag-unity/pull/774)

src/BugsnagUnity/Configuration.cs

@@ -222,55 +222,86 @@ private bool IsRunningInEditor()
                 || Application.platform == RuntimePlatform.LinuxEditor;
         }
 
+        // Thread-safe collections with locks
+        private readonly object _onErrorLock = new object();
         private List<Func<IEvent, bool>> _onErrorCallbacks = new List<Func<IEvent, bool>>();
 
+        private readonly object _onSendErrorLock = new object();
+        private List<Func<IEvent, bool>> _onSendErrorCallbacks = new List<Func<IEvent, bool>>();
+
+        private readonly object _onSessionLock = new object();
+        private List<Func<ISession, bool>> _onSessionCallbacks = new List<Func<ISession, bool>>();
+
         public void AddOnError(Func<IEvent, bool> callback)
         {
-            _onErrorCallbacks.Add(callback);
+            lock (_onErrorLock)
+            {
+                _onErrorCallbacks.Add(callback);
+            }
         }
 
         internal List<Func<IEvent, bool>> GetOnErrorCallbacks()
         {
-            return _onErrorCallbacks;
+            lock (_onErrorLock)
+            {
+                return _onErrorCallbacks.ToList();
+            }
         }
 
         public void RemoveOnError(Func<IEvent, bool> callback)
         {
-            _onErrorCallbacks.Remove(callback);
+            lock (_onErrorLock)
+            {
+                _onErrorCallbacks.Remove(callback);
+            }
         }
 
-        private List<Func<IEvent, bool>> _onSendErrorCallbacks = new List<Func<IEvent, bool>>();
-
         public void AddOnSendError(Func<IEvent, bool> callback)
         {
-            _onSendErrorCallbacks.Add(callback);
+            lock (_onSendErrorLock)
+            {
+                _onSendErrorCallbacks.Add(callback);
+            }
         }
 
         internal List<Func<IEvent, bool>> GetOnSendErrorCallbacks()
         {
-            return _onSendErrorCallbacks;
+            lock (_onSendErrorLock)
+            {
+                return _onSendErrorCallbacks.ToList();
+            }
         }
 
         public void RemoveOnSendError(Func<IEvent, bool> callback)
         {
-            _onSendErrorCallbacks.Remove(callback);
+            lock (_onSendErrorLock)
+            {
+                _onSendErrorCallbacks.Remove(callback);
+            }
         }
 
-        private List<Func<ISession, bool>> _onSessionCallbacks = new List<Func<ISession, bool>>();
-
         public void AddOnSession(Func<ISession, bool> callback)
         {
-            _onSessionCallbacks.Add(callback);
+            lock (_onSessionLock)
+            {
+                _onSessionCallbacks.Add(callback);
+            }
         }
 
         public void RemoveOnSession(Func<ISession, bool> callback)
         {
-            _onSessionCallbacks.Remove(callback);
+            lock (_onSessionLock)
+            {
+                _onSessionCallbacks.Remove(callback);
+            }
         }
 
         internal List<Func<ISession, bool>> GetOnSessionCallbacks()
         {
-            return _onSessionCallbacks;
+            lock (_onSessionLock)
+            {
+                return _onSessionCallbacks.ToList();
+            }
         }
 
         public List<TelemetryType> Telemetry = new List<TelemetryType> { TelemetryType.InternalErrors, TelemetryType.Usage };

tests/BugsnagUnity.Tests/ConfigurationTests.cs

@@ -1,4 +1,6 @@
 using NUnit.Framework;
+using System;
+using System.Collections.Generic;
 using System.Linq;
 using System.Text.RegularExpressions;
 using System.Threading;
@@ -121,5 +123,87 @@ public void DiscardedClassesTest()
             Assert.IsTrue(config2.ErrorClassIsDiscarded("System.NullReferenceException"));
             Assert.IsFalse(config2.ErrorClassIsDiscarded("System.ArgumentException"));
         }
+
+        [Test]
+        public void ThreadSafeCallbacksTest()
+        {
+            var config = new Configuration("foo");
+
+            // Define a simple callback function
+            Func<IEvent, bool> callback1 = (e) => true;
+            Func<IEvent, bool> callback2 = (e) => false;
+            Func<ISession, bool> sessionCallback = (s) => true;
+
+            // We will use these lists to store the results from multiple threads
+            List<bool> onErrorResults = new List<bool>();
+            List<bool> onSendErrorResults = new List<bool>();
+            List<bool> onSessionResults = new List<bool>();
+
+            // Adding callbacks in multiple threads
+            Thread addThread1 = new Thread(() =>
+            {
+                for (int i = 0; i < 50; i++)
+                {
+                    config.AddOnError(callback1);
+                    config.AddOnSendError(callback2);
+                    config.AddOnSession(sessionCallback);
+                }
+            });
+
+            Thread addThread2 = new Thread(() =>
+            {
+                for (int i = 0; i < 50; i++)
+                {
+                    config.AddOnError(callback2);
+                    config.AddOnSendError(callback1);
+                    config.AddOnSession(sessionCallback);
+                }
+            });
+
+            // Removing callbacks in multiple threads
+            Thread removeThread1 = new Thread(() =>
+            {
+                for (int i = 0; i < 25; i++)
+                {
+                    config.RemoveOnError(callback1);
+                    config.RemoveOnSendError(callback2);
+                    config.RemoveOnSession(sessionCallback);
+                }
+            });
+
+            Thread removeThread2 = new Thread(() =>
+            {
+                for (int i = 0; i < 25; i++)
+                {
+                    config.RemoveOnError(callback2);
+                    config.RemoveOnSendError(callback1);
+                    config.RemoveOnSession(sessionCallback);
+                }
+            });
+
+            // Start all threads
+            addThread1.Start();
+            addThread2.Start();
+            removeThread1.Start();
+            removeThread2.Start();
+
+            // Wait for all threads to complete
+            addThread1.Join();
+            addThread2.Join();
+            removeThread1.Join();
+            removeThread2.Join();
+
+            // Verify the state of the callback lists
+            // The exact number might vary depending on the execution order,
+            // but there should be no exceptions thrown and the list should not be empty
+            Assert.IsTrue(config.GetOnErrorCallbacks().Count > 0, "OnErrorCallbacks should have entries.");
+            Assert.IsTrue(config.GetOnSendErrorCallbacks().Count > 0, "OnSendErrorCallbacks should have entries.");
+            Assert.IsTrue(config.GetOnSessionCallbacks().Count > 0, "OnSessionCallbacks should have entries.");
+
+            // Check if the remaining callbacks are as expected
+            Assert.IsTrue(config.GetOnErrorCallbacks().Contains(callback1) || config.GetOnErrorCallbacks().Contains(callback2), "Callback1 or Callback2 should be in OnErrorCallbacks.");
+            Assert.IsTrue(config.GetOnSendErrorCallbacks().Contains(callback1) || config.GetOnSendErrorCallbacks().Contains(callback2), "Callback1 or Callback2 should be in OnSendErrorCallbacks.");
+            Assert.IsTrue(config.GetOnSessionCallbacks().Contains(sessionCallback), "SessionCallback should be in OnSessionCallbacks.");
+        }
     }
 }