diff --git a/Simperium/SPWebSocketInterface.m b/Simperium/SPWebSocketInterface.m index 7dbdb259..b8bae278 100644 --- a/Simperium/SPWebSocketInterface.m +++ b/Simperium/SPWebSocketInterface.m @@ -237,7 +237,7 @@ - (void)stop:(SPBucket *)bucket { self.webSocket = nil; // Prevent any pending retries - [NSObject cancelPreviousPerformRequestsWithTarget:self]; + [NSObject cancelPreviousPerformRequestsWithTarget:self selector:@selector(openWebSocket) object:nil]; } - (void)reset:(SPBucket *)bucket completion:(SPNetworkInterfaceResetCompletion)completion { @@ -388,12 +388,13 @@ - (void)webSocket:(SPWebSocket *)webSocket didReceiveMessage:(id)message { } - (void)webSocket:(SPWebSocket *)webSocket didCloseWithCode:(NSInteger)code reason:(NSString *)reason wasClean:(BOOL)wasClean { - if (self.open) { - // Closed unexpectedly, retry + // Any close that reaches us is unexpected: an intentional stop nils the delegate before closing. + // That includes a close arriving before webSocketDidOpen (self.open == NO) — failing to retry + // there left the interface with no socket and nothing scheduled to rebuild it. + if (self.simperium.networkEnabled) { [self performSelector:@selector(openWebSocket) withObject:nil afterDelay:2]; SPLogVerbose(@"Simperium connection closed (will retry): %ld, %@", (long)code, reason); } else { - // Closed on purpose SPLogInfo(@"Simperium connection closed"); } diff --git a/Simperium/Simperium.m b/Simperium/Simperium.m index e0918229..5fe808e0 100644 --- a/Simperium/Simperium.m +++ b/Simperium/Simperium.m @@ -204,12 +204,16 @@ - (SPBucket *)bucketForName:(NSString *)name { #pragma mark ==================================================================================== - (void)startNetworkManagers { - if (!self.networkEnabled || self.networkManagersStarted || !self.appID) { + if (!self.networkEnabled || !self.appID) { return; } - + + // Note: networkManagersStarted only records that this ran once; the websocket may have been + // dropped since (e.g. it closed before completing its handshake). Don't early-return on it: + // the network interface's start: is idempotent for healthy connections, and re-running it is + // the only automatic path that rebuilds a dead one. SPLogInfo(@"Simperium starting network managers..."); - + // If this gets executed before a logout is complete, make sure this gets logged if (self.logoutInProgress) { SPLogError(@"Simperium Error: there is a pending logout operation that hasn't been fulfilled"); diff --git a/SimperiumTests/MockWebSocketInterface.h b/SimperiumTests/MockWebSocketInterface.h index d672884c..5adf8c3e 100644 --- a/SimperiumTests/MockWebSocketInterface.h +++ b/SimperiumTests/MockWebSocketInterface.h @@ -19,4 +19,7 @@ - (NSSet*)mockSentMessages; - (void)mockReceiveMessage:(NSString*)message; +- (NSArray*)mockStartedBucketNames; +- (void)mockClearStartedBucketNames; + @end diff --git a/SimperiumTests/MockWebSocketInterface.m b/SimperiumTests/MockWebSocketInterface.m index 4fdc74d2..9efce652 100644 --- a/SimperiumTests/MockWebSocketInterface.m +++ b/SimperiumTests/MockWebSocketInterface.m @@ -30,6 +30,7 @@ - (void)startChannels; @interface MockWebSocketInterface() @property (nonatomic, strong, readwrite) NSMutableSet* mutableSentMessages; +@property (nonatomic, strong, readwrite) NSMutableArray* mutableStartedBucketNames; @end @@ -52,6 +53,14 @@ - (void)mockReceiveMessage:(NSString*)message { [super webSocket:nil didReceiveMessage:message]; } +- (NSArray*)mockStartedBucketNames { + return self.mutableStartedBucketNames; +} + +- (void)mockClearStartedBucketNames { + [self.mutableStartedBucketNames removeAllObjects]; +} + #pragma mark ==================================================================================== #pragma mark Overriden Methods @@ -68,6 +77,15 @@ - (void)openWebSocket { // Do not open a SPRWebSocket instance } +- (void)start:(SPBucket*)bucket { + if (self.mutableStartedBucketNames == nil) { + self.mutableStartedBucketNames = [NSMutableArray array]; + } + + [self.mutableStartedBucketNames addObject:bucket.name]; + [super start:bucket]; +} + - (BOOL)open { // The "WebSocket" is always open, for unit testing purposes return YES; diff --git a/SimperiumTests/SPWebSocketInterfaceTests.m b/SimperiumTests/SPWebSocketInterfaceTests.m index bf9776af..8c5198fc 100644 --- a/SimperiumTests/SPWebSocketInterfaceTests.m +++ b/SimperiumTests/SPWebSocketInterfaceTests.m @@ -10,12 +10,59 @@ #import "XCTestCase+Simperium.h" #import "MockSimperium.h" #import "MockWebSocketInterface.h" +#import "Simperium+Internals.h" #import "SPLogger.h" #import "JSONKit+Simperium.h" #import "Config.h" +#pragma mark ==================================================================================== +#pragma mark Constants +#pragma mark ==================================================================================== + +static NSTimeInterval const SPReconnectionDelay = 2.5; + + +#pragma mark ==================================================================================== +#pragma mark SPWebSocketInterface: Exposing Private Methods +#pragma mark ==================================================================================== + +@interface SPWebSocketInterface () +@property (nonatomic, assign, readwrite) BOOL open; +- (instancetype)initWithSimperium:(Simperium *)s; +- (void)openWebSocket; +- (void)webSocket:(SPWebSocket *)webSocket didCloseWithCode:(NSInteger)code reason:(NSString *)reason wasClean:(BOOL)wasClean; +@end + + +#pragma mark ==================================================================================== +#pragma mark Simperium: Exposing Private Methods +#pragma mark ==================================================================================== + +@interface Simperium () +- (void)startNetworkManagers; +@end + + +#pragma mark ==================================================================================== +#pragma mark CountingWebSocketInterface +#pragma mark ==================================================================================== + +// Counts reconnection attempts without ever touching the network +@interface CountingWebSocketInterface : SPWebSocketInterface +@property (nonatomic, assign, readwrite) NSUInteger openAttempts; +@end + +@implementation CountingWebSocketInterface + +- (void)openWebSocket { + self.openAttempts++; +} + +@end + + #pragma mark ==================================================================================== #pragma mark SPWebSocketInterfaceTests #pragma mark ==================================================================================== @@ -109,4 +156,69 @@ - (void)testRemoteIndexRequest { XCTAssertTrue(responseSent, @"Index Request-Response wasn't sent!!"); } +- (void)testSocketClosedBeforeFinishingHandshakeSchedulesReconnection { + // A close that arrives before webSocketDidOpen (open == NO) used to be treated as an + // intentional close, wedging the interface: no socket, no retry, and nothing upstream + // notices. See SIMPL-75. + MockSimperium* s = [MockSimperium mockSimperium]; + CountingWebSocketInterface* interface = [[CountingWebSocketInterface alloc] initWithSimperium:s]; + + interface.open = NO; + [interface webSocket:nil didCloseWithCode:1006 reason:@"connection dropped mid-handshake" wasClean:NO]; + + [self waitFor:SPReconnectionDelay]; + XCTAssertTrue(interface.openAttempts > 0, @"A close during connection setup must schedule a reconnection"); +} + +- (void)testSocketClosedAfterOpeningSchedulesReconnection { + MockSimperium* s = [MockSimperium mockSimperium]; + CountingWebSocketInterface* interface = [[CountingWebSocketInterface alloc] initWithSimperium:s]; + + interface.open = YES; + [interface webSocket:nil didCloseWithCode:1006 reason:@"connection dropped" wasClean:NO]; + + [self waitFor:SPReconnectionDelay]; + XCTAssertTrue(interface.openAttempts > 0, @"An unexpected close must schedule a reconnection"); +} + +- (void)testSocketClosedWhileNetworkDisabledDoesNotReconnect { + MockSimperium* s = [MockSimperium mockSimperium]; + CountingWebSocketInterface* interface = [[CountingWebSocketInterface alloc] initWithSimperium:s]; + s.networkEnabled = NO; + + interface.open = NO; + [interface webSocket:nil didCloseWithCode:1006 reason:@"connection dropped" wasClean:NO]; + + [self waitFor:SPReconnectionDelay]; + XCTAssertTrue(interface.openAttempts == 0, @"No reconnection should be attempted while networking is disabled"); +} + +- (void)testStopCancelsPendingReconnection { + MockSimperium* s = [MockSimperium mockSimperium]; + SPBucket* bucket = [s bucketForName:NSStringFromClass([Config class])]; + CountingWebSocketInterface* interface = [[CountingWebSocketInterface alloc] initWithSimperium:s]; + + interface.open = YES; + [interface webSocket:nil didCloseWithCode:1006 reason:@"connection dropped" wasClean:NO]; + [interface stop:bucket]; + + [self waitFor:SPReconnectionDelay]; + XCTAssertTrue(interface.openAttempts == 0, @"An intentional stop must cancel any pending reconnection"); +} + +- (void)testStartNetworkManagersRestartsBucketsEvenWhenAlreadyFlaggedAsStarted { + // The networkManagersStarted flag only records that start was called once; the websocket + // may have been dropped since. Restarting must reach the network interface regardless, + // since SPWebSocketInterface's start: is idempotent for healthy connections. + MockSimperium* s = [MockSimperium mockSimperium]; + [s bucketForName:NSStringFromClass([Config class])]; + XCTAssertTrue(s.networkManagersStarted, @"Expected network managers to be started after authentication"); + + [s.mockWebSocketInterface mockClearStartedBucketNames]; + [s startNetworkManagers]; + + XCTAssertTrue(s.mockWebSocketInterface.mockStartedBucketNames.count > 0, + @"Restarting network managers must restart the buckets' network interface"); +} + @end