Skip to content

Commit e8f7384

Browse files
fix(gotrue): address review feedback on token refresh races
- Replace single _activeRefresh record with Map<String, Completer> to fix A-B-A dedup bug: refresh(A) → refresh(B) → refresh(A) now correctly deduplicates the second A call instead of enqueuing a third network request. - Bump _sessionVersion in updateUser and setInitialSession, which were writing _currentSession directly and bypassing the version guard. - Add _isDisposed flag checked in _executeRefresh to prevent mutating state or emitting events on closed stream controllers after dispose(). - Pass stack traces to completeError for better debugging. - Add A-B-A dedup test case.
1 parent 39d26e6 commit e8f7384

2 files changed

Lines changed: 93 additions & 52 deletions

File tree

packages/gotrue/lib/src/gotrue_client.dart

Lines changed: 62 additions & 52 deletions
Original file line numberDiff line numberDiff line change
@@ -56,23 +56,26 @@ class GoTrueClient {
5656

5757
Timer? _autoRefreshTicker;
5858

59-
/// Monotonically increasing counter, incremented on every session write
60-
/// ([_saveSession] / [_removeSession]). Used inside [_executeRefresh] to
61-
/// detect that the session changed while a network request was in-flight,
62-
/// so the stale refresh result can be discarded instead of overwriting a
63-
/// newer session (e.g. from a concurrent signIn or signOut).
59+
/// Monotonically increasing counter, incremented on every session write.
60+
/// Used inside [_executeRefresh] to detect that the session changed while
61+
/// a network request was in-flight, so the stale refresh result can be
62+
/// discarded instead of overwriting a newer session.
6463
int _sessionVersion = 0;
6564

6665
/// Serial future chain for refresh operations. Each call to
6766
/// [_callRefreshToken] appends via `.then()` so that refreshes with
6867
/// *different* tokens execute sequentially rather than overlapping.
69-
/// Same-token calls still de-duplicate via [_activeRefresh].
68+
/// Same-token calls still de-duplicate via [_pendingRefreshes].
7069
Future<void> _pendingRefreshOperation = Future.value();
7170

72-
/// Tracks the in-flight refresh for de-duplication. When non-null, a
73-
/// concurrent call with the *same* [refreshToken] returns the existing
74-
/// [Completer.future] instead of starting a second network request.
75-
({String refreshToken, Completer<AuthResponse> completer})? _activeRefresh;
71+
/// Tracks all pending (queued or in-flight) refreshes keyed by token.
72+
/// Concurrent calls with the same token return the existing
73+
/// [Completer.future] instead of enqueuing a duplicate refresh.
74+
final Map<String, Completer<AuthResponse>> _pendingRefreshes = {};
75+
76+
/// Set by [dispose] to prevent [_executeRefresh] from mutating state
77+
/// or emitting events on closed stream controllers.
78+
bool _isDisposed = false;
7679

7780
JWKSet? _jwks;
7881
DateTime? _jwksCachedAt;
@@ -788,6 +791,7 @@ class GoTrueClient {
788791
);
789792
final userResponse = UserResponse.fromJson(response);
790793

794+
_sessionVersion++;
791795
_currentSession = currentSession?.copyWith(user: userResponse.user);
792796
notifyAllSubscribers(AuthChangeEvent.userUpdated);
793797

@@ -1104,6 +1108,7 @@ class GoTrueClient {
11041108
throw notifyException(AuthException('Initial session is missing data.'));
11051109
}
11061110

1111+
_sessionVersion++;
11071112
_currentSession = session;
11081113
notifyAllSubscribers(AuthChangeEvent.initialSession);
11091114
}
@@ -1362,50 +1367,46 @@ class GoTrueClient {
13621367

13631368
@mustCallSuper
13641369
void dispose() {
1370+
_isDisposed = true;
13651371
_onAuthStateChangeController.close();
13661372
_onAuthStateChangeControllerSync.close();
13671373
_broadcastChannel?.close();
13681374
_broadcastChannelSubscription?.cancel();
1369-
final active = _activeRefresh;
1370-
if (active != null && !active.completer.isCompleted) {
1371-
active.completer.completeError(AuthException('Disposed'));
1375+
for (final completer in _pendingRefreshes.values) {
1376+
if (!completer.isCompleted) {
1377+
completer.completeError(AuthException('Disposed'));
1378+
}
13721379
}
1373-
_activeRefresh = null;
1380+
_pendingRefreshes.clear();
13741381
_autoRefreshTicker?.cancel();
13751382
}
13761383

13771384
/// Generates a new JWT.
13781385
///
1379-
/// Concurrent calls with the **same** [refreshToken] are de-duplicated: only
1380-
/// the first starts a network request; subsequent callers receive the same
1381-
/// [Future]. Calls with **different** tokens are serialised via
1386+
/// Concurrent calls with the **same** [refreshToken] are de-duplicated:
1387+
/// only the first enqueues a network request; subsequent callers receive
1388+
/// the same [Future]. Calls with **different** tokens are serialised via
13821389
/// [_pendingRefreshOperation] so they never overlap.
13831390
///
1384-
/// After the network round-trip, the result is only applied (session saved,
1385-
/// [AuthChangeEvent.tokenRefreshed] emitted) when [_sessionVersion] has not
1386-
/// changed — meaning no sign-in, sign-out, or other session mutation
1387-
/// occurred while the request was in-flight.
1391+
/// After the network round-trip, the result is only applied (session
1392+
/// saved, [AuthChangeEvent.tokenRefreshed] emitted) when
1393+
/// [_sessionVersion] has not changed — meaning no sign-in, sign-out,
1394+
/// or other session mutation occurred while the request was in-flight.
13881395
Future<AuthResponse> _callRefreshToken(String refreshToken) {
1389-
// De-duplicate: if a refresh for the same token is already in-flight,
1390-
// share its future.
1391-
final active = _activeRefresh;
1392-
if (active != null && active.refreshToken == refreshToken) {
1393-
_log.finer(
1394-
"Don't call refresh token, already in progress for same token");
1395-
return active.completer.future;
1396+
// De-duplicate: return existing future if this token is already
1397+
// pending (queued or in-flight).
1398+
final existing = _pendingRefreshes[refreshToken];
1399+
if (existing != null) {
1400+
_log.finer('Refresh already pending for this token');
1401+
return existing.future;
13961402
}
13971403

13981404
final completer = Completer<AuthResponse>();
1399-
1400-
// Prevent unhandled-error if nobody awaits the future.
14011405
completer.future.ignore();
1406+
_pendingRefreshes[refreshToken] = completer;
14021407

1403-
// Register immediately so that a synchronous second call with the same
1404-
// token hits the de-dup check above rather than creating a second entry
1405-
// in the serial queue.
1406-
_activeRefresh = (refreshToken: refreshToken, completer: completer);
1407-
1408-
// Chain onto the serial queue so different-token refreshes never overlap.
1408+
// Chain onto the serial queue so different-token refreshes
1409+
// never overlap.
14091410
_pendingRefreshOperation = _pendingRefreshOperation
14101411
.then((_) => _executeRefresh(refreshToken, completer))
14111412
.catchError((_) {});
@@ -1419,25 +1420,30 @@ class GoTrueClient {
14191420
String refreshToken,
14201421
Completer<AuthResponse> completer,
14211422
) async {
1423+
if (_isDisposed) return;
1424+
14221425
final versionBeforeRefresh = _sessionVersion;
14231426

14241427
try {
14251428
_log.fine('Refresh access token');
14261429

14271430
final data = await _refreshAccessToken(refreshToken);
14281431

1432+
if (_isDisposed) return;
1433+
14291434
final session = data.session;
14301435
if (session == null) {
14311436
throw AuthSessionMissingException();
14321437
}
14331438

1434-
// Guard: if the session was mutated while we were awaiting the network
1435-
// request (e.g. a concurrent signIn or signOut), discard this result
1436-
// to avoid overwriting the newer session.
1439+
// Guard: if the session was mutated while we were awaiting
1440+
// the network request (e.g. a concurrent signIn or signOut),
1441+
// discard this result to avoid overwriting the newer session.
14371442
if (_sessionVersion != versionBeforeRefresh) {
14381443
_log.fine(
1439-
'Session changed during refresh (version $versionBeforeRefresh → '
1440-
'$_sessionVersion). Discarding stale refresh result.',
1444+
'Session changed during refresh '
1445+
'(version $versionBeforeRefresh → $_sessionVersion). '
1446+
'Discarding stale refresh result.',
14411447
);
14421448
if (!completer.isCompleted) completer.complete(data);
14431449
return;
@@ -1448,25 +1454,29 @@ class GoTrueClient {
14481454
if (!completer.isCompleted) completer.complete(data);
14491455
} on AuthException catch (error, stack) {
14501456
if (error is! AuthRetryableFetchException) {
1451-
// Only remove the session if it hasn't been replaced while we were
1452-
// refreshing — otherwise we'd sign out a user who just signed in.
1453-
if (_sessionVersion == versionBeforeRefresh) {
1457+
// Only remove the session if it hasn't been replaced
1458+
// while we were refreshing — otherwise we'd sign out a
1459+
// user who just signed in.
1460+
if (!_isDisposed && _sessionVersion == versionBeforeRefresh) {
14541461
_removeSession();
14551462
notifyAllSubscribers(AuthChangeEvent.signedOut);
14561463
}
1457-
} else {
1464+
} else if (!_isDisposed) {
14581465
notifyException(error, stack);
14591466
}
14601467

1461-
if (!completer.isCompleted) completer.completeError(error);
1468+
if (!completer.isCompleted) {
1469+
completer.completeError(error, stack);
1470+
}
14621471
} catch (error, stack) {
1463-
if (!completer.isCompleted) completer.completeError(error);
1464-
notifyException(error, stack);
1465-
} finally {
1466-
// Only clear if this is still the active refresh.
1467-
if (_activeRefresh?.completer == completer) {
1468-
_activeRefresh = null;
1472+
if (!completer.isCompleted) {
1473+
completer.completeError(error, stack);
14691474
}
1475+
if (!_isDisposed) {
1476+
notifyException(error, stack);
1477+
}
1478+
} finally {
1479+
_pendingRefreshes.remove(refreshToken);
14701480
}
14711481
}
14721482

packages/gotrue/test/src/token_refresh_race_test.dart

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -235,6 +235,37 @@ void main() {
235235
expect(completionOrder, ['A', 'B']);
236236
});
237237

238+
test(
239+
'A-B-A pattern: repeated token request deduplicates '
240+
'even when another token is queued', () async {
241+
mockClient.tokenGate = Completer<void>();
242+
243+
// refresh(A) — in-flight, blocked on gate
244+
final futureA1 = client.setSession('token-A');
245+
await Future<void>.delayed(Duration.zero);
246+
247+
// refresh(B) — queued behind A
248+
final futureB = client.setSession('token-B');
249+
250+
// refresh(A) again — must dedup with the pending A,
251+
// not enqueue a third refresh
252+
final futureA2 = client.setSession('token-A');
253+
254+
mockClient.tokenGate!.complete();
255+
256+
final results =
257+
await Future.wait([futureA1, futureB, futureA2]);
258+
259+
// Both A calls get the same access token
260+
expect(
261+
results[0].session?.accessToken,
262+
results[2].session?.accessToken,
263+
);
264+
265+
// Only 2 HTTP requests: one for A, one for B
266+
expect(mockClient.tokenRequestCount, 2);
267+
});
268+
238269
test('dispose completes active refresh with error', () async {
239270
mockClient.tokenGate = Completer<void>();
240271

0 commit comments

Comments
 (0)