Reconnect the network when we switch network types #8034
Conversation
There was a problem hiding this comment.
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.
|
@larkox will you be making this changes permanent in the library, are we proceeding with this ? |
larkox
added
3: QA Review
|
After discussing with @enahum we are going to move forward with this change on the patch. That way, if something fails, the revert is way simpler. If we see this solve the issue and doesn't bring any unexpected behavior, we can make the change in the library. |
| @@ -43,7 +44,9 @@ class WebsocketManager { | |||
| } | |||
|
|
|||
| public init = async (serverCredentials: ServerCredential[]) => { | |||
| this.netConnected = Boolean((await NetInfo.fetch()).isConnected); | |||
| const netInfo = await NetInfo.fetch(); | |||
There was a problem hiding this comment.
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.
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) {
...
}
There was a problem hiding this comment.
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.
| // 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.
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.
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.
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.
|
@yasserfaraazkhan The important part of this PR is to test changing between different kinds of connection (cell, wifi, vpn...) and see if the connection is restablished relatively fast. |
yasserfaraazkhan
added
the
Build Apps for PR
There was a problem hiding this comment.
Hi @larkox ,
I've tested this with VPN+Cellular, VPN+wifi, wifi, cellular.
the server connected was establish fast. Did not see 'No connection' message in these flow for long time.
there is one scenario, I want to share.
- If we connect to VPN. (have wifi or Cellular data connection)
- While the app is open.
- Turn off the Wifi or cellular data connection (let the VPN connect still be)
- Observe the ssl error on mattermost app
Summary
When network switches (cell -> wifi for example) the network doesn't get disconnected, disconnecting the websockets.
In certain circumstances, this may lead to delays in reconnecting the websocket.
Ticket Link
May fix https://mattermost.atlassian.net/browse/MM-57692
Release Note