fix: Remove delay for deleting old envelopes (#2541)

The SentryFileManager deleted old envelopes after 10 seconds, which resulted in accessing double-freed pointers. We delayed the deletion for 10 seconds to avoid additional operations when starting the SDK. As the overhead of deleting the old envelope items should be minimal, we remove the delay because this is simpler than not running the delayed deletion for tests and reduces complexity.
getsentry · Dec 20, 2022 · 17e2418 · 17e2418
1 parent 1018de3
commit 17e2418
Show file tree

Hide file tree

Showing 5 changed files with 23 additions and 19 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -25,6 +25,7 @@ This version adds a dependency on Swift.
 - Use the preexisting app release version format for profiles (#2470)
 - Don't add out of date context for crashes (#2523)
 - Fix ARC issue for FileManager (#2525)
+- Remove delay for deleting old envelopes (#2541)
 
 ### Breaking Changes
 

diff --git a/Sources/Sentry/SentryFileManager.m b/Sources/Sentry/SentryFileManager.m
@@ -60,7 +60,6 @@ - (nullable instancetype)initWithOptions:(SentryOptions *)options
     self = [super init];
     if (self) {
         self.currentDateProvider = currentDateProvider;
-
         [self createPathsWithOptions:options];
 
         // Remove old cached events for versions before 6.0.0
@@ -78,15 +77,13 @@ - (nullable instancetype)initWithOptions:(SentryOptions *)options
         self.maxEnvelopes = options.maxCacheItems;
 
         __weak SentryFileManager *weakSelf = self;
-        [dispatchQueueWrapper
-            dispatchAfter:10
-                    block:^{
-                        if (weakSelf == nil) {
-                            return;
-                        }
-                        SENTRY_LOG_DEBUG(@"Dispatched deletion of old envelopes from %@", weakSelf);
-                        [weakSelf deleteOldEnvelopesFromAllSentryPaths];
-                    }];
+        [dispatchQueueWrapper dispatchAsyncWithBlock:^{
+            if (weakSelf == nil) {
+                return;
+            }
+            SENTRY_LOG_DEBUG(@"Dispatched deletion of old envelopes from %@", weakSelf);
+            [weakSelf deleteOldEnvelopesFromAllSentryPaths];
+        }];
     }
     return self;
 }

diff --git a/Tests/SentryTests/Helper/SentryFileManagerTests.swift b/Tests/SentryTests/Helper/SentryFileManagerTests.swift
@@ -134,15 +134,12 @@ class SentryFileManagerTests: XCTestCase {
     }
 
     func testDeleteOldEnvelopes() throws {
-        fixture.dispatchQueueWrapper.dispatchAfterExecutesBlock = true
 
         try givenOldEnvelopes()
 
         sut = fixture.getSut()
 
         XCTAssertEqual(sut.getAllEnvelopes().count, 0)
-
-        fixture.dispatchQueueWrapper.dispatchAfterExecutesBlock = false
     }
 
     func testDontDeleteYoungEnvelopes() throws {
@@ -178,15 +175,15 @@ class SentryFileManagerTests: XCTestCase {
     func testFileManagerDeallocated_OldEnvelopesNotDeleted() throws {
         try givenOldEnvelopes()
 
-        fixture.dispatchQueueWrapper.dispatchAfterExecutesBlock = false
+        fixture.dispatchQueueWrapper.dispatchAsyncExecutesBlock = false
 
         // Initialize sut in extra function so ARC deallocates it
         func getSut() {
             _ = fixture.getSut()
         }
         getSut()
 
-        fixture.dispatchQueueWrapper.invokeLastDispatchAfter()
+        fixture.dispatchQueueWrapper.invokeLastDispatchAsync()
 
         XCTAssertEqual(sut.getAllEnvelopes().count, 1)
     }

diff --git a/...entryTests/Integrations/WatchdogTerminations/SentryWatchdogTerminationsTrackerTests.swift b/...entryTests/Integrations/WatchdogTerminations/SentryWatchdogTerminationsTrackerTests.swift
@@ -91,7 +91,7 @@ class SentryWatchdogTerminationTrackerTests: NotificationCenterTestCase {
         let appState = SentryAppState(releaseName: fixture.options.releaseName ?? "", osVersion: UIDevice.current.systemVersion, vendorId: TestData.someUUID, isDebugging: false, systemBootTimestamp: fixture.sysctl.systemBootTimestamp)
 
         XCTAssertEqual(appState, actual)
-        XCTAssertEqual(1, fixture.dispatchQueue.dispatchAsyncCalled)
+        XCTAssertEqual(2, fixture.dispatchQueue.dispatchAsyncCalled)
     }
 
     func testGoToForeground_SetsIsActive() {
@@ -106,7 +106,7 @@ class SentryWatchdogTerminationTrackerTests: NotificationCenterTestCase {
         goToBackground()
 
         XCTAssertFalse(fixture.realFileManager.readAppState()?.isActive ?? true)
-        XCTAssertEqual(3, fixture.dispatchQueue.dispatchAsyncCalled)
+        XCTAssertEqual(4, fixture.dispatchQueue.dispatchAsyncCalled)
     }
 
     func testGoToForeground_WhenAppStateNil_NothingIsStored() {

diff --git a/Tests/SentryTests/Networking/TestSentryDispatchQueueWrapper.swift b/Tests/SentryTests/Networking/TestSentryDispatchQueueWrapper.swift
@@ -9,11 +9,20 @@ class TestSentryDispatchQueueWrapper: SentryDispatchQueueWrapper {
     /// - SeeAlso: `delayDispatches`, which controls whether the block should execute immediately or with the requested delay.
     var dispatchAfterExecutesBlock = false
 
+    var dispatchAsyncInvocations = Invocations<() -> Void>()
+    var dispatchAsyncExecutesBlock = true
     override func dispatchAsync(_ block: @escaping () -> Void) {
         dispatchAsyncCalled += 1
-        block()
+        dispatchAsyncInvocations.record(block)
+        if dispatchAsyncExecutesBlock {
+            block()
+        }
     }
-
+
+    func invokeLastDispatchAsync() {
+        dispatchAsyncInvocations.invocations.last?()
+    }
+
     var blockOnMainInvocations = Invocations<() -> Void>()
     var blockBeforeMainBlock: () -> Bool = { true }