fix: Replace ThreadPool with WorkerThread for background scope sync on Android

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))

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.");
         }
     }
 }

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,33 +32,30 @@ 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();
         });
 
         // Act
         _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()
         {