Merge pull request #1855 from ably/1844/fix-ws-connection-recovery

[ECO-4914] Fix websocket reconnection can get stuck in disconnected/connecting cycle

src/common/lib/transport/connectionmanager.ts

@@ -1101,27 +1101,25 @@ class ConnectionManager extends EventEmitter {
         'ConnectionManager WebSocket slow timer',
         'checking connectivity',
       );
-      if (this.wsCheckResult === null) {
-        this.checkWsConnectivity()
-          .then(() => {
-            Logger.logAction(
-              this.logger,
-              Logger.LOG_MINOR,
-              'ConnectionManager WebSocket slow timer',
-              'ws connectivity check succeeded',
-            );
-            this.wsCheckResult = true;
-          })
-          .catch(() => {
-            Logger.logAction(
-              this.logger,
-              Logger.LOG_MAJOR,
-              'ConnectionManager WebSocket slow timer',
-              'ws connectivity check failed',
-            );
-            this.wsCheckResult = false;
-          });
-      }
+      this.checkWsConnectivity()
+        .then(() => {
+          Logger.logAction(
+            this.logger,
+            Logger.LOG_MINOR,
+            'ConnectionManager WebSocket slow timer',
+            'ws connectivity check succeeded',
+          );
+          this.wsCheckResult = true;
+        })
+        .catch(() => {
+          Logger.logAction(
+            this.logger,
+            Logger.LOG_MAJOR,
+            'ConnectionManager WebSocket slow timer',
+            'ws connectivity check failed',
+          );
+          this.wsCheckResult = false;
+        });
       if (this.realtime.http.checkConnectivity) {
         Utils.whenPromiseSettles(this.realtime.http.checkConnectivity(), (err, connectivity) => {
           if (err || !connectivity) {
@@ -1450,8 +1448,6 @@ class ConnectionManager extends EventEmitter {
     if (transportPreference && transportPreference === this.baseTransport && this.webSocketTransportAvailable) {
       this.checkWsConnectivity()
         .then(() => {
-          this.wsCheckResult = true;
-          this.abandonedWebSocket = false;
           this.unpersistTransportPreference();
           if (this.state === this.states.connecting) {
             Logger.logAction(
@@ -1493,6 +1489,8 @@ class ConnectionManager extends EventEmitter {
    */
   connectWs(transportParams: TransportParams, connectCount: number) {
     Logger.logAction(this.logger, Logger.LOG_MICRO, 'ConnectionManager.connectWs()');
+    this.wsCheckResult = null;
+    this.abandonedWebSocket = false;
     this.startWebSocketSlowTimer();
     this.startWebSocketGiveUpTimer(transportParams);
 

test/common/modules/private_api_recorder.js

@@ -68,6 +68,7 @@ define(['test/support/output_directory_paths'], function (outputDirectoryPaths)
     'pass.clientOption.disableConnectivityCheck', // actually ably-js public API (i.e. it’s in the TypeScript typings) but no other SDK has it and it doesn’t enable ably-js-specific functionality
     'pass.clientOption.pushRecipientChannel',
     'pass.clientOption.webSocketConnectTimeout',
+    'pass.clientOption.webSocketSlowTimeout',
     'read.Defaults.version',
     'read.EventEmitter.events',
     'read.Platform.Config.push',
@@ -97,6 +98,7 @@ define(['test/support/output_directory_paths'], function (outputDirectoryPaths)
     'read.realtime.options',
     'read.realtime.options.key',
     'read.realtime.options.maxMessageSize',
+    'read.realtime.options.realtimeHost',
     'read.realtime.options.token',
     'read.rest._currentFallback',
     'read.rest._currentFallback.host',
@@ -123,6 +125,7 @@ define(['test/support/output_directory_paths'], function (outputDirectoryPaths)
     'replace.transport.send',
     'serialize.recoveryKey',
     'write.Defaults.ENVIRONMENT',
+    'write.Defaults.wsConnectivityUrl',
     'write.Platform.Config.push', // This implies using a mock implementation of the internal IPlatformPushConfig interface. Our mock (in push_channel_transport.js) then interacts with internal objects and private APIs of public objects to implement this interface; I haven’t added annotations for that private API usage, since there wasn’t an easy way to pass test context information into the mock. I think that for now we can just say that if we wanted to get rid of this private API usage, then we’d need to remove this mock entirely.
     'write.auth.authOptions.requestHeaders',
     'write.auth.key',
@@ -134,6 +137,8 @@ define(['test/support/output_directory_paths'], function (outputDirectoryPaths)
     'write.connectionManager.connectionKey',
     'write.connectionManager.lastActivity',
     'write.connectionManager.msgSerial',
+    'write.connectionManager.wsHosts',
+    'write.realtime.options.realtimeHost',
     'write.realtime.options.timeouts.realtimeRequestTimeout',
     'write.rest._currentFallback.validUntil',
   ];

test/realtime/transports.test.js

@@ -157,6 +157,81 @@ define(['shared_helper', 'async', 'chai', 'ably'], function (Helper, async, chai
         });
       });
 
+      /** @nospec */
+      it('ws_can_reconnect_after_ws_connectivity_fail', function (done) {
+        const helper = this.test.helper;
+        helper.recordPrivateApi('read.realtime.options.realtimeHost');
+        const goodHost = helper.AblyRest().options.realtimeHost;
+
+        // use unroutable host ws connectivity check to simulate no internet
+        helper.recordPrivateApi('write.Defaults.wsConnectivityUrl');
+        Defaults.wsConnectivityUrl = `wss://${helper.unroutableAddress}`;
+
+        helper.recordPrivateApi('pass.clientOption.webSocketSlowTimeout');
+        const realtime = helper.AblyRealtime(
+          options(helper, {
+            realtimeHost: helper.unroutableAddress,
+            // ensure ws slow timeout procs and performs ws connectivity check, which would fail due to unroutable host
+            webSocketSlowTimeout: 1,
+            // give up trying to connect fairly quickly
+            realtimeRequestTimeout: 2000,
+            // try to reconnect quickly
+            disconnectedRetryTimeout: 2000,
+          }),
+        );
+        const connection = realtime.connection;
+
+        // simulate the internet being failed by stubbing out tryATransport to foil
+        // the initial connection
+        helper.recordPrivateApi('replace.connectionManager.tryATransport');
+        const tryATransportOriginal = connection.connectionManager.tryATransport;
+        connection.connectionManager.tryATransport = function () {};
+
+        async.series(
+          [
+            function (cb) {
+              realtime.connection.once('disconnected', function () {
+                cb();
+              });
+            },
+            function (cb) {
+              // restore original settings
+              helper.recordPrivateApi('replace.connectionManager.tryATransport');
+              connection.connectionManager.tryATransport = tryATransportOriginal;
+              helper.recordPrivateApi('write.Defaults.wsConnectivityUrl');
+              Defaults.wsConnectivityUrl = originialWsCheckUrl;
+              helper.recordPrivateApi('write.realtime.options.realtimeHost');
+              realtime.options.realtimeHost = goodHost;
+              helper.recordPrivateApi('write.connectionManager.wsHosts');
+              realtime.connection.connectionManager.wsHosts = [goodHost];
+
+              cb();
+            },
+            function (cb) {
+              // should reconnect successfully
+              realtime.connection.once('connected', function () {
+                cb();
+              });
+
+              realtime.connection.once('disconnected', function () {
+                try {
+                  // fast fail if we end up in the disconnected state again
+                  expect(
+                    connection.state !== 'disconnected',
+                    'Connection should not remain disconnected after websocket reconnection attempt even after failed ws connectivity check from previous connection attempt',
+                  ).to.be.ok;
+                } catch (err) {
+                  cb(err);
+                }
+              });
+            },
+          ],
+          function (err) {
+            helper.closeAndFinish(done, realtime, err);
+          },
+        );
+      });
+
       if (localStorageSupported) {
         /** @nospec */
         it('base_transport_preference', function (done) {