-
-
Notifications
You must be signed in to change notification settings - Fork 56
fix: Safe Execute on JVM #2215
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: Safe Execute on JVM #2215
Conversation
if (!MainThreadData.IsMainThread()) | ||
{ | ||
_logger?.LogError("Calling IsEnabled() on Android SDK requires running on MainThread"); | ||
return null; | ||
} |
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.
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.
if (!MainThreadData.IsMainThread()) | ||
{ | ||
_logger?.LogError("Calling Init() on Android SDK requires running on MainThread"); | ||
return; | ||
} |
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.
Same as above.
if (!MainThreadData.IsMainThread()) | ||
{ | ||
_logger?.LogError("Calling GetInstallationId() on Android SDK requires running on MainThread"); | ||
return null; | ||
} |
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.
Same as above.
if (!MainThreadData.IsMainThread()) | ||
{ | ||
_logger?.LogError("Calling CrashedLastRun() on Android SDK requires running on MainThread"); | ||
return null; | ||
} |
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.
Same as above.
if (!MainThreadData.IsMainThread()) | ||
{ | ||
_logger?.LogError("Calling Close() on Android SDK requires running on MainThread"); | ||
return; | ||
} |
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.
Same as above.
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.
LGTM & nice writeup. 🤞 this really fixes the issue.
…entry/sentry-unity into fix/android-jni-shenanigans
CI flaked because GitHub got unicorns
|
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 code
The 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
SentryJava
is 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
GetVersion
In versions
2022_and_newer
it's possible to callAndroidJNI.GetVersion()
which returns0
On older versions of Unity
GetVersion
always 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_newer
but straight up crashes the game on older versions.