Reconnect on network change #53
Conversation
src/reconnection.ts
Outdated
| @@ -49,6 +49,7 @@ export class ReconnectManager<PeerMetadata, TrackMetadata> { | |||
| private reconnectTimeoutId: NodeJS.Timeout | null = null; | |||
| private reconnectFailedNotificationSend: boolean = false; | |||
| private ongoingReconnection: boolean = false; | |||
| private reconnectionStartedNotificationSend: boolean = false; | |||
There was a problem hiding this comment.
| private reconnectionStartedNotificationSend: boolean = false; | |
| private reconnectionStartedNotificationSent: boolean = false; |
There was a problem hiding this comment.
Also there are now three boolean variables like this: reconnectFailedNotificationSend and reconnectionStartedNotificationSend and ongoingReconnection
I wonder if we can merge them into one variable that keeps the current state of reconnection?
There was a problem hiding this comment.
I've removed these variables and introduced a simple state machine
export type ReconnectionStatus = 'reconnecting' | 'idle' | 'error';
src/FishjamClient.ts
Outdated
| /** Emitted when the process of reconnection starts */ | ||
| reconnectionStarted: () => void; | ||
|
|
||
| /** Emitted when on successful reconnection */ |
src/FishjamClient.ts
Outdated
| reconnected: () => void; | ||
|
|
||
| /** Emitted when the maximum number of reconnection retries is reached */ | ||
| reconnectionFailed: () => void; |
There was a problem hiding this comment.
Maybe sth like reconnectionRetriesLimitReached? 🤔
src/FishjamClient.ts
Outdated
| if (this.status === 'initialized') { | ||
| this.disconnect(); | ||
| await this.disconnect(); |
There was a problem hiding this comment.
Maybe it would be worth to investigate if this rule can be used to detect issues like this one
There was a problem hiding this comment.
In general, I absolutely agree with you; we should add this rule. What's more, we should make more functions async, like connect, but I don't want to introduce such changes in this PR.
In this particular line, it's a relic of my previous attempts. The disconnect() function is not async.
src/reconnection.ts
Outdated
| }; | ||
| } | ||
|
|
||
| public getOngoingReconnectionStatus(): boolean { |
There was a problem hiding this comment.
WDYT
| public getOngoingReconnectionStatus(): boolean { | |
| public isReconnecting(): boolean { |
| for await (const element of this.lastLocalEndpoint.tracks) { | ||
| const [_, track] = element; |
There was a problem hiding this comment.
I would like to discuss this part of code, as I would like to understand something.
Description
This PR:
reconnectionStarted,reconnected,reconnectionFailedongoingReconnectionstatusRTCPeerConnectionafter successful reconnectionpeer already connectedauth join reasonMotivation and Context
It fixes the reconnection mechanism when the user changes the network (e.g., from Wi-Fi to a hotspot).
Types of changes