fix: Cleanup AppHangTracking properly when closing SDK (#2671)

When closing the SDK directly after starting it, it could happen that the ANRTracker
didn't start its watchdog thread yet. Canceling the ANRTrackers thread required the
instance of the watchdog thread and didn't work correctly. This is fixed now by using
simple state management in the ANRTracker. While this is an edge case, it can help
with flaky tests, as the test logs showed signs of the ANRTracker running in the
background.

Author: philipphofmann

CHANGELOG.md

@@ -1,5 +1,11 @@
 # Changelog
 
+## Unreleased
+
+### Fixes
+
+- Cleanup AppHangTracking properly when closing SDK (#2671)
+
 ## 8.1.0
 
 ### Features

Sources/Sentry/SentryANRTracker.m

@@ -6,6 +6,13 @@
 
 NS_ASSUME_NONNULL_BEGIN
 
+typedef NS_ENUM(NSInteger, SentryANRTrackerState) {
+    kSentryANRTrackerNotRunning = 1,
+    kSentryANRTrackerRunning,
+    kSentryANRTrackerStarting,
+    kSentryANRTrackerStopping
+};
+
 @interface
 SentryANRTracker ()
 
@@ -16,13 +23,11 @@
 @property (nonatomic, strong) NSMutableSet<id<SentryANRTrackerDelegate>> *listeners;
 @property (nonatomic, assign) NSTimeInterval timeoutInterval;
 
-@property (weak, nonatomic) NSThread *thread;
-
 @end
 
 @implementation SentryANRTracker {
     NSObject *threadLock;
-    BOOL running;
+    SentryANRTrackerState state;
 }
 
 - (instancetype)initWithTimeoutInterval:(NSTimeInterval)timeoutInterval
@@ -37,25 +42,43 @@ - (instancetype)initWithTimeoutInterval:(NSTimeInterval)timeoutInterval
         self.crashWrapper = crashWrapper;
         self.dispatchQueueWrapper = dispatchQueueWrapper;
         self.threadWrapper = threadWrapper;
-        self.listeners = [NSMutableSet new];
+        self.listeners = [NSMutableSet set];
         threadLock = [[NSObject alloc] init];
-        running = NO;
+        state = kSentryANRTrackerNotRunning;
     }
     return self;
 }
 
 - (void)detectANRs
 {
-    NSThread.currentThread.name = @"io.sentry.app-hang-tracker";
-    self.thread = NSThread.currentThread;
+    NSUUID *threadID = [NSUUID UUID];
+
+    @synchronized(threadLock) {
+        [self.threadWrapper threadStarted:threadID];
+
+        if (state != kSentryANRTrackerStarting) {
+            [self.threadWrapper threadFinished:threadID];
+            return;
+        }
+
+        NSThread.currentThread.name = @"io.sentry.app-hang-tracker";
+        state = kSentryANRTrackerRunning;
+    }
 
     __block NSInteger ticksSinceUiUpdate = 0;
     __block BOOL reported = NO;
 
     NSInteger reportThreshold = 5;
     NSTimeInterval sleepInterval = self.timeoutInterval / reportThreshold;
 
-    while (![self.thread isCancelled]) {
+    // Canceling the thread can take up to sleepInterval.
+    while (true) {
+        @synchronized(threadLock) {
+            if (state != kSentryANRTrackerRunning) {
+                break;
+            }
+        }
+
         NSDate *blockDeadline =
             [[self.currentDate date] dateByAddingTimeInterval:self.timeoutInterval];
 
@@ -98,6 +121,11 @@ - (void)detectANRs
             [self ANRDetected];
         }
     }
+
+    @synchronized(threadLock) {
+        state = kSentryANRTrackerNotRunning;
+        [self.threadWrapper threadFinished:threadID];
+    }
 }
 
 - (void)ANRDetected
@@ -129,10 +157,13 @@ - (void)addListener:(id<SentryANRTrackerDelegate>)listener
     @synchronized(self.listeners) {
         [self.listeners addObject:listener];
 
-        if (self.listeners.count > 0 && !running) {
+        if (self.listeners.count > 0 && state == kSentryANRTrackerNotRunning) {
             @synchronized(threadLock) {
-                if (!running) {
-                    [self start];
+                if (state == kSentryANRTrackerNotRunning) {
+                    state = kSentryANRTrackerStarting;
+                    [NSThread detachNewThreadSelector:@selector(detectANRs)
+                                             toTarget:self
+                                           withObject:nil];
                 }
             }
         }
@@ -158,20 +189,11 @@ - (void)clear
     }
 }
 
-- (void)start
-{
-    @synchronized(threadLock) {
-        [NSThread detachNewThreadSelector:@selector(detectANRs) toTarget:self withObject:nil];
-        running = YES;
-    }
-}
-
 - (void)stop
 {
     @synchronized(threadLock) {
         SENTRY_LOG_INFO(@"Stopping ANR detection");
-        [self.thread cancel];
-        running = NO;
+        state = kSentryANRTrackerStopping;
     }
 }
 

Sources/Sentry/SentryThreadWrapper.m

@@ -9,6 +9,16 @@ - (void)sleepForTimeInterval:(NSTimeInterval)timeInterval
     [NSThread sleepForTimeInterval:timeInterval];
 }
 
+- (void)threadStarted:(NSUUID *)threadID;
+{
+    // No op. Only needed for testing.
+}
+
+- (void)threadFinished:(NSUUID *)threadID
+{
+    // No op. Only needed for testing.
+}
+
 @end
 
 NS_ASSUME_NONNULL_END

Sources/Sentry/include/SentryThreadWrapper.h

@@ -9,6 +9,10 @@ NS_ASSUME_NONNULL_BEGIN
 
 - (void)sleepForTimeInterval:(NSTimeInterval)timeInterval;
 
+- (void)threadStarted:(NSUUID *)threadID;
+
+- (void)threadFinished:(NSUUID *)threadID;
+
 @end
 
 NS_ASSUME_NONNULL_END

Tests/SentryTests/Helper/SentryTestThreadWrapper.swift

@@ -1,9 +1,26 @@
 import Foundation
+import XCTest
 
 class SentryTestThreadWrapper: SentryThreadWrapper {
 
+    var threadFinishedExpectation = XCTestExpectation(description: "Thread Finished Expectation")
+    var threads: Set<UUID> = Set()
+    var threadStartedInvocations = Invocations<UUID>()
+    var threadFinishedInvocations = Invocations<UUID>()
+    
     override func sleep(forTimeInterval timeInterval: TimeInterval) {
         // Don't sleep. Do nothing.
     }
 
+    override func threadStarted(_ threadID: UUID) {
+        threadStartedInvocations.record(threadID)
+        threads.insert(threadID)
+    }
+    
+    override func threadFinished(_ threadID: UUID) {
+        threadFinishedInvocations.record(threadID)
+        threads.remove(threadID)
+        threadFinishedExpectation.fulfill()
+    }
+
 }

Tests/SentryTests/Integrations/ANR/SentryANRTrackerTests.swift

@@ -41,13 +41,16 @@ class SentryANRTrackerTests: XCTestCase, SentryANRTrackerDelegate {
     override func tearDown() {
         super.tearDown()
         sut.clear()
+        
+        wait(for: [fixture.threadWrapper.threadFinishedExpectation], timeout: 5)
+        XCTAssertEqual(0, fixture.threadWrapper.threads.count)
     }
 
     func start() {
         sut.addListener(self)
     }
 
-    func testContinousANR_OneReported() {
+    func testContinuousANR_OneReported() {
         fixture.dispatchQueue.blockBeforeMainBlock = {
             self.advanceTime(bySeconds: self.fixture.timeoutInterval)
             return false
@@ -153,6 +156,22 @@ class SentryANRTrackerTests: XCTestCase, SentryANRTrackerDelegate {
 
     }
 
+    func testClearDirectlyAfterStart() {
+        anrDetectedExpectation.isInverted = true
+        
+        let invocations = 10
+        for _ in 0..<invocations {
+            sut.addListener(self)
+            sut.clear()
+        }
+        
+        wait(for: [anrDetectedExpectation, anrStoppedExpectation], timeout: 1)
+        
+        XCTAssertEqual(0, fixture.threadWrapper.threads.count)
+        XCTAssertEqual(1, fixture.threadWrapper.threadStartedInvocations.count)
+        XCTAssertEqual(1, fixture.threadWrapper.threadFinishedInvocations.count)
+    }
+    
     func anrDetected() {
         anrDetectedExpectation.fulfill()
     }