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 74.00% 73.99% -0.02%
==========================================
Files 499 499
Lines 18066 18067 +1
Branches 3518 3519 +1
==========================================
- Hits 13370 13368 -2
- Misses 3837 3839 +2
- Partials 859 860 +1 ☔ 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. |
af57caa to
4486918
Compare
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 |
This comment was marked as outdated.
This comment was marked as outdated.
|
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:
|
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. |
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. |
47248ba to
448380a
Compare
448380a to
13e3e29
Compare
jamescrosswell
left a comment
There was a problem hiding this comment.
Thanks @jpnurmi - the other dependencies are now merged so I think this is almost ready.
The changes themselves look fine functionally (pretty trivial - just wrapping an android SDK option). I made a few suggestions that might hopefully head off confusion for our SDK users though.
…T runtimes Add a runtime version check that falls back to Default on .NET 10.0.0–10.0.3, which crash with ChainAtStart due to dotnet/runtime#123346. Also fix swapped runtime/SDK version numbers in XML docs. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
97c35d2 to
2a04f8f
Compare
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…falling back Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…tead of falling back" This reverts commit 9698020. Use warning + fallback to Default instead of throwing during initialization. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
jamescrosswell
left a comment
There was a problem hiding this comment.
LGTM assuming the iOS failure is just flaky tests.
Massive thank you for this @jpnurmi ❤️
Experimental opt-in Android-only solution for:
According to our newly introduced Android integration tests,
CHAIN_AT_STARTworks onandroid-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: