Skip to content
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

Merged
merged 2 commits into from
Oct 29, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 19 additions & 10 deletions app/managers/websocket_manager.ts
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';
Expand Down Expand Up @@ -29,6 +29,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;
Expand All @@ -42,7 +43,9 @@ class WebsocketManager {
}

public init = async (serverCredentials: ServerCredential[]) => {
this.netConnected = Boolean((await NetInfo.fetch()).isConnected);
const netInfo = await NetInfo.fetch();
Copy link
Contributor

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?

Copy link
Contributor

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:

  if (netInfo) {
    ...
  }

Copy link
Contributor Author

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.

this.netConnected = Boolean(netInfo.isConnected);
this.netType = netInfo.type;
serverCredentials.forEach(
({serverUrl, token}) => {
try {
Expand Down Expand Up @@ -100,6 +103,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');
}
};
Expand Down Expand Up @@ -226,35 +230,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();
Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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?

Copy link
Contributor

Choose a reason for hiding this comment

The 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!);
Expand Down
13 changes: 13 additions & 0 deletions patches/@mattermost+react-native-network-client+1.6.0.patch
Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

Copy link
Contributor

Choose a reason for hiding this comment

The 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

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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)
}