fix: Moved late option mutation into Init
#2239
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,6 +3,7 @@ | |
| using System.Threading.Tasks; | ||
| using Sentry.Extensibility; | ||
| using Sentry.Unity.Integrations; | ||
| using Sentry.Unity.NativeUtils; | ||
| using UnityEngine; | ||
|
|
||
| namespace Sentry.Unity; | ||
|
|
@@ -30,24 +31,11 @@ private SentryUnitySdk(SentryUnityOptions options) | |
|
|
||
| MainThreadData.CollectData(); | ||
|
|
||
| // 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); | ||
|
There was a problem hiding this comment. example where we take options and also access the static mutable public property There was a problem hiding this comment. 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/ |
||
| HandleWindowsPlayer(unitySdk, options); | ||
|
|
||
| unitySdk._dotnetSdk = Sentry.SentrySdk.Init(options); | ||
|
|
||
|
|
@@ -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) | ||
|
There was a problem hiding this comment. Not sure this method is called. "Handle"? It seems it's "Setting Up"? or Initializing? It's being called by |
||
| { | ||
| // 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; | ||
| } | ||
| } | ||
|
Comment on lines
+129
to
+152
There was a problem hiding this comment. To keep the |
||
|
|
||
| internal static void HandleLateIntegrations(SentryUnityOptions options) | ||
| { | ||
| if (options.AttachViewHierarchy) | ||
| { | ||
| options.AddEventProcessor(new ViewHierarchyEventProcessor(options)); | ||
| } | ||
| if (options.AttachScreenshot) | ||
| { | ||
| options.AddEventProcessor(new ScreenshotEventProcessor(options)); | ||
| } | ||
|
|
||
| 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"); | ||
| } | ||
|
Comment on lines
+165
to
+176
There was a problem hiding this comment. The |
||
| } | ||
| } | ||
|
|
||
| internal static void HandlePlatformRestrictedOptions(SentryUnityOptions options, ISentryUnityInfo? unityInfo) | ||
|
There was a problem hiding this comment. Again method name is off, seems the guard is:
There was a problem hiding this comment. It's a bit odd to see on the callsite: 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 |
||
| { | ||
| if (unityInfo?.IsKnownPlatform() == false) | ||
| { | ||
| options.DisableFileWrite = true; | ||
|
|
||
| // Requires file access, see https://github.com/getsentry/sentry-unity/issues/290#issuecomment-1163608988 | ||
| if (options.AutoSessionTracking) | ||
| { | ||
| options.DiagnosticLogger?.LogDebug("Platform support for automatic session tracking is unknown: disabling."); | ||
| options.AutoSessionTracking = false; | ||
| } | ||
|
|
||
| // This is only provided on a best-effort basis for other than the explicitly supported platforms. | ||
| if (options.BackgroundWorker is null) | ||
| { | ||
| options.DiagnosticLogger?.LogDebug("Platform support for background thread execution is unknown: using WebBackgroundWorker."); | ||
| options.BackgroundWorker = new WebBackgroundWorker(options, SentryMonoBehaviour.Instance); | ||
| } | ||
| } | ||
| } | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.