Retry websocket connections that drop before completing the handshake

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");
     }
 

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");

SimperiumTests/MockWebSocketInterface.h

@@ -19,4 +19,7 @@
 - (NSSet*)mockSentMessages;
 - (void)mockReceiveMessage:(NSString*)message;
 
+- (NSArray*)mockStartedBucketNames;
+- (void)mockClearStartedBucketNames;
+
 @end

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;

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