diff --git a/Source/ARTAttachRetryState.m b/Source/ARTAttachRetryState.m index 827f88533..7afecc300 100644 --- a/Source/ARTAttachRetryState.m +++ b/Source/ARTAttachRetryState.m @@ -31,9 +31,10 @@ - (instancetype)initWithRetryDelayCalculator:(id)retryD } - (ARTRetryAttempt *)addRetryAttempt { - // Note: we currently reset the retry sequence every time we wish to perform a retry (defeating the point of using it in the first place, but it's OK since the delays are all constant). As part of implementing #1431 we will reuse any existing retry sequence, resetting it only in response to certain state changes. - self.retrySequence = [[ARTRetrySequence alloc] initWithDelayCalculator:self.retryDelayCalculator]; - ARTLogDebug(self.logger, @"%@Created attach retry sequence %@", self.logMessagePrefix, self.retrySequence); + if (!self.retrySequence) { + self.retrySequence = [[ARTRetrySequence alloc] initWithDelayCalculator:self.retryDelayCalculator]; + ARTLogDebug(self.logger, @"%@Created attach retry sequence %@", self.logMessagePrefix, self.retrySequence); + } ARTRetryAttempt *const retryAttempt = [self.retrySequence addRetryAttempt]; ARTLogDebug(self.logger, @"%@Adding attach retry attempt to %@ gave %@", self.logMessagePrefix, self.retrySequence.id, retryAttempt); @@ -41,4 +42,11 @@ - (ARTRetryAttempt *)addRetryAttempt { return retryAttempt; } +- (void)channelWillTransitionToState:(ARTRealtimeChannelState)state { + // The client library specification doesn’t specify when to reset the retry count; have taken the logic from ably-js: https://github.com/ably/ably-js/blob/404c4128316cc5f735e3bf95a25e654e3fedd166/src/common/lib/client/realtimechannel.ts#L804-L806 (see discussion https://github.com/ably/ably-js/pull/1008/files#r925898316) + if (state != ARTRealtimeChannelAttaching && state != ARTRealtimeChannelSuspended) { + self.retrySequence = nil; + } +} + @end diff --git a/Source/ARTRealtimeChannel.m b/Source/ARTRealtimeChannel.m index 5166234c3..6aa50ce74 100644 --- a/Source/ARTRealtimeChannel.m +++ b/Source/ARTRealtimeChannel.m @@ -30,7 +30,7 @@ #import "ARTChannelStateChangeMetadata.h" #import "ARTAttachRequestMetadata.h" #import "ARTRetrySequence.h" -#import "ARTConstantRetryDelayCalculator.h" +#import "ARTBackoffRetryDelayCalculator.h" #import "ARTInternalLog.h" #import "ARTAttachRetryState.h" #if TARGET_OS_IPHONE @@ -269,7 +269,8 @@ - (instancetype)initWithRealtime:(ARTRealtimeInternal *)realtime andName:(NSStri _attachedEventEmitter = [[ARTInternalEventEmitter alloc] initWithQueue:_queue]; _detachedEventEmitter = [[ARTInternalEventEmitter alloc] initWithQueue:_queue]; _internalEventEmitter = [[ARTInternalEventEmitter alloc] initWithQueue:_queue]; - const id attachRetryDelayCalculator = [[ARTConstantRetryDelayCalculator alloc] initWithConstantDelay:realtime.options.channelRetryTimeout]; + const id attachRetryDelayCalculator = [[ARTBackoffRetryDelayCalculator alloc] initWithInitialRetryTimeout:realtime.options.channelRetryTimeout + jitterCoefficientGenerator:realtime.options.testOptions.jitterCoefficientGenerator]; _attachRetryState = [[ARTAttachRetryState alloc] initWithRetryDelayCalculator:attachRetryDelayCalculator logger:logger logMessagePrefix:[NSString stringWithFormat:@"RT: %p C:%p ", _realtime, self]]; @@ -611,6 +612,8 @@ - (void)transition:(ARTRealtimeChannelState)state withMetadata:(ARTChannelStateC _errorReason = metadata.errorInfo; } + [self.attachRetryState channelWillTransitionToState:state]; + ARTEventListener *channelRetryListener = nil; switch (state) { case ARTRealtimeChannelAttached: diff --git a/Source/PrivateHeaders/Ably/ARTAttachRetryState.h b/Source/PrivateHeaders/Ably/ARTAttachRetryState.h index ff41a406f..c5ac9957a 100644 --- a/Source/PrivateHeaders/Ably/ARTAttachRetryState.h +++ b/Source/PrivateHeaders/Ably/ARTAttachRetryState.h @@ -1,4 +1,5 @@ @import Foundation; +#import "ARTTypes.h" @class ARTRetryAttempt; @class ARTInternalLog; @@ -22,6 +23,11 @@ NS_SWIFT_NAME(AttachRetryState) */ - (ARTRetryAttempt *)addRetryAttempt; +/** + Resets the retry sequence when the channel leaves the sequence of `SUSPENDED` <-> `ATTACHING` state changes. + */ +- (void)channelWillTransitionToState:(ARTRealtimeChannelState)state; + @end NS_ASSUME_NONNULL_END diff --git a/Test/Tests/AttachRetryStateTests.swift b/Test/Tests/AttachRetryStateTests.swift index ad27608d7..30411310a 100644 --- a/Test/Tests/AttachRetryStateTests.swift +++ b/Test/Tests/AttachRetryStateTests.swift @@ -21,4 +21,41 @@ class AttachRetryStateTests: XCTestCase { XCTAssertEqual(secondRetryAttempt.delay, delays[1]) XCTAssertEqual(thirdRetryAttempt.delay, delays[2]) } + + func test_transitionToNonAttachedOrSuspendedStateResetsRetrySequence() { + // Given: an AttachRetryState initialized with a delay calculator that returns arbitrary values from its `delay(forRetryNumber:)` method... + let delays: [TimeInterval] = [0.1, 0.3, 0.9] + let calculator = MockRetryDelayCalculator(delays: delays) + + let retryState = AttachRetryState(retryDelayCalculator: calculator, + logger: .init(core: MockInternalLogCore()), + logMessagePrefix: "") + + // When: addRetryAttempt is called an arbitrarily-chosen number n ( = 3) times on the attach retry state, and then channelWillTransition(to:) is called on the attach retry state with a channel state that is not SUSPENDED or ATTACHING (arbitrarily chosen ATTACHED here), and addRetryAttempt is then called again on the attach retry state... + (0..<3).forEach { _ in let _ = retryState.addRetryAttempt() } + retryState.channelWillTransition(to: .attached) + let retryAttempt = retryState.addRetryAttempt() + + // Then: the `delay` property of the post-channelWillTransition(to:) returned RetryAttempt object matches that returned by the calculator’s `delay(forRetryNumber: 1)` method. + XCTAssertEqual(retryAttempt.delay, delays[0]) + } + + func test_transitionToAttachedOrSuspendedStateDoesNotResetRetrySequence() { + // Given: an AttachRetryState initialized with a delay calculator that returns arbitrary values from its `delay(forRetryNumber:)` method... + let delays: [TimeInterval] = [0.1, 0.3, 0.9, 0.7] + let calculator = MockRetryDelayCalculator(delays: delays) + + let retryState = AttachRetryState(retryDelayCalculator: calculator, + logger: .init(core: MockInternalLogCore()), + logMessagePrefix: "") + + // When: addRetryAttempt is called an arbitrarily-chosen number n ( = 3) times on the attach retry state, and then channelWillTransition(to:) is called on the attach retry state, once with channel state SUSPENDED and once with channel state ATTACHING, and addRetryAttempt is then called again on the attach retry state... + (0..<3).forEach { _ in let _ = retryState.addRetryAttempt() } + retryState.channelWillTransition(to: .suspended) + retryState.channelWillTransition(to: .attaching) + let retryAttempt = retryState.addRetryAttempt() + + // Then: the `delay` property of the post-channelWillTransition(to:) returned RetryAttempt object matches that returned by the calculator’s `delay(forRetryNumber: n + 1)` method. + XCTAssertEqual(retryAttempt.delay, delays[3]) + } } diff --git a/Test/Tests/RealtimeClientChannelTests.swift b/Test/Tests/RealtimeClientChannelTests.swift index e9506c16d..ead035d50 100644 --- a/Test/Tests/RealtimeClientChannelTests.swift +++ b/Test/Tests/RealtimeClientChannelTests.swift @@ -4015,13 +4015,18 @@ class RealtimeClientChannelTests: XCTestCase { ## Motivation for chosen channelRetryTimeout value - We expect the retries in this sequence to be spaced apart by channelRetryTimeout seconds. The default value is 15 seconds, so as above, in order to avoid a very long test execution time we reduce it to 1 second. + We expect the retries in this sequence to be spaced apart by values in the range of [channelRequestTimeout seconds, 2 * channelRequestTimeout seconds]. The default value of channelRequestTimeout is 15 seconds, so as above, in order to avoid a very long test execution time we reduce it to 1 second. */ let options = try AblyTests.commonAppSetup(for: test) options.channelRetryTimeout = 1.0 options.autoConnect = false options.testOptions.realtimeRequestTimeout = 1.0 options.testOptions.transportFactory = TestProxyTransportFactory() + + let jitterCoefficients = StaticJitterCoefficients() + let mockJitterCoefficientGenerator = MockJitterCoefficientGenerator(coefficients: jitterCoefficients) + options.testOptions.jitterCoefficientGenerator = mockJitterCoefficientGenerator + let client = ARTRealtime(options: options) client.connect() defer { client.dispose(); client.close() } @@ -4078,11 +4083,17 @@ class RealtimeClientChannelTests: XCTestCase { // Then... - let timeout = Double(numberOfRetriesToWaitFor) * ( + let expectedRetryDelays = Array( + AblyTests.expectedRetryDelays( + forTimeout: options.channelRetryTimeout, + jitterCoefficients: jitterCoefficients + ).prefix(numberOfRetriesToWaitFor) + ) + let timeout = expectedRetryDelays.map { retryDelay in options.testOptions.realtimeRequestTimeout // waiting for attach to time out - + options.channelRetryTimeout // waiting for retry to occur + + retryDelay // waiting for retry to occur + 0.2 // some extra tolerance, arbitrarily chosen - ) + }.reduce(0, +) let observedStateChanges = try retrySequenceDataGatherer.waitForData(timeout: timeout) let expectedNumberOfObservedStateChanges = 1 + 2 * numberOfRetriesToWaitFor @@ -4103,9 +4114,9 @@ class RealtimeClientChannelTests: XCTestCase { for retryNumber in 1 ... numberOfRetriesToWaitFor { let observedStateChangesStartIndexForThisRetry = 1 + 2 * (retryNumber - 1) - let expectedRetryDelay = options.channelRetryTimeout + let expectedRetryDelay = expectedRetryDelays[retryNumber - 1] - // after channelRetryTimeout seconds (as described by the retry metadata attached to the channel state change, and as approximately measured), the channel emits a state change to the ATTACHING state... + // after a delay (as described by the retry metadata attached to the channel state change, and as approximately measured) matching that defined by RTB1 (with initial retry timeout of channelRetryTimeout), the channel emits a state change to the ATTACHING state... let firstObservedStateChange = observedStateChanges[observedStateChangesStartIndexForThisRetry] XCTAssertEqual(firstObservedStateChange.stateChange.previous, .suspended) XCTAssertEqual(firstObservedStateChange.stateChange.current, .attaching)