-
Notifications
You must be signed in to change notification settings - Fork 3.5k
Follow up to reconnection callback fix / Fix reconnect methods not firing on laptop awakening #591
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
Changes from all commits
6b1bf7d
92720b6
17072ff
00826b7
d6545f1
d7187d8
8536b97
53c2471
6087cc4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
This file was deleted.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,111 @@ | ||
| import _ from 'underscore'; | ||
| import {AppState} from 'react-native'; | ||
| import NetInfo from '@react-native-community/netinfo'; | ||
| import Ion from './Ion'; | ||
| import IONKEYS from '../IONKEYS'; | ||
|
|
||
| // NetInfo.addEventListener() returns a function used to unsubscribe the | ||
| // listener so we must create a reference to it and call it in stopListeningForReconnect() | ||
| let unsubscribeFromNetInfo; | ||
| let sleepTimer; | ||
| let lastTime; | ||
| let isActive = false; | ||
| let isOffline = false; | ||
|
|
||
| // Holds all of the callbacks that need to be triggered when the network reconnects | ||
| const reconnectionCallbacks = []; | ||
|
|
||
| /** | ||
| * Loop over all reconnection callbacks and fire each one | ||
| */ | ||
| const triggerReconnectionCallbacks = _.throttle(() => { | ||
| _.each(reconnectionCallbacks, callback => callback()); | ||
| }, 5000, {trailing: false}); | ||
|
|
||
| /** | ||
| * Called when the offline status of the app changes and if the network is "reconnecting" (going from offline to online) | ||
| * then all of the reconnection callbacks are triggered | ||
| * | ||
| * @param {boolean} isCurrentlyOffline | ||
| */ | ||
| function setOfflineStatus(isCurrentlyOffline) { | ||
| Ion.merge(IONKEYS.NETWORK, {isOffline: isCurrentlyOffline}); | ||
|
|
||
| // When reconnecting, ie, going from offline to online, all the reconnection callbacks | ||
| // are triggered (this is usually Actions that need to re-download data from the server) | ||
| if (isOffline && !isCurrentlyOffline) { | ||
| triggerReconnectionCallbacks(); | ||
| } | ||
|
|
||
| isOffline = isCurrentlyOffline; | ||
| } | ||
|
|
||
| /** | ||
| * Set up the event listener for NetInfo to tell whether the user has | ||
| * internet connectivity or not. This is more reliable than the Pusher | ||
| * `disconnected` event which takes about 10-15 seconds to emit. | ||
| */ | ||
| function listenForReconnect() { | ||
| // Subscribe to the state change event via NetInfo so we can update | ||
| // whether a user has internet connectivity or not. | ||
| unsubscribeFromNetInfo = NetInfo.addEventListener((state) => { | ||
| console.debug('[NetInfo] isConnected:', state && state.isConnected); | ||
| setOfflineStatus(!state.isConnected); | ||
| }); | ||
|
|
||
| // When the app is in the background Pusher can still receive realtime updates | ||
| // for a few minutes, but eventually disconnects causing a delay when the app | ||
| // returns from the background. So, if we are returning from the background | ||
| // and we are online we should trigger our reconnection callbacks. | ||
| AppState.addEventListener('change', (state) => { | ||
| console.debug('[AppState] state changed:', state); | ||
| const nextStateIsActive = state === 'active'; | ||
|
|
||
| // We are moving from not active to active and we are online so fire callbacks | ||
| if (!isOffline && nextStateIsActive && !isActive) { | ||
| triggerReconnectionCallbacks(); | ||
| } | ||
|
|
||
| isActive = nextStateIsActive; | ||
| }); | ||
|
|
||
| // When a device is put to sleep, NetInfo is not always able to detect | ||
| // when connectivity has been lost. As a failsafe we will capture the time | ||
| // every two seconds and if the last time recorded is greater than 5 seconds | ||
| // we know that the computer has been asleep. | ||
| lastTime = (new Date()).getTime(); | ||
| sleepTimer = setInterval(() => { | ||
| const currentTime = (new Date()).getTime(); | ||
| if (currentTime > (lastTime + 5000)) { | ||
| triggerReconnectionCallbacks(); | ||
| } | ||
| lastTime = currentTime; | ||
| }, 2000); | ||
|
Contributor
Author
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. Some more context on this change here -> https://github.com/Expensify/Expensify/issues/141817#issuecomment-703862314
Contributor
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. Sounds like a good thing to try! |
||
| } | ||
|
|
||
| /** | ||
| * Tear down the event listeners when we are finished with them. | ||
| */ | ||
| function stopListeningForReconnect() { | ||
| clearInterval(sleepTimer); | ||
| if (unsubscribeFromNetInfo) { | ||
| unsubscribeFromNetInfo(); | ||
| } | ||
| AppState.removeEventListener('change', () => {}); | ||
| } | ||
|
|
||
| /** | ||
| * Register callback to fire when we reconnect | ||
| * | ||
| * @param {Function} callback | ||
| */ | ||
| function onReconnect(callback) { | ||
| reconnectionCallbacks.push(callback); | ||
| } | ||
|
|
||
| export default { | ||
| setOfflineStatus, | ||
| listenForReconnect, | ||
| stopListeningForReconnect, | ||
| onReconnect, | ||
| }; | ||
Uh oh!
There was an error while loading. Please reload this page.
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.
Is it worth using setTimeout instead of setInterval (and maybe establishing this as a best practice for JS)?
This is an old "best practice" but I think it's still valid, and the idea behind it is that if there's an uncaught error that prevents
clearIntervalfrom running, this function will be running in a "loop" forever.To avoid that, I think we can use setTimeout instead like so:
We define it as a recursive function that calls setTimeout
And when we stop listening, we re-define it as a no-op
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.
That's a really interesting point. And I guess my only question would be what exactly would cause the
clearInterval()to never run in this case? We are calling it viaredirectToSignIn(), but I'm not seeing how (or which) exception would prevent it from running. And if there was such an exception then wouldn't that same exception prevent you from settingcheckConnectivityas a no-op?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.
I don't know, it's mostly a defensive measure in case we ever write a bug that causes an error that isn't caught
Duh, yeah 🤦. I was looking for the right way to do this since I haven't thought about it in a while, so clearly this isn't it, but I bet there is a way. I'll look into it, and will merge this now
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.
Oh actually, I think what happens is that if there's an uncaught exception and we don't make checkConnectivity a no-op, it doesn't really matter because we didn't call setInterval, and thus, the execution of checkConnectivity will be interrupted too. Does that make sense, and do you agree with that being how it'd work?
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.
Ok, I think I get it now! You are saying that if something inside the
checkConnectivitymethod fails then we will stop looping. Therefore it's better to use recursion here since we don't want to keep calling a method that we already know is going to fail.I think that makes sense, but another perspective might be that
setIntervalis only bad if we allow these unhandled errors to exist unhandled. Consider this case:I like this better because we don't need to no-op anything or use recursion. If we encounter an error then we just decide to stop the loop and throw the 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.
I like that idea of using a try/catch. I think that's cleaner than
setTimeout. I'd also like to see that added as a utility likesafeSetIntervalor something (though not necessary for this).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.
No, not really. I think any uncaught error can stop the execution of JS code, is that not the case with React?
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.
React is JavaScript and follows the same rules. The suggestion that an uncaught error from anywhere could somehow stop a recursive
setTimeout()I think is wrong? Timers have their own execution context and can only be "broken" if an error is thrown inside the timer.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.
Yup, not from anywhere, but if
triggerReconnectionCallbacks();or anything that that function calls throws, then the interval won't be cleared. So the try/catch you propose should work well