Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Too long flush duration #2370

Merged
merged 11 commits into from
Nov 10, 2022
6 changes: 6 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,11 @@
# Changelog

## Unreleased

### Fixes

- Too long flush duration (#2370)

## 7.30.2

### Fixes
Expand Down
6 changes: 0 additions & 6 deletions Sentry.xcodeproj/xcshareddata/xcschemes/Sentry.xcscheme
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand Down
9 changes: 6 additions & 3 deletions Sources/Sentry/SentryHttpTransport.m
Original file line number Diff line number Diff line change
Expand Up @@ -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.");
Expand All @@ -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) {
Expand Down
44 changes: 25 additions & 19 deletions Tests/SentryTests/Networking/SentryHttpTransportTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand All @@ -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()
Expand All @@ -661,7 +664,8 @@ class SentryHttpTransportTests: XCTestCase {
XCTAssertTrue(sut.flush(fixture.flushTimeout), "Flush should not time out.")
let blockingDuration = getDurationNs(beforeFlush, getAbsoluteTime()).toTimeInterval()

XCTAssertLessThan(blockingDuration, 0.1)
XCTAssertLessThan(blockingDuration, fixture.flushTimeout * 2.2,
"The blocking duration must not exceed the sum of the maximum flush duration.")
}

func testFlush_WhenNoEnvelopes_BlocksAndFinishes() {
Expand All @@ -679,10 +683,10 @@ class SentryHttpTransportTests: XCTestCase {
assertFlushBlocksAndFinishesSuccessfully()
}

func testFlush_CalledMultipleTimes_ImmediatelyReturnsFalse_disabled() {
func testFlush_CalledMultipleTimes_ImmediatelyReturnsFalse() {
CurrentDate.setCurrentDateProvider(DefaultCurrentDateProvider.sharedInstance())

givenCachedEvents()
givenCachedEvents(amount: 30)
fixture.requestManager.responseDelay = fixture.flushTimeout * 2

let allFlushCallsGroup = DispatchGroup()
Expand Down Expand Up @@ -716,16 +720,17 @@ class SentryHttpTransportTests: XCTestCase {
// double-checked lock, should return immediately.

let initiallyInactiveQueue = fixture.queue
let count = 100
for _ in 0..<count {
for _ in 0..<2 {
allFlushCallsGroup.enter()
initiallyInactiveQueue.async {
let beforeFlush = getAbsoluteTime()
let result = self.sut.flush(self.fixture.flushTimeout)
let blockingDuration = getDurationNs(beforeFlush, getAbsoluteTime()).toTimeInterval()

XCTAssertGreaterThan(0.1, blockingDuration, "The flush call should have returned immediately.")
XCTAssertFalse(result)
for _ in 0..<10 {
let beforeFlush = getAbsoluteTime()
let result = self.sut.flush(self.fixture.flushTimeout)
let blockingDuration = getDurationNs(beforeFlush, getAbsoluteTime()).toTimeInterval()

XCTAssertGreaterThan(0.1, blockingDuration, "The flush call should have returned immediately.")
XCTAssertFalse(result)
}

allFlushCallsGroup.leave()
}
Expand Down Expand Up @@ -769,11 +774,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()
}
Expand Down