fix: Safe Execute on JVM #2215
Merged
fix: Safe Execute on JVM #2215
Conversation
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
bitsandfoxes
commented
Jun 25, 2025
Comment on lines
+68
to
+72
| if (!MainThreadData.IsMainThread()) | ||
| { | ||
| _logger?.LogError("Calling IsEnabled() on Android SDK requires running on MainThread"); | ||
| return null; | ||
| } |
There was a problem hiding this comment.
This is called by the SDK as part of the initialization process which is currently explicitly required to run on the main thread. I'm adding the check here now too so any future changes to the initialization flow (i.e. programmatic through user code) is not turning into a footgun.
bitsandfoxes
commented
Jun 25, 2025
Comment on lines
+89
to
+93
| if (!MainThreadData.IsMainThread()) | ||
| { | ||
| _logger?.LogError("Calling Init() on Android SDK requires running on MainThread"); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Same as above.
bitsandfoxes
commented
Jun 25, 2025
Comment on lines
+144
to
+148
| if (!MainThreadData.IsMainThread()) | ||
| { | ||
| _logger?.LogError("Calling GetInstallationId() on Android SDK requires running on MainThread"); | ||
| return null; | ||
| } |
There was a problem hiding this comment.
Same as above.
bitsandfoxes
commented
Jun 25, 2025
Comment on lines
+177
to
+181
| if (!MainThreadData.IsMainThread()) | ||
| { | ||
| _logger?.LogError("Calling CrashedLastRun() on Android SDK requires running on MainThread"); | ||
| return null; | ||
| } |
There was a problem hiding this comment.
Same as above.
bitsandfoxes
commented
Jun 25, 2025
Comment on lines
+199
to
+203
| if (!MainThreadData.IsMainThread()) | ||
| { | ||
| _logger?.LogError("Calling Close() on Android SDK requires running on MainThread"); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Same as above.
vaind
approved these changes
Jun 25, 2025
This was referenced Jun 25, 2025
…entry/sentry-unity into fix/android-jni-shenanigans
|
CI flaked because GitHub got unicorns |
bruno-garcia
approved these changes
Jun 25, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Basically a followup on the scope sync changes in #2107
Fixes #2164
Problem
The issue originates from the following scenario:
attempting to detach while still running codeThe fix
Since we cannot verify whether the current thread is already attached or not we "have" to do the scope sync on a thread we actually control, so we can safely attach/detach. We only have to do that when not running on the main thread and we only do that on fire&forget scope sync calls. The rest of
SentryJavais currently guaranteed - and now required - to run on the main thread.Alternative attempts
I could not find a way to check whether the current thread is already attached to the JVM that works for all supported versions of Unity.
Check
GetVersionIn versions
2022_and_newerit's possible to callAndroidJNI.GetVersion()which returns0On older versions of Unity
GetVersionalways returns the actual version, creating a false positive.Try-Catch AndroidException
I tried an approach by accessing AndroidObjects while not attached to capture the resulting AndroidException which again, worked on
2022_and_newerbut straight up crashes the game on older versions.