Skip to content

Commit b704932

Browse files
AndroidPoetspydon
andauthored
fix(realtime): cancel pending reconnect on disconnect after a dropped socket (#1477)
## What Fixes three related problems in `RealtimeClient` around a socket that dropped unexpectedly: 1. `disconnect()` now always cancels a pending reconnect, so disconnecting after the socket has already dropped no longer silently reopens the connection. 2. Automatic reconnects keep their exponential backoff instead of resetting to the shortest delay on every attempt. 3. A manual `connect()` after an unexpected drop now actually reconnects instead of being a no-op. ## Why When the socket drops unexpectedly, `_onConnClose` sets `connState = closed` and schedules a reconnect through `reconnectTimer.scheduleTimeout()`. `conn` is not nulled on a drop, so a later `disconnect()` still enters the `conn != null` branch, but `oldState == closed` makes `shouldCloseSink` false, so the close block that cancelled the timer was skipped. `conn` was nulled and the method returned, yet the armed backoff timer still fired, ran `_reconnect()` and reopened a connection the user explicitly closed. Cancelling the timer on every `disconnect()` fixes that, but it cannot use `reconnectTimer.reset()`: the reconnect path is `_reconnect -> disconnect -> connect`, so `reset()` would zero the retry counter on every attempt and defeat exponential backoff. During an outage the client would hammer an unreachable server at the shortest interval instead of backing off. A new `RetryTimer.cancel()` cancels the scheduled timer without touching the retry counter, so a user disconnect still stops a pending reconnect while automatic reconnects keep backing off. The counter is still zeroed on a successful open through `reset()` in `_onConnOpen`. Finally, because `conn` is intentionally kept non-null after a drop so `conn.closeCode` and `conn.closeReason` stay readable, the `if (conn != null) return` guard in `connect()` treated the dead channel as a live connection and turned a manual `connect()` into a no-op. `connect()` now only bails out when the connection is live or in progress. When `conn` is set but `connState == closed`, it tears the stale connection down through `disconnect()` first, which cancels the old stream subscription, and then opens a fresh one. ## Not a breaking change The public `conn` field is still left non-null after an unexpected drop, so consumers reading `conn.closeCode` or `conn.closeReason` are unaffected. `connState` transitions are unchanged: a socket that was already `closed` still ends up `closed`, and an open socket still ends up `disconnected` after `disconnect()`. `RetryTimer.reset()` keeps its existing behavior and now delegates its cancel step to the new `cancel()` method. ## Tests Added three tests to the `disconnect` group: - `cancels a pending reconnect after an unexpected drop`: drop the socket so a reconnect is scheduled, then `disconnect()` and assert the transport is never invoked again. - `reconnects on a manual connect() after an unexpected drop`: drop the socket, call `connect()` manually, and assert a fresh connection is opened and `connState` becomes `open`. - `grows the reconnect backoff across failed attempts`: fail the first few connection attempts and assert the retry counter passed to `reconnectAfterMs` grows as `1, 2, 3` instead of being reset to `1` on every reconnect. Each test fails without its corresponding fix. --------- Co-authored-by: Lukas Klingsbo <lukas.klingsbo@gmail.com>
1 parent ac579c6 commit b704932

3 files changed

Lines changed: 170 additions & 3 deletions

File tree

packages/realtime_client/lib/src/realtime_client.dart

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -213,7 +213,10 @@ class RealtimeClient {
213213
@internal
214214
Future<void> connect() async {
215215
if (conn != null) {
216-
return;
216+
if (connState != SocketStates.closed) {
217+
return;
218+
}
219+
await disconnect();
217220
}
218221

219222
try {
@@ -300,9 +303,15 @@ class RealtimeClient {
300303
await conn.sink.close();
301304
}
302305
connState = SocketStates.disconnected;
303-
reconnectTimer.reset();
304306
log('transport', 'disconnected', null, Level.FINE);
305307
}
308+
309+
// Cancel any reconnect scheduled by `_onConnClose`. When the socket has
310+
// already dropped (`connState == closed`) the block above is skipped, so
311+
// without this an armed backoff timer would fire after the user
312+
// explicitly disconnected and silently reopen the connection.
313+
reconnectTimer.cancel();
314+
306315
this.conn = null;
307316
await _connectionSubscription?.cancel();
308317
_connectionSubscription = null;

packages/realtime_client/lib/src/retry_timer.dart

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,7 @@
11
import 'dart:async';
22

3+
import 'package:meta/meta.dart';
4+
35
typedef TimerCallback = void Function();
46
typedef TimerCalculation = int Function(int tries);
57

@@ -34,7 +36,14 @@ class RetryTimer {
3436
/// Cancels any previous timer and reset tries
3537
void reset() {
3638
_tries = 0;
37-
if (_timer != null) _timer!.cancel();
39+
cancel();
40+
}
41+
42+
/// Cancels any scheduled timer without resetting tries, so exponential
43+
/// backoff is preserved across reconnect attempts.
44+
@internal
45+
void cancel() {
46+
_timer?.cancel();
3847
}
3948

4049
/// Cancels any previous scheduleTimeout and schedules callback

packages/realtime_client/test/socket_test.dart

Lines changed: 149 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -322,6 +322,155 @@ void main() {
322322
expect(socket.conn, isNull);
323323
});
324324

325+
test('cancels a pending reconnect after an unexpected drop', () async {
326+
final streamController = StreamController<dynamic>();
327+
final mockedSocketChannel = MockIOWebSocketChannel();
328+
final mockedSink = MockWebSocketSink();
329+
var connectCount = 0;
330+
331+
when(() => mockedSocketChannel.ready).thenAnswer((_) => Future.value());
332+
when(() => mockedSocketChannel.sink).thenReturn(mockedSink);
333+
when(() => mockedSocketChannel.stream)
334+
.thenAnswer((_) => streamController.stream);
335+
when(() => mockedSink.close(any(), any()))
336+
.thenAnswer((_) => Future.value());
337+
when(() => mockedSink.close()).thenAnswer((_) => Future.value());
338+
339+
final mockedSocket = RealtimeClient(
340+
socketEndpoint,
341+
// Reconnect almost immediately so the test doesn't wait for the
342+
// default backoff.
343+
reconnectAfterMs: (tries) => 20,
344+
transport: (url, headers) {
345+
connectCount++;
346+
return mockedSocketChannel;
347+
},
348+
);
349+
350+
await mockedSocket.connect();
351+
expect(connectCount, 1);
352+
353+
// Simulate the server dropping the connection: `onDone` fires, the socket
354+
// is marked closed and a reconnect is scheduled.
355+
await streamController.close();
356+
await Future.delayed(const Duration(milliseconds: 5));
357+
expect(mockedSocket.connState, SocketStates.closed);
358+
359+
// The user disconnects explicitly while the socket is already closed.
360+
await mockedSocket.disconnect();
361+
362+
// Wait past the reconnect delay; the scheduled reconnect must be canceled.
363+
await Future.delayed(const Duration(milliseconds: 60));
364+
expect(connectCount, 1,
365+
reason: 'must not reopen after a user disconnect');
366+
});
367+
368+
test('reconnects on a manual connect() after an unexpected drop', () async {
369+
final firstController = StreamController<dynamic>();
370+
final firstChannel = MockIOWebSocketChannel();
371+
final firstSink = MockWebSocketSink();
372+
when(() => firstChannel.ready).thenAnswer((_) => Future.value());
373+
when(() => firstChannel.sink).thenReturn(firstSink);
374+
when(() => firstChannel.stream).thenAnswer((_) => firstController.stream);
375+
when(() => firstSink.close(any(), any()))
376+
.thenAnswer((_) => Future.value());
377+
when(() => firstSink.close()).thenAnswer((_) => Future.value());
378+
379+
final secondController = StreamController<dynamic>();
380+
addTearDown(secondController.close);
381+
final secondChannel = MockIOWebSocketChannel();
382+
final secondSink = MockWebSocketSink();
383+
when(() => secondChannel.ready).thenAnswer((_) => Future.value());
384+
when(() => secondChannel.sink).thenReturn(secondSink);
385+
when(() => secondChannel.stream)
386+
.thenAnswer((_) => secondController.stream);
387+
when(() => secondSink.close(any(), any()))
388+
.thenAnswer((_) => Future.value());
389+
when(() => secondSink.close()).thenAnswer((_) => Future.value());
390+
391+
var connectCount = 0;
392+
final mockedSocket = RealtimeClient(
393+
socketEndpoint,
394+
// Large delay so the automatic reconnect stays dormant during the
395+
// test and the manual reconnect below is what reopens the socket.
396+
reconnectAfterMs: (tries) => 100000,
397+
transport: (url, headers) {
398+
connectCount++;
399+
return connectCount == 1 ? firstChannel : secondChannel;
400+
},
401+
);
402+
403+
await mockedSocket.connect();
404+
expect(connectCount, 1);
405+
expect(mockedSocket.connState, SocketStates.open);
406+
407+
// Simulate the server dropping the connection.
408+
await firstController.close();
409+
await Future.delayed(const Duration(milliseconds: 5));
410+
expect(mockedSocket.connState, SocketStates.closed);
411+
412+
// A manual reconnect must open a fresh connection instead of being a
413+
// no-op because `conn` still references the dropped socket.
414+
await mockedSocket.connect();
415+
expect(connectCount, 2,
416+
reason: 'manual connect() must reconnect after a drop');
417+
expect(mockedSocket.connState, SocketStates.open);
418+
419+
await mockedSocket.disconnect();
420+
});
421+
422+
test('grows the reconnect backoff across failed attempts', () async {
423+
final triesSeen = <int>[];
424+
var attempt = 0;
425+
426+
final failingChannel = MockIOWebSocketChannel();
427+
final failingSink = MockWebSocketSink();
428+
when(() => failingChannel.ready)
429+
.thenAnswer((_) => Future.error(Exception('unavailable')));
430+
when(() => failingChannel.sink).thenReturn(failingSink);
431+
when(() => failingSink.close(any(), any()))
432+
.thenAnswer((_) => Future.value());
433+
when(() => failingSink.close()).thenAnswer((_) => Future.value());
434+
435+
final successController = StreamController<dynamic>();
436+
addTearDown(successController.close);
437+
final successChannel = MockIOWebSocketChannel();
438+
final successSink = MockWebSocketSink();
439+
when(() => successChannel.ready).thenAnswer((_) => Future.value());
440+
when(() => successChannel.sink).thenReturn(successSink);
441+
when(() => successChannel.stream)
442+
.thenAnswer((_) => successController.stream);
443+
when(() => successSink.close(any(), any()))
444+
.thenAnswer((_) => Future.value());
445+
when(() => successSink.close()).thenAnswer((_) => Future.value());
446+
447+
final mockedSocket = RealtimeClient(
448+
socketEndpoint,
449+
reconnectAfterMs: (tries) {
450+
triesSeen.add(tries);
451+
return 10;
452+
},
453+
transport: (url, headers) {
454+
attempt++;
455+
// Fail the first attempts so the client keeps retrying, then let it
456+
// connect so the reconnect loop stops.
457+
return attempt <= 3 ? failingChannel : successChannel;
458+
},
459+
);
460+
461+
await mockedSocket.connect();
462+
463+
// Wait for the failing attempts to cycle and the fourth to connect.
464+
await Future.delayed(const Duration(milliseconds: 100));
465+
466+
// The retry counter must grow (1, 2, 3, ...) across reconnect attempts
467+
// instead of being reset to 1 on every `disconnect()` in `_reconnect`.
468+
expect(triesSeen.take(3), [1, 2, 3]);
469+
expect(mockedSocket.connState, SocketStates.open);
470+
471+
await mockedSocket.disconnect();
472+
});
473+
325474
test('disconnecting an open connection', () async {
326475
await socket.connect();
327476
expect(socket.connState, SocketStates.open);

0 commit comments

Comments
 (0)