fix: Moved late option mutation into Init
#2239
Conversation
…tsentry/sentry-unity into fix/new-init-options-creation
| 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); |
There was a problem hiding this comment.
This got moved into SentryUnitySdk.Init to keep the SDKs behaviour consistent.
| return options; | ||
| } | ||
|
|
||
| internal void HandlePlatformRestrictedOptions(SentryUnityOptions options, ISentryUnityInfo? unityInfo, IApplication application) |
There was a problem hiding this comment.
Same as above. Even if users call Init the SDK should make an effort not to crash the game.
| internal SentryUnityOptions(bool isBuilding, IApplication application, ISentryUnityInfo? unityInfo = null) : | ||
| this(SentryMonoBehaviour.Instance, application, isBuilding, unityInfo) |
There was a problem hiding this comment.
Got rid of a redundant internal constructor.
| 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"); | ||
| } | ||
| } |
There was a problem hiding this comment.
Moved to Init.
src/Sentry.Unity/SentryUnitySdk.cs
Outdated
| 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; | ||
| } | ||
| } |
There was a problem hiding this comment.
To keep the Init more readable.
| 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"); | ||
| } |
There was a problem hiding this comment.
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.
…try-unity into fix/initialization
"kinda" 😅 |
| "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 |
There was a problem hiding this comment.
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.
src/Sentry.Unity/SentryUnitySdk.cs
Outdated
|
|
||
| try | ||
| { | ||
| unitySdk._lockFile = new FileStream(Path.Combine(options.CacheDirectoryPath, "sentry-unity.lock"), FileMode.OpenOrCreate, |
There was a problem hiding this comment.
oh this looks odd. We're accessing a field from that instance in here? If that's internal name it LockFIle or something (Pascal Case)
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
Initcall to be added "late" to keep the SDKs behavious consistent between self and manual initialization.Context
There are two ways
SentryUnityOptionsgets created:.assetand creates new options from the ScriptableObjectIn 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,ScreenshotProcessorandViewHierarchyProcessor. We could make changes to this but we'd require users to update fromto
which is not all that much better.