From cc1915bc7a8238b7262d74ff4afacfe00da77836 Mon Sep 17 00:00:00 2001 From: bitsandfoxes Date: Tue, 18 Nov 2025 15:34:08 +0100 Subject: [PATCH 1/5] Replaced ThreadPool with WorkerThread --- src/Sentry.Unity.Android/SentryJava.cs | 58 +++++++++++++++++++++----- 1 file changed, 48 insertions(+), 10 deletions(-) diff --git a/src/Sentry.Unity.Android/SentryJava.cs b/src/Sentry.Unity.Android/SentryJava.cs index ee6b42004..f8b139846 100644 --- a/src/Sentry.Unity.Android/SentryJava.cs +++ b/src/Sentry.Unity.Android/SentryJava.cs @@ -1,6 +1,8 @@ using System; +using System.Collections.Concurrent; using System.Runtime.CompilerServices; using System.Threading; +using System.Threading.Tasks; using Sentry.Extensibility; using UnityEngine; @@ -54,6 +56,12 @@ internal class SentryJava : ISentryJava private readonly IAndroidJNI _androidJNI; private readonly IDiagnosticLogger? _logger; + private readonly CancellationTokenSource _scopeSyncShutdownSource; + private readonly AutoResetEvent _scopeSyncEvent; + private Thread? _scopeSyncThread; + + private ConcurrentQueue _scopeSyncItems = new(); + private static AndroidJavaObject GetInternalSentryJava() => new AndroidJavaClass("io.sentry.android.core.InternalSentrySdk"); protected virtual AndroidJavaObject GetSentryJava() => new AndroidJavaClass("io.sentry.Sentry"); @@ -61,6 +69,11 @@ 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() @@ -199,6 +212,16 @@ public void Init(SentryUnityOptions options) public void Close() { + _scopeSyncShutdownSource?.Cancel(); + if (!_scopeSyncThread?.Join(TimeSpan.FromSeconds(2)) ?? false) + { + _logger?.LogWarning("Worker thread did not exit cleanly within timeout"); + } + + _scopeSyncEvent?.Dispose(); + _scopeSyncShutdownSource?.Dispose(); + + if (!MainThreadData.IsMainThread()) { _logger?.LogError("Calling Close() on Android SDK requires running on MainThread"); @@ -379,25 +402,40 @@ internal void RunJniSafe(Action action, [CallerMemberName] string actionName = " } else { - ThreadPool.QueueUserWorkItem(state => + _scopeSyncItems.Enqueue(action); + _scopeSyncEvent.Set(); + } + } + + private void SyncScope() + { + _androidJNI.AttachCurrentThread(); + + var waitHandles = new[] { _scopeSyncEvent, _scopeSyncShutdownSource.Token.WaitHandle }; + + while (true) + { + var index = WaitHandle.WaitAny(waitHandles); + if (index > 0) { - var (androidJNI, logger, name) = ((IAndroidJNI, IDiagnosticLogger?, string))state; + // We only care about the _taskEvent + break; + } - androidJNI.AttachCurrentThread(); + while (_scopeSyncItems.TryDequeue(out var action)) + { try { action.Invoke(); } - catch (Exception) - { - logger?.LogError("Calling '{0}' failed.", name); - } - finally + catch (Exception e) { - androidJNI.DetachCurrentThread(); + _logger?.LogError(e, "Failed to sync scope."); } - }, (_androidJNI, _logger, actionName)); + } } + + _androidJNI.DetachCurrentThread(); } } From 26051b506f60868d47cc28afac8d90f87d080a56 Mon Sep 17 00:00:00 2001 From: bitsandfoxes Date: Wed, 19 Nov 2025 10:28:05 +0100 Subject: [PATCH 2/5] Updated logging and tests --- src/Sentry.Unity.Android/SentryJava.cs | 14 ++-- .../SentryJavaTests.cs | 79 +++++++++---------- 2 files changed, 45 insertions(+), 48 deletions(-) diff --git a/src/Sentry.Unity.Android/SentryJava.cs b/src/Sentry.Unity.Android/SentryJava.cs index f8b139846..8a329e69c 100644 --- a/src/Sentry.Unity.Android/SentryJava.cs +++ b/src/Sentry.Unity.Android/SentryJava.cs @@ -2,7 +2,6 @@ using System.Collections.Concurrent; using System.Runtime.CompilerServices; using System.Threading; -using System.Threading.Tasks; using Sentry.Extensibility; using UnityEngine; @@ -60,10 +59,10 @@ internal class SentryJava : ISentryJava private readonly AutoResetEvent _scopeSyncEvent; private Thread? _scopeSyncThread; - private ConcurrentQueue _scopeSyncItems = new(); + private ConcurrentQueue<(Action action, string actionName)> _scopeSyncItems = new(); private static AndroidJavaObject GetInternalSentryJava() => new AndroidJavaClass("io.sentry.android.core.InternalSentrySdk"); - protected virtual AndroidJavaObject GetSentryJava() => new AndroidJavaClass("io.sentry.Sentry"); + protected static AndroidJavaObject GetSentryJava() => new AndroidJavaClass("io.sentry.Sentry"); public SentryJava(IDiagnosticLogger? logger, IAndroidJNI? androidJNI = null) { @@ -402,7 +401,7 @@ internal void RunJniSafe(Action action, [CallerMemberName] string actionName = " } else { - _scopeSyncItems.Enqueue(action); + _scopeSyncItems.Enqueue((action, actionName)); _scopeSyncEvent.Set(); } } @@ -422,15 +421,16 @@ private void SyncScope() break; } - while (_scopeSyncItems.TryDequeue(out var action)) + while (_scopeSyncItems.TryDequeue(out var workItem)) { + var (action, actionName) = workItem; try { action.Invoke(); } - catch (Exception e) + catch (Exception) { - _logger?.LogError(e, "Failed to sync scope."); + _logger?.LogError("Calling '{0}' failed.", actionName); } } } diff --git a/test/Sentry.Unity.Android.Tests/SentryJavaTests.cs b/test/Sentry.Unity.Android.Tests/SentryJavaTests.cs index 11a1ed871..5736d094e 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,55 @@ 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 - _sut.RunJniSafe(action, isMainThread: false); + // Act - Create instance (should start worker thread and attach) + var sut = new SentryJava(logger, androidJni); - // 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"); + // 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); } 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() { From 4dca98e44c31f8f28a776c36e65f78e28c081727 Mon Sep 17 00:00:00 2001 From: bitsandfoxes Date: Wed, 19 Nov 2025 11:14:54 +0100 Subject: [PATCH 3/5] WorkerThread close cleanup --- src/Sentry.Unity.Android/SentryJava.cs | 58 +++++++++++++------------- 1 file changed, 29 insertions(+), 29 deletions(-) diff --git a/src/Sentry.Unity.Android/SentryJava.cs b/src/Sentry.Unity.Android/SentryJava.cs index 8a329e69c..082e20df1 100644 --- a/src/Sentry.Unity.Android/SentryJava.cs +++ b/src/Sentry.Unity.Android/SentryJava.cs @@ -57,12 +57,12 @@ internal class SentryJava : ISentryJava private readonly CancellationTokenSource _scopeSyncShutdownSource; private readonly AutoResetEvent _scopeSyncEvent; - private Thread? _scopeSyncThread; + private readonly Thread _scopeSyncThread; - private ConcurrentQueue<(Action action, string actionName)> _scopeSyncItems = new(); + private readonly ConcurrentQueue<(Action action, string actionName)> _scopeSyncItems = new(); private static AndroidJavaObject GetInternalSentryJava() => new AndroidJavaClass("io.sentry.android.core.InternalSentrySdk"); - protected static AndroidJavaObject GetSentryJava() => new AndroidJavaClass("io.sentry.Sentry"); + private static AndroidJavaObject GetSentryJava() => new AndroidJavaClass("io.sentry.Sentry"); public SentryJava(IDiagnosticLogger? logger, IAndroidJNI? androidJNI = null) { @@ -211,15 +211,10 @@ public void Init(SentryUnityOptions options) public void Close() { - _scopeSyncShutdownSource?.Cancel(); - if (!_scopeSyncThread?.Join(TimeSpan.FromSeconds(2)) ?? false) - { - _logger?.LogWarning("Worker thread did not exit cleanly within timeout"); - } - - _scopeSyncEvent?.Dispose(); - _scopeSyncShutdownSource?.Dispose(); - + _scopeSyncShutdownSource.Cancel(); + _scopeSyncThread.Join(); + _scopeSyncEvent.Dispose(); + _scopeSyncShutdownSource.Dispose(); if (!MainThreadData.IsMainThread()) { @@ -410,32 +405,37 @@ private void SyncScope() { _androidJNI.AttachCurrentThread(); - var waitHandles = new[] { _scopeSyncEvent, _scopeSyncShutdownSource.Token.WaitHandle }; - - while (true) + try { - var index = WaitHandle.WaitAny(waitHandles); - if (index > 0) - { - // We only care about the _taskEvent - break; - } + var waitHandles = new[] { _scopeSyncEvent, _scopeSyncShutdownSource.Token.WaitHandle }; - while (_scopeSyncItems.TryDequeue(out var workItem)) + while (true) { - var (action, actionName) = workItem; - try + var index = WaitHandle.WaitAny(waitHandles); + if (index > 0) { - action.Invoke(); + // Shutdown requested + break; } - catch (Exception) + + while (_scopeSyncItems.TryDequeue(out var workItem)) { - _logger?.LogError("Calling '{0}' failed.", actionName); + var (action, actionName) = workItem; + try + { + action.Invoke(); + } + catch (Exception) + { + _logger?.LogError("Calling '{0}' failed.", actionName); + } } } } - - _androidJNI.DetachCurrentThread(); + finally + { + _androidJNI.DetachCurrentThread(); + } } } From 6e509c3a0aa2f6a43255b631bcacc10ceb464d88 Mon Sep 17 00:00:00 2001 From: bitsandfoxes Date: Wed, 19 Nov 2025 11:44:55 +0100 Subject: [PATCH 4/5] Close flag --- src/Sentry.Unity.Android/SentryJava.cs | 61 +++++++++++-------- .../SentryJavaTests.cs | 19 ++++++ 2 files changed, 55 insertions(+), 25 deletions(-) diff --git a/src/Sentry.Unity.Android/SentryJava.cs b/src/Sentry.Unity.Android/SentryJava.cs index 082e20df1..eb1e75310 100644 --- a/src/Sentry.Unity.Android/SentryJava.cs +++ b/src/Sentry.Unity.Android/SentryJava.cs @@ -58,8 +58,8 @@ internal class SentryJava : ISentryJava 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"); private static AndroidJavaObject GetSentryJava() => new AndroidJavaClass("io.sentry.Sentry"); @@ -209,30 +209,6 @@ public void Init(SentryUnityOptions options) return null; } - public void Close() - { - _scopeSyncShutdownSource.Cancel(); - _scopeSyncThread.Join(); - _scopeSyncEvent.Dispose(); - _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."); - } - } - public void WriteScope( int? GpuId, string? GpuName, @@ -382,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) { @@ -437,6 +419,35 @@ private void SyncScope() _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."); + } + } } internal static class AndroidJavaObjectExtension diff --git a/test/Sentry.Unity.Android.Tests/SentryJavaTests.cs b/test/Sentry.Unity.Android.Tests/SentryJavaTests.cs index 5736d094e..f4b658e8f 100644 --- a/test/Sentry.Unity.Android.Tests/SentryJavaTests.cs +++ b/test/Sentry.Unity.Android.Tests/SentryJavaTests.cs @@ -133,6 +133,25 @@ public void WorkerThread_AttachesOnCreationAndDetachesOnClose() Assert.That(androidJni.DetachCalled, Is.True); } + [Test] + public void RunJniSafe_AfterClose_SkipsActionAndLogsWarning() + { + // Arrange + var actionExecuted = false; + _sut.Close(); + + // Act + _sut.RunJniSafe(() => actionExecuted = true, "TestAction", isMainThread: false); + + // Assert + 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; } From 444cf3c650efc951c694c8f0f50bde5b71bf64a9 Mon Sep 17 00:00:00 2001 From: bitsandfoxes Date: Wed, 19 Nov 2025 11:57:26 +0100 Subject: [PATCH 5/5] Updated CHANGELOG.md --- CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) 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))