Skip to content

Commit

Permalink
Merge pull request #1823 from ably/1817-treat-all-transport-errors-as…
Browse files Browse the repository at this point in the history 
…-recoverable

[SDK-3926] Treat all transport errors as recoverable
  • Loading branch information
@lawrence-forooghian
lawrence-forooghian authored Nov 7, 2023
2 parents 65349da + 86b7cc7 commit f3bf3c4
Show file tree
Hide file tree
Showing 5 changed files with 108 additions and 53 deletions.
30 changes: 10 additions & 20 deletions Source/ARTRealtime.m
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1629,21 +1629,10 @@ - (void)realtimeTransportFailed:(id<ARTRealtimeTransport>)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<ARTRealtimeTransport>)transport {
Expand All @@ -1654,7 +1643,7 @@ - (void)realtimeTransportNeverConnected:(id<ARTRealtimeTransport>)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<ARTRealtimeTransport>)transport withError:(ARTRealtimeTransportError *)error {
Expand All @@ -1666,15 +1655,16 @@ - (void)realtimeTransportRefused:(id<ARTRealtimeTransport>)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];
}
}

Expand All @@ -1686,7 +1676,7 @@ - (void)realtimeTransportTooBig:(id<ARTRealtimeTransport>)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<ARTRealtimeTransport>)transport msgSerial:(int64_t)msgSerial {
Expand Down
18 changes: 14 additions & 4 deletions Source/ARTWebSocketTransport.m
View file
Original file line number Diff line number Diff line change
Expand Up @@ -282,20 +282,30 @@ - (void)webSocket:(id<ARTWebSocket>)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;
case ARTWsNoUtf8:
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;
Expand Down
3 changes: 3 additions & 0 deletions Source/PrivateHeaders/Ably/ARTRealtimeTransport.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down
18 changes: 14 additions & 4 deletions Test/Test Utilities/TestUtilities.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -751,6 +751,7 @@ enum FakeNetworkResponse {
case requestTimeout(timeout: TimeInterval)
case hostInternalError(code: Int)
case host400BadRequest
case arbitraryError

var error: NSError {
switch self {
Expand All @@ -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"])
}
}

Expand All @@ -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)
}
}
}
Expand Down Expand Up @@ -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"]))
}
}

Expand Down Expand Up @@ -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))
Expand Down Expand Up @@ -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)
}
}

Expand Down
92 changes: 67 additions & 25 deletions Test/Tests/RealtimeClientConnectionTests.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2258,22 +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.realtimeHost = "10.255.255.1" // non-routable IP address
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

Expand All @@ -2284,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
}
Expand Down Expand Up @@ -2342,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) {
Expand Down Expand Up @@ -2371,6 +2370,40 @@ 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"))
})
}

// 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()
Expand Down Expand Up @@ -3685,37 +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() }

waitUntil(timeout: testTimeout) { done in
channel.publish(nil, data: "message") { _ in
done()
}
}
let data = try dataGatherer.waitForData(timeout: testTimeout)

XCTAssertEqual(urlConnections.count, 1)
XCTAssertTrue(NSRegularExpression.match(urlConnections[0].absoluteString, pattern: "//realtime.ably.io"))
// 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(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
Expand Down

0 comments on commit f3bf3c4

Please sign in to comment.