feat: native signal handler strategy on Android#4676
Conversation
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4676 +/- ##
==========================================
+ Coverage 73.90% 73.92% +0.01%
==========================================
Files 497 497
Lines 17957 17965 +8
Branches 3516 3517 +1
==========================================
+ Hits 13271 13280 +9
+ Misses 3825 3824 -1
Partials 861 861 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@jamescrosswell This is marked as a draft because it's still waiting for dependencies, but we could already kick off the review process if you have time. :) Do you have a preferred approach for exposing this as an opt-in API? The current proposal mirrors the sentry-native enum. #if ANDROID
options.Native.SignalHandlerStrategy = Sentry.Android.SignalHandlerStrategy.ChainAtStart;
#endifWith only two values, a simple boolean flag could also work for the time being, but I'm unsure if potential Android Tombstone support could change things in the future. |
jpnurmi
force-pushed
the
feat/android-signal-handler-strategy
branch
from
af57caa to
4486918
Compare
jpnurmi
force-pushed
the
feat/android-signal-handler-strategy
branch
2 times, most recently
from
7a489eb to
543b3e3
Compare
|
Update: I'll get back to this after #4750 to make sure this works with .NET 10 |
Semver Impact of This PR⚪ None (no version bump detected) 📋 Changelog PreviewThis is how your changes will appear in the changelog. Features ✨
Fixes 🐛
Dependencies ⬆️Deps
Other
🤖 This preview updates automatically when you update the PR. |
|
While the signal handler strategy fixes the redundant |
|
.NET 10 on ARM64 and .NET 9.0 on both ARM64 and X86_64 behave as desired when sentry-native calls the runtime's signal handler. It returns to sentry-native which then captures the crash. .NET 10 on X86_64, however, crashes trying to lock a destroyed mutex? No crash captured. .NET 9.0 (Android ARM64 & X86_64) 👍NET 10.0 (Android ARM64) 👍NET 10.0 (Android X86_64) 👎 |
|
Claude confidently claims the issue was fixed by: The fix was backported to Investigation: .NET 10 x86_64 native crash test failureTraced the root cause via What happensWhen sentry-native defers to Mono's signal handler (
The assertion triggers FixAlready merged upstream:
|
jpnurmi
force-pushed
the
feat/android-signal-handler-strategy
branch
from
b64b284 to
bda8f1b
Compare
|
SDK 10.0.103 ships with runtime 10.0.3, which was built 12 days before the fix was merged. The fix is in the
Confirmed with strace — the crash sequence is identical to before: |
|
I confirmed with a locally built I used the 10.0.3 baseline of |
Nice! So once 10.0.30 is released we could put together a fix. Would our SDK users also need to be building with 10.0.3 or is it sufficient for us to be building the Sentry SDK for .NET with it? |
|
Applying the fix when building Sentry alone is not sufficient. The fix must be present in the .NET runtime / Mono DLL that is packaged inside the application APK. Unfortunately, the fix was not included in version 10.0.3, which was released a few days ago. We'll have to wait for 10.0.4, the next monthly servicing update. |
jpnurmi
force-pushed
the
feat/android-signal-handler-strategy
branch
from
bda8f1b to
862423a
Compare
jpnurmi
force-pushed
the
feat/android-signal-handler-strategy
branch
from
862423a to
47248ba
Compare
|
Looks like 10.0.4 is finally out and even 10.0.5, which we have a PR to bump to here: That PR needs some debugging - @jpnurmi would you have the bandwidth to look at it or should @Flash0ver or I pick it up? |
|
Another issue has come up. 😓 While testing against 10.0.4, I discovered that the refactored inproc backend introduced in sentry-native 0.13 broke the To prevent similar regressions going forward, we're also adding a .NET Android integration test to the Native SDK: |
Hm... what's your gut tell you about the robustness of the solution we're putting in place here? Is it something that's likely to work for long periods without us needing to pay attention to it or is it possibly fragile and going to be a time sink? I mean, pretty much everything about MAUI seems fragile to some extent... is this going to be worse though? |
|
As AI would put it, "You're absolutely right to question that!" Chaining signal handlers as we do with the "chain-at-start" strategy is not an officially supported use case and could potentially break in any future version of .NET or Android. Furthermore, there's an endless combination of versions and targets/devices/emulators/architectures to cover, and we have quite big gaps in testing as we currently can't even test on ARM64 (#4660). If we still want to release this, it's best to start with an opt-in to gather feedback. |
🤣
That sounds like a good approach... maybe we leave it opt in for a decent amount of time to see if it can survive major version bumps in the various different moving parts. |
2 participants
Opt-in, Android-only solution for:
According to our newly introduced Android integration tests, getsentry/sentry-native#1392 works on
android-arm64andandroid-x64in bothReleaseandDebugconfigurations, but we'd like to validate the fix further in real-world conditions. To that end, we're making it opt-in initially so customers can try it on more devices, platforms, and configurations before considering it as the new default.Note
Depends on: