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.
getsentry · Feb 6, 2023 · 075a044 · 075a044
1 parent ddc9b9a
commit 075a044
Show file tree

Hide file tree

Showing 6 changed files with 100 additions and 22 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -1,5 +1,11 @@
 # Changelog
 
+## Unreleased
+
+### Fixes
+
+- Cleanup AppHangTracking properly when closing SDK (#2671)
+
 ## 8.1.0
 
 ### Features

diff --git a/Sources/Sentry/SentryANRTracker.m b/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;
     }
 }
 

diff --git a/Sources/Sentry/SentryThreadWrapper.m b/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
diff --git a/Sources/Sentry/include/SentryThreadWrapper.h b/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
diff --git a/Tests/SentryTests/Helper/SentryTestThreadWrapper.swift b/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()
+    }
+
 }
diff --git a/Tests/SentryTests/Integrations/ANR/SentryANRTrackerTests.swift b/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()
     }