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

Update RSC15a to new requirements #466

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions Source/ARTClientOptions.h
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,11 @@ ART_ASSUME_NONNULL_BEGIN
*/
@property (readwrite, assign, nonatomic) NSTimeInterval httpMaxRetryDuration;

/**
If an array of custom fallback hosts are provided, then they will be used instead of default hosts.
*/
@property (nonatomic, copy, art_nullable) NSArray *fallbackHosts;

- (BOOL)isBasicAuth;
- (NSURL *)restUrl;
- (NSURL *)realtimeUrl;
Expand Down
2 changes: 2 additions & 0 deletions Source/ARTClientOptions.m
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ - (instancetype)initDefaults {
_httpRequestTimeout = 15.0; //Seconds
_httpMaxRetryDuration = 10.0; //Seconds
_httpMaxRetryCount = 3;
_fallbackHosts = nil;
return self;
}

Expand Down Expand Up @@ -98,6 +99,7 @@ - (id)copyWithZone:(NSZone *)zone {
options.httpMaxRetryDuration = self.httpMaxRetryDuration;
options.httpOpenTimeout = self.httpOpenTimeout;
options.httpRequestTimeout = self.httpRequestTimeout;
options.fallbackHosts = self.fallbackHosts;

return options;
}
Expand Down
5 changes: 5 additions & 0 deletions Source/ARTFallback.h
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,11 @@ extern int (^ARTFallback_getRandomHostIndex)(int count);
*/
-(NSString *) popFallbackHost;

/**
Init with fallback hosts array.
*/
-(instancetype)initWithFallbackHosts:(NSArray *)fallbackHosts;

@end

ART_ASSUME_NONNULL_END
7 changes: 6 additions & 1 deletion Source/ARTFallback.m
Original file line number Diff line number Diff line change
Expand Up @@ -26,10 +26,15 @@ @interface ARTFallback ()
@implementation ARTFallback

- (id)init {
self = [self initWithFallbackHosts:[ARTDefault fallbackHosts]];
return self;
}

-(instancetype)initWithFallbackHosts:(NSArray *)fallbackHosts {
self = [super init];
if(self) {
self.hosts = [NSMutableArray array];
NSMutableArray * hostArray =[[NSMutableArray alloc] initWithArray: [ARTDefault fallbackHosts]];
NSMutableArray * hostArray = [[NSMutableArray alloc] initWithArray: [fallbackHosts copy]];
size_t count = [hostArray count];
for(int i=0; i <count; i++ ) {
int randomIndex = ARTFallback_getRandomHostIndex((int)[hostArray count]);
Expand Down
6 changes: 5 additions & 1 deletion Source/ARTRest.m
Original file line number Diff line number Diff line change
Expand Up @@ -154,7 +154,11 @@ - (void)executeRequest:(NSMutableURLRequest *)request completion:(void (^)(NSHTT
}
if (retries < _options.httpMaxRetryCount && [self shouldRetryWithFallback:request response:response error:error]) {
if (!blockFallbacks && [request.URL.host isEqualToString:(_prioritizedHost ? _prioritizedHost : [ARTDefault restHost])]) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RSC15b has changed as well, so this condition needs to incorporate this: "or an array of ClientOptions#fallbackHosts is provided"

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @tcard, however @CezaryKopacz's scope was only to look at RSC15a for now, so that's a valid comment, but will be addressed in a separate PR.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh OK, sorry, just felt weird as if you're using custom fallback hosts you most probably are also using a custom primary host.

blockFallbacks = [[ARTFallback alloc] init];
if(self.options.fallbackHosts != nil) {
blockFallbacks = [[ARTFallback alloc] initWithFallbackHosts:self.options.fallbackHosts];
} else {
blockFallbacks = [[ARTFallback alloc] init];
}
}
if (blockFallbacks) {
NSString *host = [blockFallbacks popFallbackHost];
Expand Down
31 changes: 31 additions & 0 deletions Spec/RestClient.swift
Original file line number Diff line number Diff line change
Expand Up @@ -677,6 +677,37 @@ class RestClient: QuickSpec {

expect(resultFallbackHosts).to(equal(expectedFallbackHosts))
}

it("use custom fallback hosts if set") {
let options = ARTClientOptions(key: "xxxx:xxxx")
options.httpMaxRetryCount = 10
let customFallbackHosts = ["j.ably-realtime.com",
"i.ably-realtime.com",
"h.ably-realtime.com",
"g.ably-realtime.com",
"f.ably-realtime.com"]
options.fallbackHosts = customFallbackHosts
let client = ARTRest(options: options)
client.httpExecutor = testHTTPExecutor
testHTTPExecutor.http = MockHTTP(network: .HostUnreachable)
let channel = client.channels.get("test")

waitUntil(timeout: testTimeout) { done in
channel.publish(nil, data: "nil") { _ in
done()
}
}

expect(testHTTPExecutor.requests).to(haveCount(customFallbackHosts.count + 1))

let extractHostname = { (request: NSMutableURLRequest) in
NSRegularExpression.extract(request.URL!.absoluteString, pattern: "[f-j].ably-realtime.com")
}
let resultFallbackHosts = testHTTPExecutor.requests.flatMap(extractHostname)
let expectedFallbackHosts = expectedHostOrder.map { customFallbackHosts[$0] }

expect(resultFallbackHosts).to(equal(expectedFallbackHosts))
}

it("until all fallback hosts have been tried") {
let options = ARTClientOptions(key: "xxxx:xxxx")
Expand Down
44 changes: 44 additions & 0 deletions Tests/ARTFallbackTest.m
Original file line number Diff line number Diff line change
Expand Up @@ -43,4 +43,48 @@ - (void)testAllHostsIncludedOnce {
}
}

- (void)testCustomFallbackHosts {
NSArray *customHosts = @[@"test.ably.com",
@"test2.ably.com",
@"test3.ably.com",
@"test4.ably.com",
@"test5.ably.com",
@"test6.ably.com",
@"test7.ably.com",
@"test8.ably.com",
@"test9.ably.com",
@"test10.ably.com",
@"test11.ably.com",
@"test12.ably.com",
@"test13.ably.com",
@"test14.ably.com"];
ARTFallback *f = [[ARTFallback alloc] initWithFallbackHosts:customHosts];

NSSet *customSet = [NSSet setWithArray:customHosts];
NSMutableArray *hostsRandomised = [NSMutableArray array];
for(int i=0;i < [customHosts count]; i++) {
[hostsRandomised addObject:[f popFallbackHost]];
}

//popping after all hosts are exhausted returns nil
XCTAssertTrue([f popFallbackHost] == nil);

// all fallback hosts are used in artfallback
XCTAssertEqual([hostsRandomised count], [customHosts count]);
bool inOrder = true;
for(int i=0;i < [customHosts count]; i++) {
if(![[customHosts objectAtIndex:i] isEqualToString:[hostsRandomised objectAtIndex:i]]) {
inOrder = false;
break;
}
}
//check artfallback randomises the order.
XCTAssertFalse(inOrder);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW. This test will unfortunately fail every 6 (3x2) times as randomising a set of fallback hosts will be in the original order once every 6 times statistically. So this test is probably not ideal. We could make it statistically unlikely to fail by having a decent number of random hosts (12 would result in failure once every 480 million test runs) or simply run the tests multiple times in a row and only fail if all test runs fail.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point! BTW I can see similar test which will probably fail too. Anyway, what needs to be done yet in order to merge PR?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well I wouldn't worry about the other tests. If you could fix this one that would be great.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please check my latest commit

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have already support for doing this clean by overriding the random function. See https://github.com/ably/ably-ios/blob/e049dab0e87eaa1643e21d70ae3a771529e0d9e7/Spec/RealtimeClientConnection.swift#L2362-L2372 which lets us expect a given order: https://github.com/ably/ably-ios/blob/e049dab0e87eaa1643e21d70ae3a771529e0d9e7/Spec/RealtimeClientConnection.swift#L2585 If we get the expected order, we prove that fallback logic is using the ARTFallback_getRandomHostIndex function correctly, and we can just trust that the default value for ARTFallback_getRandomHostIndex works as expected


//every member of fallbacks hosts are in the list of custom hosts
for(int i=0;i < [hostsRandomised count]; i++) {
XCTAssertTrue([customSet containsObject:[hostsRandomised objectAtIndex:i]]);
}
}

@end