From c77f95f6c7fc90fe0976b932a153f8a21a1f1fec Mon Sep 17 00:00:00 2001 From: Lawrence Forooghian Date: Fri, 3 Nov 2023 14:36:30 -0300 Subject: [PATCH 1/5] Improve readability of error mapping Includes checking error domains by pointer equality (which is how it should be done). --- Source/ARTWebSocketTransport.m | 6 +++--- Test/Test Utilities/TestUtilities.swift | 2 +- Test/Tests/RealtimeClientConnectionTests.swift | 10 +++++----- 3 files changed, 9 insertions(+), 9 deletions(-) diff --git a/Source/ARTWebSocketTransport.m b/Source/ARTWebSocketTransport.m index 3ac74e466..a65e2b0b2 100644 --- a/Source/ARTWebSocketTransport.m +++ b/Source/ARTWebSocketTransport.m @@ -317,11 +317,11 @@ - (ARTRealtimeTransportError *)classifyError:(NSError *)error { if ([error.domain isEqualToString:@"com.squareup.SocketRocket"] && error.code == 504) { type = ARTRealtimeTransportErrorTypeTimeout; - } else if ([error.domain isEqualToString:(NSString *)kCFErrorDomainCFNetwork]) { + } else if (error.domain == (NSString *)kCFErrorDomainCFNetwork) { type = ARTRealtimeTransportErrorTypeHostUnreachable; - } else if ([error.domain isEqualToString:@"NSPOSIXErrorDomain"] && (error.code == 57 || error.code == 50)) { + } else if (error.domain == NSPOSIXErrorDomain && (error.code == ENOTCONN || error.code == ENETDOWN)) { type = ARTRealtimeTransportErrorTypeNoInternet; - } else if ([error.domain isEqualToString:ARTSRWebSocketErrorDomain] && error.code == 2132) { + } else if (error.domain == ARTSRWebSocketErrorDomain && error.code == 2132) { id status = error.userInfo[ARTSRHTTPResponseErrorKey]; if (status) { return [[ARTRealtimeTransportError alloc] initWithError:error diff --git a/Test/Test Utilities/TestUtilities.swift b/Test/Test Utilities/TestUtilities.swift index ef07b6ccf..46721160e 100644 --- a/Test/Test Utilities/TestUtilities.swift +++ b/Test/Test Utilities/TestUtilities.swift @@ -755,7 +755,7 @@ enum FakeNetworkResponse { var error: NSError { switch self { case .noInternet: - return NSError(domain: NSPOSIXErrorDomain, code: 50, userInfo: [NSLocalizedDescriptionKey: "network is down", NSLocalizedFailureReasonErrorKey: AblyTestsErrorDomain + ".FakeNetworkResponse"]) + return NSError(domain: NSPOSIXErrorDomain, code: Int(ENETDOWN), userInfo: [NSLocalizedDescriptionKey: "network is down", NSLocalizedFailureReasonErrorKey: AblyTestsErrorDomain + ".FakeNetworkResponse"]) case .hostUnreachable: return NSError(domain: kCFErrorDomainCFNetwork as String, code: 2, userInfo: [NSLocalizedDescriptionKey: "host unreachable", NSLocalizedFailureReasonErrorKey: AblyTestsErrorDomain + ".FakeNetworkResponse"]) case .requestTimeout: diff --git a/Test/Tests/RealtimeClientConnectionTests.swift b/Test/Tests/RealtimeClientConnectionTests.swift index 734aa8845..5ef6b7476 100644 --- a/Test/Tests/RealtimeClientConnectionTests.swift +++ b/Test/Tests/RealtimeClientConnectionTests.swift @@ -3670,19 +3670,19 @@ class RealtimeClientConnectionTests: XCTestCase { testUsesAlternativeHostOnResponse(.hostInternalError(code: 501), channelName: test.uniqueChannelName()) } - func test__100__Connection__Host_Fallback__should_move_to_disconnected_when_there_s_no_internet__with_NSPOSIXErrorDomain_with_code_57() throws { + func test__100__Connection__Host_Fallback__should_move_to_disconnected_when_there_s_no_internet__with_NSPOSIXErrorDomain_with_code_ENOTCONN() throws { let test = Test() - try testMovesToDisconnectedWithNetworkingError(NSError(domain: "NSPOSIXErrorDomain", code: 57, userInfo: [NSLocalizedDescriptionKey: "shouldn't matter"]), for: test) + try testMovesToDisconnectedWithNetworkingError(NSError(domain: NSPOSIXErrorDomain, code: Int(ENOTCONN), userInfo: [NSLocalizedDescriptionKey: "shouldn't matter"]), for: test) } - func test__101__Connection__Host_Fallback__should_move_to_disconnected_when_there_s_no_internet__with_NSPOSIXErrorDomain_with_code_50() throws { + func test__101__Connection__Host_Fallback__should_move_to_disconnected_when_there_s_no_internet__with_NSPOSIXErrorDomain_with_code_ENETDOWN() throws { let test = Test() - try testMovesToDisconnectedWithNetworkingError(NSError(domain: "NSPOSIXErrorDomain", code: 50, userInfo: [NSLocalizedDescriptionKey: "shouldn't matter"]), for: test) + try testMovesToDisconnectedWithNetworkingError(NSError(domain: NSPOSIXErrorDomain, code: Int(ENETDOWN), userInfo: [NSLocalizedDescriptionKey: "shouldn't matter"]), for: test) } func test__102__Connection__Host_Fallback__should_move_to_disconnected_when_there_s_no_internet__with_any_kCFErrorDomainCFNetwork() throws { let test = Test() - try testMovesToDisconnectedWithNetworkingError(NSError(domain: "kCFErrorDomainCFNetwork", code: 1337, userInfo: [NSLocalizedDescriptionKey: "shouldn't matter"]), for: test) + try testMovesToDisconnectedWithNetworkingError(NSError(domain: kCFErrorDomainCFNetwork as String, 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() { From 9a81bc554d27e1e379c4b545259f4875ce60f2d8 Mon Sep 17 00:00:00 2001 From: Lawrence Forooghian Date: Fri, 3 Nov 2023 14:50:40 -0300 Subject: [PATCH 2/5] 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 a65e2b0b2..77a7d32ba 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 c16c7e98f5a0874f5a584493e0a8ce9ba3f48591 Mon Sep 17 00:00:00 2001 From: Lawrence Forooghian Date: Fri, 3 Nov 2023 14:55:47 -0300 Subject: [PATCH 3/5] 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 373211c5e0a73fce2c734996df1618fc1c4ea0dd Mon Sep 17 00:00:00 2001 From: Lawrence Forooghian Date: Mon, 6 Nov 2023 09:34:23 -0300 Subject: [PATCH 4/5] 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 5ef6b7476..ba2755392 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 561ea45b17171d9ee6a00c9a77d14f90b90cd2aa Mon Sep 17 00:00:00 2001 From: Lawrence Forooghian Date: Fri, 3 Nov 2023 15:24:49 -0300 Subject: [PATCH 5/5] Treat all transport errors as recoverable to see what tests fail Investigating #1817. --- Source/ARTRealtime.m | 30 +++++--------- Test/Test Utilities/TestUtilities.swift | 8 ++-- .../Tests/RealtimeClientConnectionTests.swift | 41 +++++++++++-------- 3 files changed, 38 insertions(+), 41 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 46721160e..ca55f278c 100644 --- a/Test/Test Utilities/TestUtilities.swift +++ b/Test/Test Utilities/TestUtilities.swift @@ -1658,9 +1658,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 ba2755392..a33af027a 100644 --- a/Test/Tests/RealtimeClientConnectionTests.swift +++ b/Test/Tests/RealtimeClientConnectionTests.swift @@ -3685,41 +3685,46 @@ class RealtimeClientConnectionTests: XCTestCase { try testMovesToDisconnectedWithNetworkingError(NSError(domain: kCFErrorDomainCFNetwork as String, 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