Merge f38ccfc into f5f8910

getsentry · Nov 10, 2022 · eb6e60b · eb6e60b
2 parents f5f8910 + f38ccfc
commit eb6e60b
Show file tree

Hide file tree

Showing 5 changed files with 95 additions and 84 deletions.
diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml
@@ -59,72 +59,81 @@ jobs:
     strategy:
       fail-fast: false
       matrix:
-        # Can't run tests on watchOS because XCTest is not available
-        # We can't use Xcode 11.7 as we use XCTestObservation. When building with Xcode 11.7
-        # we get the error 'XCTest/XCTest.h' not found. Setting ENABLE_TESTING_SEARCH_PATH=YES
-        # doesn't work.
-        include:
-          # iOS 12.4
-          - runs-on: macos-11
-            platform: 'iOS'
-            xcode: '13.2.1'
-            test-destination-os: '12.4'
-
-          # iOS 13.7
-          - runs-on: macos-11
-            platform: 'iOS'
-            xcode: '13.2.1'
-            test-destination-os: '13.7'
-
-          # iOS 14
-          - runs-on: macos-11
-            platform: 'iOS'
-            xcode: '12.5.1'
-            test-destination-os: 'latest'
-
-          # iOS 15
-          - runs-on: macos-12
-            platform: 'iOS'
-            xcode: '13.4.1'
-            test-destination-os: 'latest'
-
-          # iOS 16
-          - runs-on: macos-12
-            platform: 'iOS'
-            xcode: '14.0'
-            test-destination-os: 'latest'
-
-          # macOS 11
-          - runs-on: macos-11
-            platform: 'macOS'
-            xcode: '12.5.1'
-            test-destination-os: 'latest'
-
-          # macOS 12
-          - runs-on: macos-12
-            platform: 'macOS'
-            xcode: '13.4.1'
-            test-destination-os: 'latest'
-
-          # Catalyst. We only test the latest version, as
-          # the risk something breaking on Catalyst and not
-          # on an older iOS or macOS version is low.
-          - runs-on: macos-12
-            platform: 'Catalyst'
-            xcode: '13.4.1'
-            test-destination-os: 'latest'
-
-          # tvOS 14
-          - runs-on: macos-11
-            platform: 'tvOS'
-            xcode: '12.5.1'
-            test-destination-os: 'latest'
-
-          # tvOS 15
-          - runs-on: macos-12
-            platform: 'tvOS'
-            xcode: '13.4.1'
-            test-destination-os: 'latest'
+        # Dummy matrix for testing.
+        # TODO: Remove before merging
+        a: [1,2,3,4]
+        b: [1,2,3,4]
+        runs-on: ['macos-12']
+        platform: ['iOS']
+        xcode: ['14.0', '13.4.1']
+        test-destination-os: ['latest']
+
+        # # Can't run tests on watchOS because XCTest is not available
+        # # We can't use Xcode 11.7 as we use XCTestObservation. When building with Xcode 11.7
+        # # we get the error 'XCTest/XCTest.h' not found. Setting ENABLE_TESTING_SEARCH_PATH=YES
+        # # doesn't work.
+        # include:
+        #   # iOS 12.4
+        #   - runs-on: macos-11
+        #     platform: 'iOS'
+        #     xcode: '13.2.1'
+        #     test-destination-os: '12.4'
+
+        #   # iOS 13.7
+        #   - runs-on: macos-11
+        #     platform: 'iOS'
+        #     xcode: '13.2.1'
+        #     test-destination-os: '13.7'
+
+        #   # iOS 14
+        #   - runs-on: macos-11
+        #     platform: 'iOS'
+        #     xcode: '12.5.1'
+        #     test-destination-os: 'latest'
+
+        #   # iOS 15
+        #   - runs-on: macos-12
+        #     platform: 'iOS'
+        #     xcode: '13.4.1'
+        #     test-destination-os: 'latest'
+
+        #   # iOS 16
+        #   - runs-on: macos-12
+        #     platform: 'iOS'
+        #     xcode: '14.0'
+        #     test-destination-os: 'latest'
+
+        #   # macOS 11
+        #   - runs-on: macos-11
+        #     platform: 'macOS'
+        #     xcode: '12.5.1'
+        #     test-destination-os: 'latest'
+
+        #   # macOS 12
+        #   - runs-on: macos-12
+        #     platform: 'macOS'
+        #     xcode: '13.4.1'
+        #     test-destination-os: 'latest'
+
+        #   # Catalyst. We only test the latest version, as
+        #   # the risk something breaking on Catalyst and not
+        #   # on an older iOS or macOS version is low.
+        #   - runs-on: macos-12
+        #     platform: 'Catalyst'
+        #     xcode: '13.4.1'
+        #     test-destination-os: 'latest'
+
+        #   # tvOS 14
+        #   - runs-on: macos-11
+        #     platform: 'tvOS'
+        #     xcode: '12.5.1'
+        #     test-destination-os: 'latest'
+
+        #   # tvOS 15
+        #   - runs-on: macos-12
+        #     platform: 'tvOS'
+        #     xcode: '13.4.1'
+        #     test-destination-os: 'latest'
 
     steps:
       - uses: actions/checkout@v3

diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -4,6 +4,7 @@
 
 ### Fixes
 
+- Too long flush duration (#2370)
 - Fix issue with invalid profiles uploading (#2358 and #2359)
 - Call UIDevice methods on the main thread (#2369)
 - Avoid sending profiles with 0 samples or incorrectly deduplicated backtrace elements (#2375)

diff --git a/Sentry.xcodeproj/xcshareddata/xcschemes/Sentry.xcscheme b/Sentry.xcodeproj/xcshareddata/xcschemes/Sentry.xcscheme
@@ -64,12 +64,6 @@
                <Test
                   Identifier = "SentryFileIOTrackingIntegrationTests/test_DataConsistency_readUrl_disabled()">
                </Test>
-               <Test
-                  Identifier = "SentryHttpTransportTests/testFlush_CalledMultipleTimes_ImmediatelyReturnsFalse_disabled()">
-               </Test>
-               <Test
-                  Identifier = "SentryHttpTransportTests/testFlush_CalledSequentially_BlocksTwice_disabled()">
-               </Test>
                <Test
                   Identifier = "SentryNetworkTrackerIntegrationTests/testGetRequest_SpanCreatedAndBaggageHeaderAdded_disabled()">
                </Test>

diff --git a/Sources/Sentry/SentryHttpTransport.m b/Sources/Sentry/SentryHttpTransport.m
@@ -151,6 +151,12 @@ - (void)recordLostEvent:(SentryDataCategory)category reason:(SentryDiscardReason
 
 - (BOOL)flush:(NSTimeInterval)timeout
 {
+    // Calculate the dispatch time of the flush duration as early as possible to guarantee an exact
+    // flush duration. Any code up to the dispatch_group_wait can take a couple of ms, adding up to
+    // the flush duration.
+    dispatch_time_t delta = (int64_t)(timeout * (NSTimeInterval)NSEC_PER_SEC);
+    dispatch_time_t dispatchTimeout = dispatch_time(DISPATCH_TIME_NOW, delta);
+
     // Double-Checked Locking to avoid acquiring unnecessary locks.
     if (_isFlushing) {
         SENTRY_LOG_DEBUG(@"Already flushing.");
@@ -171,9 +177,6 @@ - (BOOL)flush:(NSTimeInterval)timeout
 
     [self sendAllCachedEnvelopes];
 
-    dispatch_time_t delta = (int64_t)(timeout * (NSTimeInterval)NSEC_PER_SEC);
-    dispatch_time_t dispatchTimeout = dispatch_time(DISPATCH_TIME_NOW, delta);
-
     intptr_t result = dispatch_group_wait(self.dispatchGroup, dispatchTimeout);
 
     @synchronized(self) {

diff --git a/Tests/SentryTests/Networking/SentryHttpTransportTests.swift b/Tests/SentryTests/Networking/SentryHttpTransportTests.swift
@@ -629,7 +629,7 @@ class SentryHttpTransportTests: XCTestCase {
     func testFlush_BlocksCallingThread_TimesOut() {
         CurrentDate.setCurrentDateProvider(DefaultCurrentDateProvider.sharedInstance())
 
-        givenCachedEvents()
+        givenCachedEvents(amount: 30)
 
         fixture.requestManager.responseDelay = fixture.flushTimeout + 0.2
 
@@ -638,20 +638,23 @@ class SentryHttpTransportTests: XCTestCase {
         let blockingDuration = getDurationNs(beforeFlush, getAbsoluteTime()).toTimeInterval()
 
         XCTAssertGreaterThan(blockingDuration, fixture.flushTimeout)
-        XCTAssertLessThan(blockingDuration, fixture.flushTimeout + 0.2)
+        XCTAssertLessThan(blockingDuration, fixture.flushTimeout + 0.1)
 
         XCTAssertFalse(success, "Flush should time out.")
     }
 
     func testFlush_BlocksCallingThread_FinishesFlushingWhenSent() {
         CurrentDate.setCurrentDateProvider(DefaultCurrentDateProvider.sharedInstance())
 
-        givenCachedEvents()
+        givenCachedEvents(amount: 1)
 
-        assertFlushBlocksAndFinishesSuccessfully()
+        let beforeFlush = getAbsoluteTime()
+        XCTAssertTrue(sut.flush(fixture.flushTimeout), "Flush should not time out.")
+        let blockingDuration = getDurationNs(beforeFlush, getAbsoluteTime()).toTimeInterval()
+        XCTAssertLessThan(blockingDuration, fixture.flushTimeout)
     }
 
-    func testFlush_CalledSequentially_BlocksTwice_disabled() {
+    func testFlush_CalledSequentially_BlocksTwice() {
         CurrentDate.setCurrentDateProvider(DefaultCurrentDateProvider.sharedInstance())
 
         givenCachedEvents()
@@ -679,7 +682,7 @@ class SentryHttpTransportTests: XCTestCase {
         assertFlushBlocksAndFinishesSuccessfully()
     }
 
-    func testFlush_CalledMultipleTimes_ImmediatelyReturnsFalse_disabled() {
+    func testFlush_CalledMultipleTimes_ImmediatelyReturnsFalse() {
         CurrentDate.setCurrentDateProvider(DefaultCurrentDateProvider.sharedInstance())
 
         givenCachedEvents()
@@ -769,11 +772,12 @@ class SentryHttpTransportTests: XCTestCase {
         fixture.requestManager.returnResponse(response: HTTPURLResponse())
     }
 
-    private func givenCachedEvents() {
+    private func givenCachedEvents(amount: Int = 2) {
         givenNoInternetConnection()
 
-        sendEvent()
-        sendEvent()
+        for _ in 0..<amount {
+            sendEvent()
+        }
 
         givenOkResponse()
     }