Skip to content

Commit

Permalink
Fix websocket reconnection can get stuck in disconnected/connecting c…
Browse files Browse the repository at this point in the history
…ycle

Fixes a regression introduced in ably-js v2 in the "no upgrade" PR [1].
Under certain network conditions (internet down) a race condition could
occur in the connection manager. Specifically, websocket connectivity
check in the `startWebSocketSlowTimer` function needed to set the
`wsCheckResult` flag to `false` before the timer was cleared by other
code branches. If that happened, it then caused an endless loop of
websocket connection retries, even after the network conditions
stabilized. A websocket transport would successfully open but then be
immediately disposed due to the `wsCheckResult` flag still being set to
`false`, without any mechanism to reset it in this scenario.

Upon closer look, it makes sense that the websocket connection health
flags (`wsCheckResult` and `abandonedWebSocket`) should be reset when
attempting a new websocket connection. The previous solution did not
explicitly reset these flags in the `connectWs` function, leading to the
described race condition and endless loop. Thus, `wsCheckResult` and
`abandonedWebSocket` are now reset to their neutral values upon entering
the `connectWs` function.

Resolves #1844

[1] #1645
  • Loading branch information
VeskeR committed Sep 6, 2024
1 parent 32efc56 commit 60288b3
Showing 1 changed file with 21 additions and 23 deletions.
44 changes: 21 additions & 23 deletions src/common/lib/transport/connectionmanager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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);

Expand Down

0 comments on commit 60288b3

Please sign in to comment.