Detect network changes (e.g. establishing VPN connection) #5708
Conversation
src/script/event/WebSocketService.js
Outdated
| return { | ||
| PING_INTERVAL: z.util.TimeUtil.UNITS_IN_MILLIS.SECOND * 30, | ||
| PING_INTERVAL: z.util.TimeUtil.UNITS_IN_MILLIS.SECOND * 5, | ||
| PING_INTERVAL_THRESHOLD: z.util.TimeUtil.UNITS_IN_MILLIS.SECOND * 2, |
There was a problem hiding this comment.
Please note, I increased the amount of 'ping' requests, because we have to wait for the PING_INTERVAL_THRESHOLD before Wire detects network loss, and it was a whole minute. Now it is only 10 seconds, which I personally find acceptable.
The amount of traffic is not increased as much, because we also changed from sending 36 bytes per message (Wire is so much nicer with internet!) to only 4 (ping).
|
@maximbaz Thanks for the PR. Have you considered the following implementation strategy?
|
|
The current implementation is essentially the same:
So essentially when we are about to send the 3rd PING and there still have been no PONG, we show the "no internet connection" bar and attempt to reconnect. |
|
Could the stack idea replace the "received within |
| if (this._pingHasExperiencedSuspiciousInactivity()) { | ||
| if (this.hasAlreadySentUnansweredPing) { | ||
| this.logger.warn('Ping interval check failed'); | ||
| return this.reconnect(WebSocketService.CHANGE_TRIGGER.PING_INTERVAL); |
There was a problem hiding this comment.
Done @bennyn, this is now based on a counter (used boolean for readability), a second ping in the stack now causes reconnect as you can see right in this snippet
There was a problem hiding this comment.
Quick question that I cannot seem to answer just looking at the code.
Will the non-time-based solution work in case of the laptop being put in sleep mode?
Previously the time would also tell us if the computer is not active (and thus not sending any ping). This was why time is an important component here.
I believe with this solution, if the computer is put to sleep we will not reconnect automatically.
Now this scenario might actually be covered by the fact that we are going to send pings that are not going to be replied to. Can you confirm this @maximbaz ?
There was a problem hiding this comment.
I've just confirmed that both putting computer to sleep and simply shutting down wifi doesn't affect Wire, when Internet is back Wire does reconnect as expected. However I can also see that Wire is detecting "connection lost" and "connection regained", so when connection is lost, WebsocketService is reset and no pings are being sent. Once "connection regained" occurs, pings are being sent again as expected.
There was a problem hiding this comment.
Awesome!! This is a great PR @maximbaz Thanks a lot 👍
3 participants
This implements client-side support for wireapp/wire-server#561 and fixes wireapp/wire-desktop#1129
Finally no need to restart Wire after connecting to VPN! 🎉