Skip to content

Commit a2c453d

Browse files
committed
fix: address post-review issues in SignalingModule and IsolateManager
- isolate_manager: fix connectivityNoneCounter reset — error now fires exactly once at maxConnectivityNoneRepeats; subsequent none-events are silently ignored until connectivity is restored and counter resets to 0 - main_shell: split SignalingModule construction into valid/invalid-creds branches to remove ?? '' fallbacks and make intent explicit - signaling_module: document sync:true reentrancy assumption, single-use constraint on events getter, and _errorHandled ordering invariant
1 parent 625b58c commit a2c453d

3 files changed

Lines changed: 38 additions & 10 deletions

File tree

lib/app/router/main_shell.dart

Lines changed: 14 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -84,18 +84,25 @@ class _MainShellState extends State<MainShell> with WidgetsBindingObserver {
8484
final session = context.read<AppBloc>().state.session;
8585
final coreUrl = session.coreUrl;
8686
final token = session.token;
87-
_signalingModule = SignalingModule(
88-
coreUrl: coreUrl ?? '',
89-
tenantId: session.tenantId,
90-
token: token ?? '',
91-
trustedCertificates: context.read<AppCertificates>().trustedCertificates,
92-
signalingClientFactory: defaultSignalingClientFactory,
93-
);
9487
if (coreUrl == null || token == null) {
88+
_signalingModule = SignalingModule(
89+
coreUrl: '',
90+
tenantId: session.tenantId,
91+
token: '',
92+
trustedCertificates: context.read<AppCertificates>().trustedCertificates,
93+
signalingClientFactory: defaultSignalingClientFactory,
94+
);
9595
WidgetsBinding.instance.addPostFrameCallback((_) {
9696
context.read<AppBloc>().add(const AppLogoutRequested(reason: AppLogoutReason.sessionMissed));
9797
});
9898
} else {
99+
_signalingModule = SignalingModule(
100+
coreUrl: coreUrl,
101+
tenantId: session.tenantId,
102+
token: token,
103+
trustedCertificates: context.read<AppCertificates>().trustedCertificates,
104+
signalingClientFactory: defaultSignalingClientFactory,
105+
);
99106
_signalingModule.connect();
100107
}
101108

lib/features/call/services/isolate_manager.dart

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -128,9 +128,10 @@ abstract class IsolateManager implements CallkeepBackgroundServiceDelegate {
128128
logger.warning('No internet connection detected ($connectivityNoneCounter/$maxConnectivityNoneRepeats)');
129129

130130
if (connectivityNoneCounter >= maxConnectivityNoneRepeats) {
131-
logger.severe('Max connectivity loss reached');
132-
_onSignalingError('Max connectivity loss reached');
133-
connectivityNoneCounter = 0;
131+
if (connectivityNoneCounter == maxConnectivityNoneRepeats) {
132+
logger.severe('Max connectivity loss reached');
133+
_onSignalingError('Max connectivity loss reached');
134+
}
134135
return;
135136
}
136137

lib/features/call/services/signaling_module.dart

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -98,6 +98,12 @@ class SignalingModule {
9898
// sync: true ensures events are delivered to existing listeners in the same
9999
// call stack as _emit(), preserving ordering and eliminating async-dispatch
100100
// races where a late-arriving broadcast event could duplicate a replayed one.
101+
//
102+
// Reentrancy assumption: consumers must not call connect() or disconnect()
103+
// synchronously inside an event listener. Doing so would re-enter _emit()
104+
// in the same stack frame and produce unexpected ordering. All current
105+
// consumers (CallBloc.add(), IsolateManager timer callbacks) satisfy this —
106+
// they either queue events or schedule via Timer, never calling back directly.
101107
final _controller = StreamController<SignalingModuleEvent>.broadcast(sync: true);
102108

103109
/// Events buffered since the last [connect] call. Cleared on every [connect]
@@ -123,6 +129,13 @@ class SignalingModule {
123129
/// (which fires when the underlying socket closes after an error) is
124130
/// suppressed — [_onError] already emits [SignalingConnectionFailed] and
125131
/// consumers would otherwise receive two separate reconnect triggers.
132+
///
133+
/// Ordering invariant: [_onDisconnect] for the failed socket always arrives
134+
/// before any reconnect-triggered [_connectAsync] completes, because reconnect
135+
/// is scheduled via [kSignalingClientReconnectDelay] (several seconds) while
136+
/// the socket close-ack is delivered in the current event loop iteration.
137+
/// [_onDisconnect] resets this flag to false, so a newly connected client
138+
/// starts with a clean state.
126139
bool _errorHandled = false;
127140

128141
/// Last connect error as string for deduplication.
@@ -138,6 +151,13 @@ class SignalingModule {
138151
///
139152
/// Note: [SignalingProtocolEvent] (call events) are NOT replayed — they are
140153
/// only delivered to subscribers already listening when they occur.
154+
///
155+
/// Each call to [events] creates a new independent stream backed by a
156+
/// dedicated [StreamController] and a live subscription to the internal
157+
/// broadcast controller. Call [events] exactly once per consumer and hold
158+
/// the returned [StreamSubscription] for the consumer's lifetime; calling
159+
/// it multiple times without cancelling previous subscriptions leaks
160+
/// controllers.
141161
Stream<SignalingModuleEvent> get events {
142162
// Take a snapshot of the buffer BEFORE subscribing to the live stream so
143163
// that any event arriving between snapshot and subscribe is delivered only

0 commit comments

Comments
 (0)