diff --git a/CHANGELOG.md b/CHANGELOG.md index 548cc596c..0975026d8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,10 @@ ## Unreleased +## Fixes + +- Fixed race conditions that could cause crashes when targeting Android, especially in concurrent scenarios. The ThreadPool used to sync scope to the native layer could prematurely detach threads from the JVM. This is solved by using a dedicated background worker thread that properly manages JNI lifecycle. ([#2426](https://github.com/getsentry/sentry-unity/pull/2426)) + ### Dependencies - Bump Java SDK from v8.25.0 to v8.26.0 ([#2419](https://github.com/getsentry/sentry-unity/pull/2419)) diff --git a/src/Sentry.Unity.Android/SentryJava.cs b/src/Sentry.Unity.Android/SentryJava.cs index ee6b42004..eb1e75310 100644 --- a/src/Sentry.Unity.Android/SentryJava.cs +++ b/src/Sentry.Unity.Android/SentryJava.cs @@ -1,4 +1,5 @@ using System; +using System.Collections.Concurrent; using System.Runtime.CompilerServices; using System.Threading; using Sentry.Extensibility; @@ -54,13 +55,24 @@ internal class SentryJava : ISentryJava private readonly IAndroidJNI _androidJNI; private readonly IDiagnosticLogger? _logger; + private readonly CancellationTokenSource _scopeSyncShutdownSource; + private readonly AutoResetEvent _scopeSyncEvent; + private readonly Thread _scopeSyncThread; + private readonly ConcurrentQueue<(Action action, string actionName)> _scopeSyncItems = new(); + private volatile bool _closed; + private static AndroidJavaObject GetInternalSentryJava() => new AndroidJavaClass("io.sentry.android.core.InternalSentrySdk"); - protected virtual AndroidJavaObject GetSentryJava() => new AndroidJavaClass("io.sentry.Sentry"); + private static AndroidJavaObject GetSentryJava() => new AndroidJavaClass("io.sentry.Sentry"); public SentryJava(IDiagnosticLogger? logger, IAndroidJNI? androidJNI = null) { _logger = logger; _androidJNI ??= androidJNI ?? AndroidJNIAdapter.Instance; + + _scopeSyncEvent = new AutoResetEvent(false); + _scopeSyncShutdownSource = new CancellationTokenSource(); + _scopeSyncThread = new Thread(SyncScope) { IsBackground = true, Name = "SentryScopeSyncWorkerThread" }; + _scopeSyncThread.Start(); } public bool? IsEnabled() @@ -197,25 +209,6 @@ public void Init(SentryUnityOptions options) return null; } - public void Close() - { - if (!MainThreadData.IsMainThread()) - { - _logger?.LogError("Calling Close() on Android SDK requires running on MainThread"); - return; - } - - try - { - using var sentry = GetSentryJava(); - sentry.CallStatic("close"); - } - catch (Exception e) - { - _logger?.LogError(e, "Calling 'SentryJava.Close' failed."); - } - } - public void WriteScope( int? GpuId, string? GpuName, @@ -365,6 +358,12 @@ public void SetTrace(SentryId traceId, SpanId spanId) internal void RunJniSafe(Action action, [CallerMemberName] string actionName = "", bool? isMainThread = null) { + if (_closed) + { + _logger?.LogInfo("Scope sync is closed, skipping '{0}'", actionName); + return; + } + isMainThread ??= MainThreadData.IsMainThread(); if (isMainThread is true) { @@ -379,24 +378,74 @@ internal void RunJniSafe(Action action, [CallerMemberName] string actionName = " } else { - ThreadPool.QueueUserWorkItem(state => - { - var (androidJNI, logger, name) = ((IAndroidJNI, IDiagnosticLogger?, string))state; + _scopeSyncItems.Enqueue((action, actionName)); + _scopeSyncEvent.Set(); + } + } - androidJNI.AttachCurrentThread(); - try - { - action.Invoke(); - } - catch (Exception) + private void SyncScope() + { + _androidJNI.AttachCurrentThread(); + + try + { + var waitHandles = new[] { _scopeSyncEvent, _scopeSyncShutdownSource.Token.WaitHandle }; + + while (true) + { + var index = WaitHandle.WaitAny(waitHandles); + if (index > 0) { - logger?.LogError("Calling '{0}' failed.", name); + // Shutdown requested + break; } - finally + + while (_scopeSyncItems.TryDequeue(out var workItem)) { - androidJNI.DetachCurrentThread(); + var (action, actionName) = workItem; + try + { + action.Invoke(); + } + catch (Exception) + { + _logger?.LogError("Calling '{0}' failed.", actionName); + } } - }, (_androidJNI, _logger, actionName)); + } + } + finally + { + _androidJNI.DetachCurrentThread(); + } + } + + public void Close() + { + _closed = true; + + _scopeSyncShutdownSource.Cancel(); + _scopeSyncThread.Join(); + + // Note: We intentionally don't dispose _scopeSyncEvent to avoid race conditions + // where other threads might call RunJniSafe() after Close() but before disposal. + // The memory overhead of a single AutoResetEvent is negligible. + _scopeSyncShutdownSource.Dispose(); + + if (!MainThreadData.IsMainThread()) + { + _logger?.LogError("Calling Close() on Android SDK requires running on MainThread"); + return; + } + + try + { + using var sentry = GetSentryJava(); + sentry.CallStatic("close"); + } + catch (Exception e) + { + _logger?.LogError(e, "Calling 'SentryJava.Close' failed."); } } } diff --git a/test/Sentry.Unity.Android.Tests/SentryJavaTests.cs b/test/Sentry.Unity.Android.Tests/SentryJavaTests.cs index 11a1ed871..f4b658e8f 100644 --- a/test/Sentry.Unity.Android.Tests/SentryJavaTests.cs +++ b/test/Sentry.Unity.Android.Tests/SentryJavaTests.cs @@ -21,7 +21,7 @@ public void SetUp() } [Test] - public void RunJniSafe_OnMainThread_ExecutesActionWithoutAttachingDetaching() + public void RunJniSafe_OnMainThread_ExecutesAction() { // Arrange var actionExecuted = false; @@ -32,21 +32,19 @@ public void RunJniSafe_OnMainThread_ExecutesActionWithoutAttachingDetaching() // Assert Assert.That(actionExecuted, Is.True); - Assert.That(_androidJni.AttachCalled, Is.False); // Sanity Check - Assert.That(_androidJni.DetachCalled, Is.False); // Sanity Check } [Test] - public void RunJniSafe_NotMainThread_ExecutesOnThreadPool() + public void RunJniSafe_NotMainThread_ExecutesOnWorkerThread() { // Arrange var actionExecuted = false; + var executionThreadId = 0; var resetEvent = new ManualResetEvent(false); - var detachResetEvent = new ManualResetEvent(false); - _androidJni.OnDetachCalled = () => detachResetEvent.Set(); var action = new Action(() => { actionExecuted = true; + executionThreadId = Thread.CurrentThread.ManagedThreadId; resetEvent.Set(); }); @@ -54,11 +52,10 @@ public void RunJniSafe_NotMainThread_ExecutesOnThreadPool() _sut.RunJniSafe(action, isMainThread: false); // Assert - Assert.That(resetEvent.WaitOne(TimeSpan.FromSeconds(1)), Is.True, "Action should execute within timeout"); - Assert.That(detachResetEvent.WaitOne(TimeSpan.FromSeconds(1)), Is.True, "Detach should execute within timeout"); + Assert.That(resetEvent.WaitOne(TimeSpan.FromSeconds(1)), Is.True); Assert.That(actionExecuted, Is.True); - Assert.That(_androidJni.AttachCalled, Is.True); - Assert.That(_androidJni.DetachCalled, Is.True, "Detach should be called"); + Assert.That(executionThreadId, Is.Not.EqualTo(Thread.CurrentThread.ManagedThreadId), + "Action should execute on worker thread, not the calling thread"); } [Test] @@ -71,8 +68,7 @@ public void RunJniSafe_ActionThrowsOnMainThread_CatchesExceptionAndLogsError() // Act _sut.RunJniSafe(action, "TestAction", isMainThread: true); - Assert.That(_androidJni.AttachCalled, Is.False); - Assert.That(_androidJni.DetachCalled, Is.False); + // Assert Assert.IsTrue(_logger.Logs.Any(log => log.logLevel == SentryLevel.Error && log.message.Contains("Calling 'TestAction' failed."))); @@ -84,8 +80,6 @@ public void RunJniSafe_ActionThrowsOnNonMainThread_CatchesExceptionAndLogsError( // Arrange var exception = new InvalidOperationException("Test exception"); var resetEvent = new ManualResetEvent(false); - var detachResetEvent = new ManualResetEvent(false); - _androidJni.OnDetachCalled = () => detachResetEvent.Set(); var action = new Action(() => { try @@ -102,52 +96,74 @@ public void RunJniSafe_ActionThrowsOnNonMainThread_CatchesExceptionAndLogsError( _sut.RunJniSafe(action, "TestAction", isMainThread: false); // Assert - Assert.That(resetEvent.WaitOne(TimeSpan.FromSeconds(1)), Is.True, "Action should execute within timeout"); - Assert.That(detachResetEvent.WaitOne(TimeSpan.FromSeconds(1)), Is.True, "Detach should execute within timeout"); - Assert.That(_androidJni.AttachCalled, Is.True); - Assert.That(_androidJni.DetachCalled, Is.True); + Assert.That(resetEvent.WaitOne(TimeSpan.FromSeconds(1)), Is.True); Assert.IsTrue(_logger.Logs.Any(log => log.logLevel == SentryLevel.Error && log.message.Contains("Calling 'TestAction' failed."))); } [Test] - public void RunJniSafe_ActionThrowsOnNonMainThread_DetachIsAlwaysCalled() + public void WorkerThread_AttachesOnCreationAndDetachesOnClose() { // Arrange - var exception = new InvalidOperationException("Test exception"); - var resetEvent = new ManualResetEvent(false); + var logger = new TestLogger(); + var androidJni = new TestAndroidJNI(); + var attachResetEvent = new ManualResetEvent(false); var detachResetEvent = new ManualResetEvent(false); - _androidJni.OnDetachCalled = () => detachResetEvent.Set(); - var action = new Action(() => - { - try - { - throw exception; - } - finally - { - resetEvent.Set(); - } - }); + androidJni.OnAttachCalled = () => attachResetEvent.Set(); + androidJni.OnDetachCalled = () => detachResetEvent.Set(); + + // Act - Create instance (should start worker thread and attach) + var sut = new SentryJava(logger, androidJni); + + // Trigger worker thread initialization by queuing work + var workExecuted = new ManualResetEvent(false); + sut.RunJniSafe(() => workExecuted.Set(), isMainThread: false); + + // Assert - Worker thread should be attached + Assert.That(workExecuted.WaitOne(TimeSpan.FromSeconds(1)), Is.True, "Work should execute to initialize worker thread"); + Assert.That(attachResetEvent.WaitOne(TimeSpan.FromSeconds(1)), Is.True, "AttachCurrentThread should be called on worker thread creation"); + Assert.That(androidJni.AttachCalled, Is.True); + + // Act - Close/Dispose (should stop worker thread and detach) + sut.Close(); + + // Assert - Worker thread should be detached + Assert.That(detachResetEvent.WaitOne(TimeSpan.FromSeconds(2)), Is.True, "DetachCurrentThread should be called when closing"); + Assert.That(androidJni.DetachCalled, Is.True); + } + + [Test] + public void RunJniSafe_AfterClose_SkipsActionAndLogsWarning() + { + // Arrange + var actionExecuted = false; + _sut.Close(); // Act - _sut.RunJniSafe(action, isMainThread: false); + _sut.RunJniSafe(() => actionExecuted = true, "TestAction", isMainThread: false); // Assert - Assert.That(resetEvent.WaitOne(TimeSpan.FromSeconds(1)), Is.True, "Action should execute within timeout"); - Assert.That(detachResetEvent.WaitOne(TimeSpan.FromSeconds(1)), Is.True, "Detach should execute within timeout"); - Assert.That(_androidJni.AttachCalled, Is.True); - Assert.That(_androidJni.DetachCalled, Is.True, "DetachCurrentThread should always be called"); + Assert.That(actionExecuted, Is.False, "Action should not execute after Close()"); + Assert.That(_logger.Logs.Any(log => + log.logLevel == SentryLevel.Info && + log.message.Contains("Scope sync is closed, skipping 'TestAction'")), + Is.True, + "Should log warning when trying to queue action after Close()"); } internal class TestAndroidJNI : IAndroidJNI { public bool AttachCalled { get; private set; } public bool DetachCalled { get; private set; } + public Action? OnAttachCalled { get; set; } public Action? OnDetachCalled { get; set; } - public void AttachCurrentThread() => AttachCalled = true; + public void AttachCurrentThread() + { + AttachCalled = true; + OnAttachCalled?.Invoke(); + } public void DetachCurrentThread() {