From aaf6b69d42621fd51114313e6d457611e13054f2 Mon Sep 17 00:00:00 2001 From: Lawrence Forooghian Date: Fri, 3 Nov 2023 14:50:40 -0300 Subject: [PATCH 1/6] Make creation of ARTRealtimeTransportError a bit more readable --- Source/ARTWebSocketTransport.m | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/Source/ARTWebSocketTransport.m b/Source/ARTWebSocketTransport.m index 3ac74e466..f0394e87f 100644 --- a/Source/ARTWebSocketTransport.m +++ b/Source/ARTWebSocketTransport.m @@ -282,9 +282,14 @@ - (void)webSocket:(id)webSocket didCloseWithCode:(NSInteger)code r [_delegate realtimeTransportDisconnected:self withError:nil]; break; case ARTWsRefuse: - case ARTWsPolicyValidation: - [_delegate realtimeTransportRefused:self withError:[[ARTRealtimeTransportError alloc] initWithError:[ARTErrorInfo createWithCode:code message:reason] type:ARTRealtimeTransportErrorTypeRefused url:self.websocketURL]]; + case ARTWsPolicyValidation: { + ARTErrorInfo *const errorInfo = [ARTErrorInfo createWithCode:code message:reason]; + ARTRealtimeTransportError *const error = [[ARTRealtimeTransportError alloc] initWithError:errorInfo + type:ARTRealtimeTransportErrorTypeRefused + url:self.websocketURL]; + [_delegate realtimeTransportRefused:self withError:error]; break; + } case ARTWsTooBig: [_delegate realtimeTransportTooBig:self]; break; @@ -292,10 +297,15 @@ - (void)webSocket:(id)webSocket didCloseWithCode:(NSInteger)code r case ARTWsCloseProtocolError: case ARTWsUnexpectedCondition: case ARTWsExtension: - case ARTWsTlsError: + case ARTWsTlsError: { // Failed - [_delegate realtimeTransportFailed:self withError:[[ARTRealtimeTransportError alloc] initWithError:[ARTErrorInfo createWithCode:code message:reason] type:ARTRealtimeTransportErrorTypeOther url:self.websocketURL]]; + ARTErrorInfo *const errorInfo = [ARTErrorInfo createWithCode:code message:reason]; + ARTRealtimeTransportError *const error = [[ARTRealtimeTransportError alloc] initWithError:errorInfo + type:ARTRealtimeTransportErrorTypeOther + url:self.websocketURL]; + [_delegate realtimeTransportFailed:self withError:error]; break; + } default: NSAssert(true, @"WebSocket close: unknown code"); break; From 3c0875a5add89ded160001312fd1a164017ee107 Mon Sep 17 00:00:00 2001 From: Lawrence Forooghian Date: Fri, 3 Nov 2023 14:55:47 -0300 Subject: [PATCH 2/6] Document validity of ARTRealtimeTransportError badResponseCode --- Source/PrivateHeaders/Ably/ARTRealtimeTransport.h | 3 +++ 1 file changed, 3 insertions(+) diff --git a/Source/PrivateHeaders/Ably/ARTRealtimeTransport.h b/Source/PrivateHeaders/Ably/ARTRealtimeTransport.h index 46d02d544..96dc62310 100644 --- a/Source/PrivateHeaders/Ably/ARTRealtimeTransport.h +++ b/Source/PrivateHeaders/Ably/ARTRealtimeTransport.h @@ -32,6 +32,9 @@ typedef NS_ENUM(NSUInteger, ARTRealtimeTransportState) { @property (nonatomic) NSError *error; @property (nonatomic) ARTRealtimeTransportErrorType type; +/** + This meaning of this property is only defined if the error is of type `ARTRealtimeTransportErrorTypeBadResponse`. + */ @property (nonatomic) NSInteger badResponseCode; @property (nonatomic) NSURL *url; From e3604ea7b24beb11aa08f460038c4de745ecc906 Mon Sep 17 00:00:00 2001 From: Lawrence Forooghian Date: Mon, 6 Nov 2023 09:34:23 -0300 Subject: [PATCH 3/6] Explain approach of fallback host test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit I’m going to need to change this test in an upcoming commit and want the original intent to be clear before I do. --- Test/Tests/RealtimeClientConnectionTests.swift | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/Test/Tests/RealtimeClientConnectionTests.swift b/Test/Tests/RealtimeClientConnectionTests.swift index 734aa8845..ad6e39514 100644 --- a/Test/Tests/RealtimeClientConnectionTests.swift +++ b/Test/Tests/RealtimeClientConnectionTests.swift @@ -3708,12 +3708,16 @@ class RealtimeClientConnectionTests: XCTestCase { client.connect() defer { client.dispose(); client.close() } + // We expect the first connection attempt to fail due to the .fakeNetworkResponse configured above. This error does not meet the criteria for trying a fallback host, and so should not provoke the use of a fallback host. Hence the connection should give up and transition to the FAILED state (which causes the publish to fail). We should see that there was only one connection attempt, to the primary host. + waitUntil(timeout: testTimeout) { done in - channel.publish(nil, data: "message") { _ in + channel.publish(nil, data: "message") { error in + XCTAssertNotNil(error) done() } } + XCTAssertEqual(client.connection.state, .failed) XCTAssertEqual(urlConnections.count, 1) XCTAssertTrue(NSRegularExpression.match(urlConnections[0].absoluteString, pattern: "//realtime.ably.io")) } From 0f1a91292109b744b0925e85ca3d70e974029b53 Mon Sep 17 00:00:00 2001 From: Lawrence Forooghian Date: Mon, 6 Nov 2023 11:52:10 -0300 Subject: [PATCH 4/6] Remove use of non-routable IP address in RTN14d test The use of the non-completing authCallback already ensures a timeout. --- Test/Tests/RealtimeClientConnectionTests.swift | 1 - 1 file changed, 1 deletion(-) diff --git a/Test/Tests/RealtimeClientConnectionTests.swift b/Test/Tests/RealtimeClientConnectionTests.swift index ad6e39514..bf7c1c632 100644 --- a/Test/Tests/RealtimeClientConnectionTests.swift +++ b/Test/Tests/RealtimeClientConnectionTests.swift @@ -2262,7 +2262,6 @@ class RealtimeClientConnectionTests: XCTestCase { func test__059__Connection__connection_request_fails__connection_attempt_fails_for_any_recoverable_reason() throws { let test = Test() let options = try AblyTests.commonAppSetup(for: test) - options.realtimeHost = "10.255.255.1" // non-routable IP address options.disconnectedRetryTimeout = 1.0 options.autoConnect = false options.testOptions.realtimeRequestTimeout = 0.1 From 30a09795b97948d41b0959ee37428955171c77b4 Mon Sep 17 00:00:00 2001 From: Lawrence Forooghian Date: Mon, 6 Nov 2023 12:00:55 -0300 Subject: [PATCH 5/6] Refactor RTN14d test to allow configuring failure reason Want to add a new test with another failure reason. --- .../Tests/RealtimeClientConnectionTests.swift | 37 ++++++++++++++----- 1 file changed, 28 insertions(+), 9 deletions(-) diff --git a/Test/Tests/RealtimeClientConnectionTests.swift b/Test/Tests/RealtimeClientConnectionTests.swift index bf7c1c632..4524c05e1 100644 --- a/Test/Tests/RealtimeClientConnectionTests.swift +++ b/Test/Tests/RealtimeClientConnectionTests.swift @@ -2258,21 +2258,21 @@ class RealtimeClientConnectionTests: XCTestCase { } } - // RTN14d, RTB1 - func test__059__Connection__connection_request_fails__connection_attempt_fails_for_any_recoverable_reason() throws { - let test = Test() + func testRTN14dAndRTB1( + test: Test, + extraTimeNeededToObserveEachRetry: TimeInterval = 0, + modifyOptions: (ARTClientOptions) -> Void, + checkError: (ARTErrorInfo) -> Void + ) throws { let options = try AblyTests.commonAppSetup(for: test) options.disconnectedRetryTimeout = 1.0 options.autoConnect = false - options.testOptions.realtimeRequestTimeout = 0.1 let jitterCoefficients = StaticJitterCoefficients() let mockJitterCoefficientGenerator = MockJitterCoefficientGenerator(coefficients: jitterCoefficients) options.testOptions.jitterCoefficientGenerator = mockJitterCoefficientGenerator - options.authCallback = { _, _ in - // Ignore `completion` closure to force a time out - } + modifyOptions(options) let numberOfRetriesToWaitFor = 5 // arbitrarily chosen, large enough for us to have confidence that a sequence of retries is occurring, with the correct retry delays @@ -2283,7 +2283,7 @@ class RealtimeClientConnectionTests: XCTestCase { ).prefix(numberOfRetriesToWaitFor + 1 /* The +1 can be removed after #1782 is fixed; see note below */) ) let timesNeededToObserveRetries = expectedRetryDelays.map { retryDelay in - options.testOptions.realtimeRequestTimeout // waiting for connect to time out + extraTimeNeededToObserveEachRetry + retryDelay // waiting for retry to occur + 0.2 // some extra tolerance, arbitrarily chosen } @@ -2341,7 +2341,7 @@ class RealtimeClientConnectionTests: XCTestCase { let firstObservedStateChange = observedStateChanges[observedStateChangesStartIndexForThisRetry] XCTAssertEqual(firstObservedStateChange.stateChange.previous, .connecting) XCTAssertEqual(firstObservedStateChange.stateChange.current, .disconnected) - XCTAssertTrue(firstObservedStateChange.stateChange.reason!.message.contains("timed out")) + checkError(firstObservedStateChange.stateChange.reason!) XCTAssertEqual(firstObservedStateChange.stateChange.retryIn, expectedRetryDelay) if (firstObservedStateChangeToDisconnected == nil) { @@ -2370,6 +2370,25 @@ class RealtimeClientConnectionTests: XCTestCase { .to(beLessThan(connectionStateTtl + timeTakenByPotentialExcessRetry + tolerance)) } + // RTN14d, RTB1 + func test__059__Connection__connection_request_fails__connection_attempt_fails_for_any_recoverable_reason__for_example_a_timeout() throws { + let test = Test() + + let realtimeRequestTimeout = 0.1 + + try testRTN14dAndRTB1(test: test, + extraTimeNeededToObserveEachRetry: realtimeRequestTimeout, // waiting for connect to time out + modifyOptions: { options in + options.testOptions.realtimeRequestTimeout = realtimeRequestTimeout + options.authCallback = { _, _ in + // Ignore `completion` closure to force a time out + } + }, + checkError: { error in + XCTAssertTrue(error.message.contains("timed out")) + }) + } + // RTN14e func test__060__Connection__connection_request_fails__connection_state_has_been_in_the_DISCONNECTED_state_for_more_than_the_default_connectionStateTtl_should_change_the_state_to_SUSPENDED() throws { let test = Test() From 86b7cc72d1cf593bdd81d1a62021f7c0212c3210 Mon Sep 17 00:00:00 2001 From: Lawrence Forooghian Date: Fri, 3 Nov 2023 15:24:49 -0300 Subject: [PATCH 6/6] Treat all transport errors as recoverable MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit RTN14d says that a transport error should be considered recoverable if it is > a network failure, a timeout such as RTN14c, or a disconnected > response, other than a token failure RTN14b) However, it does not define what it means by a "network failure", leading to each platform having to provide its own interpretation. In particular, a client recently told us that they’ve been seeing lots of OSStatus 9806 (errSSLClosedAbort) errors. This appears to indicate some sort of failure to perform an SSL handshake. We don’t understand the cause of this issue but we noticed that it was causing the client to transition to the FAILED state. Speaking to Paddy, he said that this error should be provoking a connection retry, not failure. And more broadly, he indicated that, basically, _all_ transport errors should be considered recoverable. So, that’s what we do here. He’s raised a specification issue [1] for us to properly specify this and to decide if there are any nuances to consider, but is keen for us to implement this broad behaviour in ably-cocoa ASAP to help the customer experiencing the errSSLClosedAbort errors. Resolves #1817. [1] https://github.com/ably/specification/issues/171 --- Source/ARTRealtime.m | 30 ++++------ Test/Test Utilities/TestUtilities.swift | 18 ++++-- .../Tests/RealtimeClientConnectionTests.swift | 56 +++++++++++++------ 3 files changed, 62 insertions(+), 42 deletions(-) diff --git a/Source/ARTRealtime.m b/Source/ARTRealtime.m index ac1b0af9e..d207b27c8 100644 --- a/Source/ARTRealtime.m +++ b/Source/ARTRealtime.m @@ -1629,21 +1629,10 @@ - (void)realtimeTransportFailed:(id)transport withError:(A return; } } - - switch (transportError.type) { - case ARTRealtimeTransportErrorTypeBadResponse: - case ARTRealtimeTransportErrorTypeOther: { - ARTErrorInfo *const errorInfo = [ARTErrorInfo createFromNSError:transportError.error]; - ARTConnectionStateChangeMetadata *const metadata = [[ARTConnectionStateChangeMetadata alloc] initWithErrorInfo:errorInfo]; - [self transition:ARTRealtimeFailed withMetadata:metadata]; - break; - } - default: { - ARTErrorInfo *error = [ARTErrorInfo createFromNSError:transportError.error]; - ARTConnectionStateChangeMetadata *const metadata = [[ARTConnectionStateChangeMetadata alloc] initWithErrorInfo:error]; - [self transitionToDisconnectedOrSuspendedWithMetadata:metadata]; - } - } + + ARTErrorInfo *const errorInfo = [ARTErrorInfo createFromNSError:transportError.error]; + ARTConnectionStateChangeMetadata *const metadata = [[ARTConnectionStateChangeMetadata alloc] initWithErrorInfo:errorInfo]; + [self transitionToDisconnectedOrSuspendedWithMetadata:metadata]; } - (void)realtimeTransportNeverConnected:(id)transport { @@ -1654,7 +1643,7 @@ - (void)realtimeTransportNeverConnected:(id)transport { ARTErrorInfo *const errorInfo = [ARTErrorInfo createWithCode:ARTClientCodeErrorTransport message:@"Transport never connected"]; ARTConnectionStateChangeMetadata *const metadata = [[ARTConnectionStateChangeMetadata alloc] initWithErrorInfo:errorInfo]; - [self transition:ARTRealtimeFailed withMetadata:metadata]; + [self transitionToDisconnectedOrSuspendedWithMetadata:metadata]; } - (void)realtimeTransportRefused:(id)transport withError:(ARTRealtimeTransportError *)error { @@ -1666,15 +1655,16 @@ - (void)realtimeTransportRefused:(id)transport withError:( if (error && error.type == ARTRealtimeTransportErrorTypeRefused) { ARTErrorInfo *const errorInfo = [ARTErrorInfo createWithCode:ARTClientCodeErrorTransport message:[NSString stringWithFormat:@"Connection refused using %@", error.url]]; ARTConnectionStateChangeMetadata *const metadata = [[ARTConnectionStateChangeMetadata alloc] initWithErrorInfo:errorInfo]; - [self transition:ARTRealtimeFailed withMetadata:metadata]; + [self transitionToDisconnectedOrSuspendedWithMetadata:metadata]; } else if (error) { ARTErrorInfo *const errorInfo = [ARTErrorInfo createFromNSError:error.error]; ARTConnectionStateChangeMetadata *const metadata = [[ARTConnectionStateChangeMetadata alloc] initWithErrorInfo:errorInfo]; - [self transition:ARTRealtimeFailed withMetadata:metadata]; + [self transitionToDisconnectedOrSuspendedWithMetadata:metadata]; } else { - [self transition:ARTRealtimeFailed withMetadata:[[ARTConnectionStateChangeMetadata alloc] init]]; + ARTConnectionStateChangeMetadata *const metadata = [[ARTConnectionStateChangeMetadata alloc] init]; + [self transitionToDisconnectedOrSuspendedWithMetadata:metadata]; } } @@ -1686,7 +1676,7 @@ - (void)realtimeTransportTooBig:(id)transport { ARTErrorInfo *const errorInfo = [ARTErrorInfo createWithCode:ARTClientCodeErrorTransport message:@"Transport too big"]; ARTConnectionStateChangeMetadata *const metadata = [[ARTConnectionStateChangeMetadata alloc] initWithErrorInfo:errorInfo]; - [self transition:ARTRealtimeFailed withMetadata:metadata]; + [self transitionToDisconnectedOrSuspendedWithMetadata:metadata]; } - (void)realtimeTransportSetMsgSerial:(id)transport msgSerial:(int64_t)msgSerial { diff --git a/Test/Test Utilities/TestUtilities.swift b/Test/Test Utilities/TestUtilities.swift index ef07b6ccf..92aa7e809 100644 --- a/Test/Test Utilities/TestUtilities.swift +++ b/Test/Test Utilities/TestUtilities.swift @@ -751,6 +751,7 @@ enum FakeNetworkResponse { case requestTimeout(timeout: TimeInterval) case hostInternalError(code: Int) case host400BadRequest + case arbitraryError var error: NSError { switch self { @@ -764,6 +765,8 @@ enum FakeNetworkResponse { return NSError(domain: AblyTestsErrorDomain, code: code, userInfo: [NSLocalizedDescriptionKey: "internal error", NSLocalizedFailureReasonErrorKey: AblyTestsErrorDomain + ".FakeNetworkResponse"]) case .host400BadRequest: return NSError(domain: AblyTestsErrorDomain, code: 400, userInfo: [NSLocalizedDescriptionKey: "bad request", NSLocalizedFailureReasonErrorKey: AblyTestsErrorDomain + ".FakeNetworkResponse"]) + case .arbitraryError: + return NSError(domain: AblyTestsErrorDomain, code: 1, userInfo: [NSLocalizedDescriptionKey: "error from FakeNetworkResponse.arbitraryError"]) } } @@ -779,6 +782,8 @@ enum FakeNetworkResponse { return ARTRealtimeTransportError(error: error, badResponseCode: code, url: url) case .host400BadRequest: return ARTRealtimeTransportError(error: error, badResponseCode: 400, url: url) + case .arbitraryError: + return ARTRealtimeTransportError(error: error, type: .other, url: url) } } } @@ -864,6 +869,8 @@ class MockHTTP: ARTHttp { requestCallback?(HTTPURLResponse(url: URL(string: "http://cocoa.test.suite")!, statusCode: code, httpVersion: nil, headerFields: nil), nil, nil) case .host400BadRequest: requestCallback?(HTTPURLResponse(url: URL(string: "http://cocoa.test.suite")!, statusCode: 400, httpVersion: nil, headerFields: nil), nil, nil) + case .arbitraryError: + requestCallback?(nil, nil, NSError(domain: AblyTestsErrorDomain, code: 1, userInfo: [NSLocalizedDescriptionKey: "error from FakeNetworkResponse.arbitraryError"])) } } @@ -1289,7 +1296,8 @@ class TestProxyTransport: ARTWebSocketTransport { case .noInternet, .hostUnreachable, .hostInternalError, - .host400BadRequest: + .host400BadRequest, + .arbitraryError: performFakeConnectionError(0.1, error: networkResponse.transportError(for: url)) case .requestTimeout(let timeout): performFakeConnectionError(0.1 + timeout, error: networkResponse.transportError(for: url)) @@ -1658,9 +1666,11 @@ extension ARTWebSocketTransport { } func simulateIncomingError() { - let error = NSError(domain: ARTAblyErrorDomain, code: 0, userInfo: [NSLocalizedDescriptionKey:"Fail test"]) - let webSocketDelegate = self as ARTWebSocketDelegate - webSocketDelegate.webSocket?(self.websocket!, didFailWithError: error) + // Simulate receiving an ERROR ProtocolMessage, which should put a client into the FAILED state (per RTN15i) + let protocolMessage = ARTProtocolMessage() + protocolMessage.action = .error + protocolMessage.error = ARTErrorInfo.create(withCode: 50000 /* arbitrarily chosen */, message: "Fail test") + receive(protocolMessage) } } diff --git a/Test/Tests/RealtimeClientConnectionTests.swift b/Test/Tests/RealtimeClientConnectionTests.swift index 4524c05e1..346b4689d 100644 --- a/Test/Tests/RealtimeClientConnectionTests.swift +++ b/Test/Tests/RealtimeClientConnectionTests.swift @@ -2389,6 +2389,21 @@ class RealtimeClientConnectionTests: XCTestCase { }) } + // RTN14d, RTB1 + func test__059b__Connection__connection_request_fails__connection_attempt_fails_for_any_recoverable_reason__for_example_an_arbitrary_transport_error() throws { + let test = Test() + + try testRTN14dAndRTB1(test: test, + modifyOptions: { options in + let transportFactory = TestProxyTransportFactory() + transportFactory.fakeNetworkResponse = .arbitraryError + options.testOptions.transportFactory = transportFactory + }, + checkError: { error in + XCTAssertTrue(error.message.contains("error from FakeNetworkResponse.arbitraryError")) + }) + } + // RTN14e func test__060__Connection__connection_request_fails__connection_state_has_been_in_the_DISCONNECTED_state_for_more_than_the_default_connectionStateTtl_should_change_the_state_to_SUSPENDED() throws { let test = Test() @@ -3703,41 +3718,46 @@ class RealtimeClientConnectionTests: XCTestCase { try testMovesToDisconnectedWithNetworkingError(NSError(domain: "kCFErrorDomainCFNetwork", code: 1337, userInfo: [NSLocalizedDescriptionKey: "shouldn't matter"]), for: test) } - func test__090__Connection__Host_Fallback__should_not_use_an_alternative_host_when_the_client_receives_a_bad_request() { - let test = Test() + func test__090__Connection__Host_Fallback__should_not_use_an_alternative_host_when_the_client_receives_a_bad_request() throws { let options = ARTClientOptions(key: "xxxx:xxxx") options.autoConnect = false + options.disconnectedRetryTimeout = 1.0 // so that the test doesn't have to wait a long time to observe a retry options.testOptions.realtimeRequestTimeout = 1.0 let transportFactory = TestProxyTransportFactory() options.testOptions.transportFactory = transportFactory let client = ARTRealtime(options: options) - let channel = client.channels.get(test.uniqueChannelName()) transportFactory.fakeNetworkResponse = .host400BadRequest - var urlConnections = [URL]() - transportFactory.networkConnectEvent = { transport, url in - if client.internal.transport !== transport { - return + let dataGatherer = DataGatherer(description: "Observe emitted state changes and transport connection attempts") { submit in + var stateChanges: [ARTConnectionStateChange] = [] + var urlConnections = [URL]() + + client.connection.on { stateChange in + stateChanges.append(stateChange) + if (stateChanges.count == 3) { + submit((stateChanges: stateChanges, urlConnections: urlConnections)) + } + } + + transportFactory.networkConnectEvent = { transport, url in + if client.internal.transport !== transport { + return + } + urlConnections.append(url) } - urlConnections.append(url) } client.connect() defer { client.dispose(); client.close() } - // We expect the first connection attempt to fail due to the .fakeNetworkResponse configured above. This error does not meet the criteria for trying a fallback host, and so should not provoke the use of a fallback host. Hence the connection should give up and transition to the FAILED state (which causes the publish to fail). We should see that there was only one connection attempt, to the primary host. + let data = try dataGatherer.waitForData(timeout: testTimeout) - waitUntil(timeout: testTimeout) { done in - channel.publish(nil, data: "message") { error in - XCTAssertNotNil(error) - done() - } - } + // We expect the first connection attempt to fail due to the .fakeNetworkResponse configured above. This error does not meet the criteria for trying a fallback host, and so should not provoke the use of a fallback host. Hence the connection should transition to DISCONNECTED, and then subsequently retry, transitioning back to CONNECTING. We should see that there were two connection attempts, both to the primary host. - XCTAssertEqual(client.connection.state, .failed) - XCTAssertEqual(urlConnections.count, 1) - XCTAssertTrue(NSRegularExpression.match(urlConnections[0].absoluteString, pattern: "//realtime.ably.io")) + XCTAssertEqual(data.stateChanges.map(\.current), [.connecting, .disconnected, .connecting]) + XCTAssertEqual(data.urlConnections.count, 2) + XCTAssertTrue(data.urlConnections.allSatisfy { url in NSRegularExpression.match(url.absoluteString, pattern: "//realtime.ably.io") }) } // RTN17a