-
Notifications
You must be signed in to change notification settings - Fork 1.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Reconnect the network when we switch network types #8034
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,7 +1,7 @@ | ||
// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. | ||
// See LICENSE.txt for license information. | ||
|
||
import NetInfo, {type NetInfoState, type NetInfoSubscription} from '@react-native-community/netinfo'; | ||
import NetInfo, {NetInfoStateType, type NetInfoState, type NetInfoSubscription} from '@react-native-community/netinfo'; | ||
import {AppState, type AppStateStatus, type NativeEventSubscription} from 'react-native'; | ||
import BackgroundTimer from 'react-native-background-timer'; | ||
import {BehaviorSubject} from 'rxjs'; | ||
|
@@ -30,6 +30,7 @@ class WebsocketManager { | |
private connectionTimerIDs: Record<string, NodeJS.Timeout> = {}; | ||
private isBackgroundTimerRunning = false; | ||
private netConnected = false; | ||
private netType: NetInfoStateType = NetInfoStateType.none; | ||
private previousActiveState: boolean; | ||
private statusUpdatesIntervalIDs: Record<string, NodeJS.Timeout> = {}; | ||
private backgroundIntervalId: number | undefined; | ||
|
@@ -43,7 +44,9 @@ class WebsocketManager { | |
} | ||
|
||
public init = async (serverCredentials: ServerCredential[]) => { | ||
this.netConnected = Boolean((await NetInfo.fetch()).isConnected); | ||
const netInfo = await NetInfo.fetch(); | ||
this.netConnected = Boolean(netInfo.isConnected); | ||
this.netType = netInfo.type; | ||
serverCredentials.forEach( | ||
({serverUrl, token}) => { | ||
try { | ||
|
@@ -101,6 +104,7 @@ class WebsocketManager { | |
for (const url of Object.keys(this.clients)) { | ||
const client = this.clients[url]; | ||
client.close(true); | ||
client.invalidate(); | ||
this.getConnectedSubject(url).next('not_connected'); | ||
} | ||
}; | ||
|
@@ -227,35 +231,40 @@ class WebsocketManager { | |
} | ||
|
||
const isActive = appState === 'active'; | ||
this.handleStateChange(this.netConnected, isActive); | ||
this.handleStateChange(this.netConnected, this.netType, isActive); | ||
}; | ||
|
||
private onNetStateChange = (netState: NetInfoState) => { | ||
const newState = Boolean(netState.isConnected); | ||
if (this.netConnected === newState) { | ||
return; | ||
} | ||
|
||
this.handleStateChange(newState, this.previousActiveState); | ||
this.handleStateChange(newState, netState.type, this.previousActiveState); | ||
}; | ||
|
||
private handleStateChange = (currentIsConnected: boolean, currentIsActive: boolean) => { | ||
if (currentIsActive === this.previousActiveState && currentIsConnected === this.netConnected) { | ||
private handleStateChange = (currentIsConnected: boolean, currentNetType: NetInfoStateType, currentIsActive: boolean) => { | ||
if (currentIsActive === this.previousActiveState && currentIsConnected === this.netConnected && currentNetType === this.netType) { | ||
return; | ||
} | ||
|
||
this.cancelConnectTimers(); | ||
|
||
const wentBackground = this.previousActiveState && !currentIsActive; | ||
const switchedNetworks = currentIsConnected && currentNetType !== this.netType && this.netType !== 'none'; | ||
|
||
this.previousActiveState = currentIsActive; | ||
this.netConnected = currentIsConnected; | ||
this.netType = currentNetType; | ||
|
||
if (!currentIsConnected) { | ||
this.closeAll(); | ||
return; | ||
} | ||
|
||
if (switchedNetworks) { | ||
// Close all connections when we switch from (for example) vpn to wifi | ||
// to ensure we are using the right network and doesn't get stuck on | ||
// retries. | ||
this.closeAll(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. wondering if it would make sense to set here some sort of debounce time here. I say in case of potential network fluctuation. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You mean the network quickly moves between, let's say, wifi and cell? I don't expect that to happen often, but it is something we could consider. @rahimrahman thoughts on this? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 0/5. I know you can toggle the Wifi switch back and forth quickly, but I doubt that it will get picked up by netinfo to worry about putting a debounce. |
||
} | ||
|
||
if (currentIsActive) { | ||
if (this.isBackgroundTimerRunning) { | ||
BackgroundTimer.clearInterval(this.backgroundIntervalId!); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This change has to be made in the library. I added it here just for visibility and to agree on whether this is the desired change. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this should not have been merged.. we should decide if it is going to be added to the lib and include it or remove it entirely, but I'm 5/5 that should not be done here like this. FYI: @larkox There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We discussed this back in the day. We decided that we would move with this approach until we verified that it improved the behavior. If the behavior was improved, we can make the change in the library. If the behavior got worse, we can easily revert, without reverting in the library and the app. I will try to verify if this improves the behavior for the original reporter. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,13 @@ | ||
diff --git a/node_modules/@mattermost/react-native-network-client/android/src/main/java/com/mattermost/networkclient/WebSocketClientModuleImpl.kt b/node_modules/@mattermost/react-native-network-client/android/src/main/java/com/mattermost/networkclient/WebSocketClientModuleImpl.kt | ||
index 42f620b..d03cf08 100644 | ||
--- a/node_modules/@mattermost/react-native-network-client/android/src/main/java/com/mattermost/networkclient/WebSocketClientModuleImpl.kt | ||
+++ b/node_modules/@mattermost/react-native-network-client/android/src/main/java/com/mattermost/networkclient/WebSocketClientModuleImpl.kt | ||
@@ -113,7 +113,7 @@ class WebSocketClientModuleImpl(reactApplicationContext: ReactApplicationContext | ||
} | ||
|
||
try { | ||
- clients[wsUri]!!.webSocket!!.close(1000, "manual") | ||
+ clients[wsUri]!!.webSocket!!.cancel() | ||
} catch (error: Exception) { | ||
promise.reject(error) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would it be worth or necessary to handle any potential error in the case of a promise rejection?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the worse case here is if the fetch() timed-out, which is default at 15s. I wonder how often is that happening and how badly backed up is the JS thread to not get any response in 15s.
But could just safely change:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this code has been like this for a long while and (seemingly) never got us any problem, I will leave it as is for readability sake.