bugsnag / bugsnag-unity

Automatic error reporting for Unity games
https://docs.bugsnag.com/platforms/unity
MIT License
89 stars 31 forks source link

InvalidOperationException Collection was modified; enumeration operation may not execute #782

Closed gagbaghdas closed 2 months ago

gagbaghdas commented 4 months ago

Describe the bug

I'm seeing exceptions coming from BugsnagUnity.Client.

Steps to reproduce

I couldn't reproduce when the exception is being thrown, so can't give exact stepts.

Environment

Example Repo

Example code snippet

# (Insert code sample to reproduce the problem)
Error messages: ``` InvalidOperationException Collection was modified; enumeration operation may not execute. <00000000000000000000000000000000> System.Collections.Generic.List`1+Enumerator[T].MoveNext() <00000000000000000000000000000000> BugsnagUnity.Client.NotifyOnMainThread(BugsnagUnity.Payload.Error[] exceptions, BugsnagUnity.Payload.HandledState handledState, System.Func`2[T,TResult] callback, System.Nullable`1[T] logType) <00000000000000000000000000000000> BugsnagUnity.Client+<>c__DisplayClass56_0.b__0() <00000000000000000000000000000000> BugsnagUnity.MainThreadDispatchBehaviour+d__6.MoveNext() <00000000000000000000000000000000> UnityEngine.SetupCoroutine.InvokeMoveNext(System.Collections.IEnumerator enumerator, System.IntPtr returnValueAddress) <00000000000000000000000000000000> BugsnagUnity.MainThreadDispatchBehaviour.Update() ``` ``` InvalidOperationException Collection was modified; enumeration operation may not execute. <00000000000000000000000000000000> System.Collections.Generic.List`1+Enumerator[T].MoveNextRare() <00000000000000000000000000000000> System.Collections.Generic.List`1+Enumerator[T].MoveNext() <00000000000000000000000000000000> BugsnagUnity.Client.NotifyOnMainThread(BugsnagUnity.Payload.Error[] exceptions, BugsnagUnity.Payload.HandledState handledState, System.Func`2[T,TResult] callback, System.Nullable`1[T] logType) <00000000000000000000000000000000> BugsnagUnity.MainThreadDispatchBehaviour+d__6.MoveNext() <00000000000000000000000000000000> UnityEngine.SetupCoroutine.InvokeMoveNext(System.Collections.IEnumerator enumerator, System.IntPtr returnValueAddress) <00000000000000000000000000000000> BugsnagUnity.MainThreadDispatchBehaviour.Update() ```
gagbaghdas commented 4 months ago

It seems it is being thrown when the notify is being called from different threads. Any thoughts?

clr182 commented 4 months ago

Hi @gagbaghdas

Can you please share a copy of your BugSnag configuration and callback configurations, including any code snippets or screenshots that may aid in our investigation?

gagbaghdas commented 4 months ago

@clr182 sure. Here is my code:

using System;
using System.Collections.Generic;
using BugsnagUnity;

namespace MyNameSpace
{
    public class BugsnagWrapper : MyInterface
    {
        private const string bugsangApiKey = "MySDKKey";
        private bool initialized = false;
        private bool userInitialized = false;
        private ErrorReportingUserMainProperties userProperties;
        private readonly Dictionary<string, object> customProperties = new();

        public void Initialize(IContext context)
        {
            // Call this method if you don't want Bugsnag to be started automatically before first scene load

            //InitializeBugsangManully();
        }

        /// <summary>
        /// Starts Bugsnag Service manually
        /// </summary>
        private void InitializeBugsangManully()
        {
            if (!initialized)
            {
                var config = BugsnagSettingsObject.LoadConfiguration();

                config.ApiKey = bugsangApiKey;

                Bugsnag.Start(config);
                initialized = true;
            }
        }

        public void InitializeUser(ErrorReportingUserMainProperties userProperties)
        {
            if (userInitialized)
            {
                return;
            }
            this.userProperties = userProperties;

            Bugsnag.SetUser(userProperties.UserId, string.Empty, string.Empty);
            AddMetadata("user", userProperties.GetPropertiesDictionary());
            userInitialized = true;
        }

        public void Notify(Exception ex)
        {
            bool BeforeNotify(BugsnagUnity.IEvent @event)
            {
                if(userProperties != null)
                {
                    if (!string.IsNullOrEmpty(userProperties.UserId) && @event.GetUser().Id != userProperties.UserId)
                    {
                        Bugsnag.SetUser(userProperties.UserId, string.Empty, string.Empty);
                    }

                    AddMetadata("user", userProperties.GetPropertiesDictionary());
                }

                AddMetadata("customProperties", customProperties);

                return true;

            }

            Bugsnag.AddOnError(BeforeNotify);
            Bugsnag.Notify(ex);
            Bugsnag.RemoveOnError(BeforeNotify);
        }

        private void AddMetadata(string section, Dictionary<string, object> addingMetadata)
        {
            if (addingMetadata.Count == 0)
            {
                return;
            }

            if (Bugsnag.GetMetadata(section) == null || Bugsnag.GetMetadata(section).Count == 0)
            {
                Bugsnag.AddMetadata(section, addingMetadata);
                return;
            }

            foreach (var metaData in addingMetadata)
            {
                if (Bugsnag.GetMetadata(section, metaData.Key) == null)
                {
                    Bugsnag.AddMetadata(section, metaData.Key, metaData.Value);
                    continue;
                }

                if (Bugsnag.GetMetadata(section, metaData.Key) != metaData.Value)
                {
                    Bugsnag.ClearMetadata(section, metaData.Key);
                    Bugsnag.AddMetadata(section, metaData.Key, metaData.Value);
                }
            }
        }

        public void Restart()
        {
            initialized = false;
        }

        public void SetCustomProperty(string key, string value)
        {
            customProperties[key] = value;
        }
    }
}

And here is the configuration:

Screenshot 2024-04-19 at 19 46 15

Thanks

clr182 commented 2 months ago

Hi @gagbaghdas

The error message InvalidOperationException: Collection was modified; enumeration operation may not execute. occurs in C# when a collection is being modified (e.g., an item is added, removed, or changed) while it is being enumerated. This is a common issue when working with collections like lists or dictionaries in multi-threaded environments or when performing operations that modify the collection during an iteration. As such, you are likely correct in that this is being caused when notify is called via another thread, as it would appear that the bugsnag methods you are using are not protected by a lock.

Rather, you could look to use the notify overwrite function that accepts a function and callback in one method, instead of the Bugsnag.AddOnError(BeforeNotify), Bugsnag.Notify(ex) and Bugsnag.RemoveOnError(beforeNotify) methods to avoid any unnecessary modifications during enumeration.

Hopefully this helps! Please let us know if you have any further questions.

clr182 commented 2 months ago

Hi @gagbaghdas

I'm updating this thread to let you know that we have just released v8.0.0 of the Unity notifier. This version addresses threading for callback collections within the configuration class and making them thread safe.

You can find this release here: https://github.com/bugsnag/bugsnag-unity/releases/tag/v8.0.0

After updating to the latest version, if you continue to see these issues please let us know and we can investigate further.