fix: Moved late option mutation into `Init` #2239

bitsandfoxes · 2025-07-10T13:42:07Z

This is a follow-up on #2227 as this was getting already kinda big.

Goal

This PR moves adding integrations that depend on certain flags into the Init call to be added "late" to keep the SDKs behavious consistent between self and manual initialization.

Context

There are two ways SentryUnityOptions gets created:

Self-Initializing: The SDK reads the .asset and creates new options from the ScriptableObject
Manual-Initialization: User code initializes the SDK

In both cases, changes to the options need to be made at the same time. This leaves either the constructor or right before initialization.

This means that handling those "late" integrations moved out of ScriptableUnityOptions - which is not getting called when initializing manually - into the initialization flow.

Note

One could point out the inconsistency how integrations get controlled and added, i.e. AnrIntegration, ScreenshotProcessor and ViewHierarchyProcessor. We could make changes to this but we'd require users to update from

options.AttachScreenshot = true;

to

options.AddEventProcessor(new ViewHierarchyEventProcessor(options));

which is not all that much better.

…tsentry/sentry-unity into fix/new-init-options-creation

bitsandfoxes · 2025-07-10T14:45:07Z

src/Sentry.Unity/ScriptableSentryUnityOptions.cs

-        if (options.AttachViewHierarchy)
-        {
-            options.AddEventProcessor(new ViewHierarchyEventProcessor(options));
-        }
-        if (options.AttachScreenshot)
-        {
-            options.AddEventProcessor(new ScreenshotEventProcessor(options));
-        }
-
-        if (!application.IsEditor && options.Il2CppLineNumberSupportEnabled && unityInfo is not null)
-        {
-            options.AddIl2CppExceptionProcessor(unityInfo);
-        }
-
-        HandlePlatformRestrictedOptions(options, unityInfo, application);


This got moved into SentryUnitySdk.Init to keep the SDKs behaviour consistent.

bitsandfoxes · 2025-07-10T14:45:43Z

src/Sentry.Unity/ScriptableSentryUnityOptions.cs

@@ -252,41 +246,6 @@ internal SentryUnityOptions ToSentryUnityOptions(bool isBuilding, ISentryUnityIn
        return options;
    }

-    internal void HandlePlatformRestrictedOptions(SentryUnityOptions options, ISentryUnityInfo? unityInfo, IApplication application)


Same as above. Even if users call Init the SDK should make an effort not to crash the game.

bitsandfoxes · 2025-07-10T14:46:11Z

src/Sentry.Unity/SentryUnityOptions.cs

-    internal SentryUnityOptions(bool isBuilding, IApplication application, ISentryUnityInfo? unityInfo = null) :
-        this(SentryMonoBehaviour.Instance, application, isBuilding, unityInfo)


Got rid of a redundant internal constructor.

bitsandfoxes · 2025-07-10T14:47:50Z

src/Sentry.Unity/SentryUnityOptionsExtensions.cs

-    internal static void AddIl2CppExceptionProcessor(this SentryUnityOptions options, ISentryUnityInfo unityInfo)
-    {
-        if (unityInfo.Il2CppMethods is not null)
-        {
-            options.AddExceptionProcessor(new UnityIl2CppEventExceptionProcessor(options, unityInfo));
-        }
-        else
-        {
-            options.DiagnosticLogger?.LogWarning("Failed to find required IL2CPP methods - Skipping line number support");
-        }
-    }


Moved to Init.

bitsandfoxes · 2025-07-10T14:48:11Z

src/Sentry.Unity/SentryUnitySdk.cs

+    internal static void HandleWindowsPlayer(SentryUnitySdk unitySdk, SentryUnityOptions options)
+    {
+        // On Windows-Standalone, we disable cache dir in case multiple app instances run over the same path.
+        // Note: we cannot use a named Mutex, because Unity doesn't support it. Instead, we create a file with `FileShare.None`.
+        // https://forum.unity.com/threads/unsupported-internal-call-for-il2cpp-mutex-createmutex_internal-named-mutexes-are-not-supported.387334/
+        if (ApplicationAdapter.Instance.Platform is not RuntimePlatform.WindowsPlayer ||
+            options.CacheDirectoryPath is null)
+        {
+            return;
+        }
+
+        try
+        {
+            unitySdk._lockFile = new FileStream(Path.Combine(options.CacheDirectoryPath, "sentry-unity.lock"), FileMode.OpenOrCreate,
+                FileAccess.ReadWrite, FileShare.None);
+        }
+        catch (Exception ex)
+        {
+            options.DiagnosticLogger?.LogWarning("An exception was thrown while trying to " +
+                                                 "acquire a lockfile on the config directory: .NET event cache will be disabled.", ex);
+            options.CacheDirectoryPath = null;
+            options.AutoSessionTracking = false;
+        }
+    }


To keep the Init more readable.

bitsandfoxes · 2025-07-10T14:48:32Z

src/Sentry.Unity/SentryUnitySdk.cs

+        if (!ApplicationAdapter.Instance.IsEditor &&
+            (SentryPlatformServices.UnityInfo?.IL2CPP ?? false) &&
+            options.Il2CppLineNumberSupportEnabled)
+        {
+            if (SentryPlatformServices.UnityInfo.Il2CppMethods is not null)
+            {
+                options.AddExceptionProcessor(new UnityIl2CppEventExceptionProcessor(options));
+            }
+            else
+            {
+                options.DiagnosticLogger?.LogWarning("Failed to find required IL2CPP methods - Skipping line number support");
+            }


The IL2CPP Exception Processor is critical and "dangerous" as it calls into the IL2CPP backend and requires certain functions to be there. These functions change depending of Unity versions and are provided via IUnityInfo. As such, this does not get added by default but late during Init with extensive checks.

bruno-garcia · 2025-07-10T19:31:26Z

This is a follow-up on #2227 as this was getting already kinda big.

"kinda" 😅

bruno-garcia · 2025-07-10T19:33:53Z

src/Sentry.Unity/Il2CppEventProcessor.cs

-            "Unity IL2CPP methods are not available.");
+
+        // We're throwing here but this should never happen. We're validating UnityInfo before adding the processor.
+        UnityInfo = SentryPlatformServices.UnityInfo


great! this was my only feedback in the other PR, validation that we're at the right state before going ahead

Also note that you copy a reference to use the same/consistent reference. This also means there's no path for a user to change these values after-the-fact.

But I understand that's the goal, and we then should follow this pattern when it makes sense. Instead of accessing the static mutable property in many places try to keep it to a minimum.

bruno-garcia · 2025-07-10T19:39:07Z

src/Sentry.Unity/SentryUnityOptions.cs

-    internal SentryUnityOptions(bool isBuilding, IApplication application, ISentryUnityInfo? unityInfo = null) :
-        this(SentryMonoBehaviour.Instance, application, isBuilding, unityInfo)
+    public SentryUnityOptions() :
+        this(false, ApplicationAdapter.Instance, SentryPlatformServices.UnityInfo, SentryMonoBehaviour.Instance)


we access SentryPlatformServices.UnityInfo here but also in other places where they take SentryOptions but also ISentryUnityInfo. This makes it a lot more confusing.

Can we then only get it from here?

Once you do new Options we read SentryPlatformServices.UnityInfo and if it's null, we throw. Or go into the mode where this doesn't exist if that's a thing. But from that point any access to UnityInfo is through options. If you find references of the getter for SentryPlatformServices.UnityInfo you'd only see tests and this contructor.

bruno-garcia · 2025-07-10T19:40:51Z

src/Sentry.Unity/SentryUnitySdk.cs

+        // Some integrations are controlled through a flag and opt-in. Adding these integrations late so we have the
+        // same behaviour whether we're self or manually initializing
+        HandleLateIntegrations(options);
+        HandlePlatformRestrictedOptions(options, SentryPlatformServices.UnityInfo);


example where we take options and also access the static mutable public property

btw, I'm saying "static public mutable property" a lot because it's a thing you should be aware, I didn't read it btw just got the first link:

https://www.reddit.com/r/learnprogramming/comments/7nzlec/why_are_mutable_static_variables_bad/

bruno-garcia · 2025-07-10T19:43:44Z

src/Sentry.Unity/SentryUnitySdk.cs

@@ -137,4 +125,77 @@ public void CaptureFeedback(string message, string? email, string? name, bool ad

        Sentry.SentrySdk.CurrentHub.CaptureFeedback(message, email, name, hint: hint);
    }
+
+    internal static void HandleWindowsPlayer(SentryUnitySdk unitySdk, SentryUnityOptions options)


Not sure this method is called. "Handle"? It seems it's "Setting Up"? or Initializing?

It's being called by Init so perhaps InitWindowsPlayer?

bruno-garcia · 2025-07-10T19:45:01Z

src/Sentry.Unity/SentryUnitySdk.cs

+        }
+    }
+
+    internal static void HandlePlatformRestrictedOptions(SentryUnityOptions options, ISentryUnityInfo? unityInfo)


Again method name is off, seems the guard is: IsKnownPlatform so how about:

InitUnknownPlatform

It's a bit odd to see on the callsite:

InitWindows InitOtherPlatform

Since you'd expect the guards to be outside the functions as the function names already 'declare' what they are supposed to be for.

But this is all internal code so not worth splitting hairs 🙈 And I don't have a great suggestion on how to improve this other than maybe trying yet other method names "TryInit" or whatever

bruno-garcia · 2025-07-10T19:48:05Z

test/Sentry.Unity.Tests/ScriptableSentryUnityOptionsTests.cs

-    }
-
-    [Test]
-    public void HandlePlatformRestrictedOptions_KnownPlatform_SetsRestrictedOptions()


this was lost on purpose?

bruno-garcia · 2025-07-10T19:48:15Z

test/Sentry.Unity.Tests/ScriptableSentryUnityOptionsTests.cs

@@ -127,18 +127,6 @@ public void ToSentryUnityOptions_HasOptionsConfiguration_GetsCalled(bool isBuild
        Assert.AreEqual(optionsConfiguration.GotCalled, !isBuilding);
    }

-    [Test]
-    public void ToSentryUnityOptions_UnknownPlatforms_DoesNotAccessDisk()


This was also deleted on purpsoe?

bitsandfoxes and others added 6 commits July 10, 2025 15:18

moved late integrations and sumsuch into init

0b8a02f

Updated CHANGELOG.md

7d0ffca

Merge branch 'fix/initialization' into fix/new-init-options-creation

2ed2686

Format code

4379954

Updated CHANGELOG.md

26567b3

Merge branch 'fix/new-init-options-creation' of https://github.com/ge…

24c4d94

…tsentry/sentry-unity into fix/new-init-options-creation

bitsandfoxes commented Jul 10, 2025

View reviewed changes

Merge branch 'fix/initialization' into fix/new-init-options-creation

70222d3

bitsandfoxes requested review from bruno-garcia, vaind and Flash0ver July 10, 2025 14:52

bruno-garcia reviewed Jul 10, 2025

View reviewed changes

		internal SentryUnityOptions(bool isBuilding, IApplication application, ISentryUnityInfo? unityInfo = null) :
		this(SentryMonoBehaviour.Instance, application, isBuilding, unityInfo)

Uh oh!

fix: Moved late option mutation into Init #2239

Are you sure you want to change the base?

fix: Moved late option mutation into Init #2239

Uh oh!

Conversation

bitsandfoxes commented Jul 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Goal

Context

Note

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bruno-garcia commented Jul 10, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

fix: Moved late option mutation into `Init` #2239

fix: Moved late option mutation into `Init` #2239

bitsandfoxes commented Jul 10, 2025 •

edited

Loading