From 1f19665b240d636b64f31d9116ef1bc431097886 Mon Sep 17 00:00:00 2001 From: Peter Velkov Date: Fri, 18 Feb 2022 22:51:58 +0200 Subject: [PATCH 01/15] Delete libs/NetInfo - no longer needed libs/NetInfo served to provide shim for Electron due to an issue The issue is now resolved and this is no longer needed --- src/libs/NetInfo/index.desktop.js | 51 ------------------------------- src/libs/NetInfo/index.js | 3 -- src/libs/NetworkConnection.js | 2 +- 3 files changed, 1 insertion(+), 55 deletions(-) delete mode 100644 src/libs/NetInfo/index.desktop.js delete mode 100644 src/libs/NetInfo/index.js diff --git a/src/libs/NetInfo/index.desktop.js b/src/libs/NetInfo/index.desktop.js deleted file mode 100644 index 9c80a63363bb..000000000000 --- a/src/libs/NetInfo/index.desktop.js +++ /dev/null @@ -1,51 +0,0 @@ -/** - * This is a minimal shim for NetInfo on Electron / desktop. It's necessary since the NetworkInformation API exists, - * but does not work as expected in Electron environments since the navigator.connection.type returns "unknown". - * - * See: https://github.com/react-native-netinfo/react-native-netinfo/issues/450 - */ - -let callback; -const connection = window.navigator.connection; - -/** - * Calls the callback with the status - */ -function onStatusChange() { - callback({isConnected: window.navigator.onLine}); -} - -/** - * Unsubscribe events - */ -function unsubscribe() { - if (connection) { - connection.removeEventListener('change', onStatusChange); - } else { - window.removeEventListener('online', onStatusChange); - window.removeEventListener('offline', onStatusChange); - } -} - -/** - * @param {Function} cb - * @return {Function} method to unsubscribe - */ -function addEventListener(cb) { - callback = cb; - - if (connection) { - connection.addEventListener('change', onStatusChange); - } else { - window.addEventListener('online', onStatusChange); - window.addEventListener('offline', onStatusChange); - } - - // We initialize the status once when first subscribing - onStatusChange(); - return unsubscribe; -} - -export default { - addEventListener, -}; diff --git a/src/libs/NetInfo/index.js b/src/libs/NetInfo/index.js deleted file mode 100644 index 93af543cc636..000000000000 --- a/src/libs/NetInfo/index.js +++ /dev/null @@ -1,3 +0,0 @@ -import NetInfo from '@react-native-community/netinfo'; - -export default NetInfo; diff --git a/src/libs/NetworkConnection.js b/src/libs/NetworkConnection.js index e73a88fcb688..2dd7736696e1 100644 --- a/src/libs/NetworkConnection.js +++ b/src/libs/NetworkConnection.js @@ -1,5 +1,5 @@ import _ from 'underscore'; -import NetInfo from './NetInfo'; +import NetInfo from '@react-native-community/netinfo'; import AppStateMonitor from './AppStateMonitor'; import promiseAllSettled from './promiseAllSettled'; import Log from './Log'; From d1181e27a55646abe88f7211d6c2cc219e861c75 Mon Sep 17 00:00:00 2001 From: Peter Velkov Date: Fri, 18 Feb 2022 22:55:04 +0200 Subject: [PATCH 02/15] Update connectivity check with `isInternetReachable` --- src/libs/NetworkConnection.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/libs/NetworkConnection.js b/src/libs/NetworkConnection.js index 2dd7736696e1..480d286b8efd 100644 --- a/src/libs/NetworkConnection.js +++ b/src/libs/NetworkConnection.js @@ -57,8 +57,8 @@ 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) => { - Log.info(`[NetworkConnection] NetInfo isConnected: ${state && state.isConnected}`); - setOfflineStatus(!state.isConnected); + Log.info(`[NetworkConnection] NetInfo isInternetReachable: ${state.isInternetReachable}`); + setOfflineStatus(!state.isInternetReachable); }); } From fc4a58cd576a8d6900590f8c26a950205d6abf06 Mon Sep 17 00:00:00 2001 From: Peter Velkov Date: Fri, 18 Feb 2022 23:09:45 +0200 Subject: [PATCH 03/15] Update NetInfo to v8 Regarding the breaking change - we're not using SSID information - it doesn't affect us v8 Includes the following fixes: 8.0.0 (2022-02-10) BREAKING CHANGES it's possible this is a breaking change, depending on your app use case. If you relied on iOS SSID information and met Apple's strict criteria for accessing SSID, you need to set the new config value shouldFetchWiFiSSID to true. If you set it to true and do not meet the criteria your app may crash due to a memory leak. All versions prior to 7.1.12 would attempt to fetch the information regardless of permission, leak memory, and possibly crash. This change avoids that crash. Bug Fixes ios: avoid memory leak from ssid APIs by adding explicit config (#560) (fbf7c15), closes #420 7.1.11 (2022-02-08) Bug Fixes windows: fix race condition in WiFi connection details feature (#568) (0cd8132) 7.1.10 (2022-02-07) Bug Fixes android: use registerDefaultNetworkCallback so toggling airplane mode works (#571) (e8af2de) 7.1.9 (2022-01-26) Bug Fixes android: count native listeners / correctly disable listener if count == 0 (#569) (5ae16f6) 7.1.8 (2022-01-25) Bug Fixes windows: refactor implementation to avoid crashes (#564) (cc4bfa3) 7.1.7 (2021-12-20) Bug Fixes android: populate network value during initial module startup (#553) (c05080f) 7.1.6 (2021-12-13) Bug Fixes android: avoid send event when has no listener (#548) (cad47d8) 7.1.5 (2021-12-09) Bug Fixes android: use method-local ref to instance var for multi-thread safety #549 (#550) (81bbc87) 7.1.4 (2021-12-07) Bug Fixes android: try async state fetch as stale state workaround (#547) (937cf48), closes #542 --- package-lock.json | 6 +++--- package.json | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/package-lock.json b/package-lock.json index 5ed8a4d4a627..6981622de1af 100644 --- a/package-lock.json +++ b/package-lock.json @@ -6663,9 +6663,9 @@ "dev": true }, "@react-native-community/netinfo": { - "version": "7.1.3", - "resolved": "https://registry.npmjs.org/@react-native-community/netinfo/-/netinfo-7.1.3.tgz", - "integrity": "sha512-E8q3yuges6NYhrXBDQdzwCnG0bBQXATRjs6fpTjRJ37nURmdpe4HLq1awvooG4ymGTpZhrx4elcs/aUM7PTgrA==" + "version": "8.0.0", + "resolved": "https://registry.npmjs.org/@react-native-community/netinfo/-/netinfo-8.0.0.tgz", + "integrity": "sha512-8cjkbOWe55vzzc64hfjDv6GWSY8+kfEnxRbwTf9l3hFYDIUMRmMoW+SwxE+QoAfMY32nbEERDy68iev3busRFQ==" }, "@react-native-community/progress-bar-android": { "version": "1.0.4", diff --git a/package.json b/package.json index 28610c6e340d..a1d0faf2a717 100644 --- a/package.json +++ b/package.json @@ -46,7 +46,7 @@ "@react-native-community/cli": "6.2.0", "@react-native-community/clipboard": "^1.5.1", "@react-native-community/datetimepicker": "^3.5.2", - "@react-native-community/netinfo": "^7.1.3", + "@react-native-community/netinfo": "^8.0.0", "@react-native-community/progress-bar-android": "^1.0.4", "@react-native-community/progress-view": "^1.2.3", "@react-native-firebase/analytics": "^12.3.0", From 3b528753886c9e3c6d2884496a6d621aaa5bb45f Mon Sep 17 00:00:00 2001 From: Peter Velkov Date: Fri, 18 Feb 2022 23:38:18 +0200 Subject: [PATCH 04/15] Update Podfile.lock for react-native-netinfo --- ios/Podfile.lock | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/ios/Podfile.lock b/ios/Podfile.lock index fe5225951dcc..5fab81d28dc2 100644 --- a/ios/Podfile.lock +++ b/ios/Podfile.lock @@ -445,7 +445,7 @@ PODS: - React-Core - react-native-image-picker (4.1.2): - React-Core - - react-native-netinfo (7.1.3): + - react-native-netinfo (8.0.0): - React-Core - react-native-pdf (6.2.2): - React-Core @@ -934,7 +934,7 @@ SPEC CHECKSUMS: react-native-document-picker: 772d04a4bc5c35da9abe27b08ac271420ae3f9ef react-native-flipper: cd9eabd8917104c1bbdca2621717cdca3b2addef react-native-image-picker: f45729c43d4f854508ab25c0d0f0f711a2a8a267 - react-native-netinfo: 3a61a486f2329f5884753fd5bab21450a535d97c + react-native-netinfo: 0124c0695373fce63cea24aeebb97ab2d237947a react-native-pdf: 4b5a9e4465a6a3b399e91dc4838eb44ddf716d1f react-native-performance: 8edfa2bbc9a2af4a02f01d342118e413a95145e0 react-native-plaid-link-sdk: 9e0ebdaed648a237b36d5f6f6292b5147af92da7 From fa198f602d3fe2fef26393ddc11935e208ed5bc2 Mon Sep 17 00:00:00 2001 From: Peter Velkov Date: Sat, 19 Feb 2022 00:42:57 +0200 Subject: [PATCH 05/15] Use api url for internet checks This is primarily to help local development and desktop By default on web (desktop) the url for checking is `/` And for local testing you'll always hit it matching localhost:8080 For Desktop since the app is served from electron - requesting `/` would respond with OK even if there's no internet --- src/libs/NetworkConnection.js | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/src/libs/NetworkConnection.js b/src/libs/NetworkConnection.js index 480d286b8efd..2f83d30e8407 100644 --- a/src/libs/NetworkConnection.js +++ b/src/libs/NetworkConnection.js @@ -4,6 +4,7 @@ import AppStateMonitor from './AppStateMonitor'; import promiseAllSettled from './promiseAllSettled'; import Log from './Log'; import * as Network from './actions/Network'; +import CONFIG from '../CONFIG'; // NetInfo.addEventListener() returns a function used to unsubscribe the // listener so we must create a reference to it and call it in stopListeningForReconnect() @@ -50,6 +51,14 @@ function setOfflineStatus(isCurrentlyOffline) { function listenForReconnect() { Log.info('[NetworkConnection] listenForReconnect called'); + NetInfo.configure({ + // By default, for web (including Electron) NetInfo uses `/` for `reachabilityUrl` + // When App is served locally or from Electron this would respond with OK even with no internet + // Using API url ensures reachability is tested over internet + reachabilityUrl: CONFIG.EXPENSIFY.URL_API_ROOT, + reachabilityTest: response => Promise.resolve(response.status === 200), + }); + unsubscribeFromAppState = AppStateMonitor.addBecameActiveListener(() => { triggerReconnectionCallbacks('app became active'); }); From 7ad9bc214ca3fe75cebdfc12eaae89e689e85a69 Mon Sep 17 00:00:00 2001 From: Peter Velkov Date: Sat, 19 Feb 2022 00:52:35 +0200 Subject: [PATCH 06/15] Only trigger offline state if we're certain there's no internet --- src/libs/NetworkConnection.js | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/libs/NetworkConnection.js b/src/libs/NetworkConnection.js index 2f83d30e8407..2b9dfb9bd291 100644 --- a/src/libs/NetworkConnection.js +++ b/src/libs/NetworkConnection.js @@ -66,8 +66,11 @@ 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) => { - Log.info(`[NetworkConnection] NetInfo isInternetReachable: ${state.isInternetReachable}`); - setOfflineStatus(!state.isInternetReachable); + Log.info('[NetworkConnection] NetInfo state', false, state); + + // state.internetReachable can be `null` - unknown, let's assume we have internet unless it's false + const connected = _.isBoolean(state.isInternetReachable) ? state.isInternetReachable : state.isConnected; + setOfflineStatus(!connected); }); } From f5a017144b64bb1f674689b46517d8511391bc8e Mon Sep 17 00:00:00 2001 From: Peter Velkov Date: Sat, 19 Feb 2022 03:24:47 +0200 Subject: [PATCH 07/15] Faster offline detection --- src/CONST/index.js | 1 + src/libs/Network.js | 9 +++++++- src/libs/NetworkConnection.js | 40 +++++++++++++++++++++++------------ 3 files changed, 35 insertions(+), 15 deletions(-) diff --git a/src/CONST/index.js b/src/CONST/index.js index a6ed085bd523..22386b484858 100755 --- a/src/CONST/index.js +++ b/src/CONST/index.js @@ -306,6 +306,7 @@ const CONST = { }, MAX_REQUEST_RETRIES: 10, PROCESS_REQUEST_DELAY_MS: 1000, + MAX_PENDING_TIME: 7 * 1000, }, HTTP_STATUS_CODE: { SUCCESS: 200, diff --git a/src/libs/Network.js b/src/libs/Network.js index 9e874ffdc332..7b164184789a 100644 --- a/src/libs/Network.js +++ b/src/libs/Network.js @@ -28,6 +28,7 @@ const [onResponse, registerResponseHandler] = createCallback(); const [onError, registerErrorHandler] = createCallback(); const [onRequestSkipped, registerRequestSkippedHandler] = createCallback(); const [getLogger, registerLogHandler] = createCallback(); +const [recheckConnectivity, registerConnectionCheckCallback] = createCallback(); /** * @param {Object} request @@ -42,8 +43,12 @@ function processRequest(request) { ? enhanceParameters(request.command, request.data) : request.data; + // If request is still in processing after this time, we might be offline + const timerId = setTimeout(() => recheckConnectivity(), CONST.NETWORK.MAX_PENDING_TIME); + onRequest(request, finalParameters); - return HttpUtils.xhr(request.command, finalParameters, request.type, request.shouldUseSecure); + return HttpUtils.xhr(request.command, finalParameters, request.type, request.shouldUseSecure) + .finally(() => clearTimeout(timerId)); } function processPersistedRequestsQueue() { @@ -233,6 +238,7 @@ function processNetworkRequestQueue() { .catch((error) => { // When the request did not reach its destination add it back the queue to be retried const shouldRetry = lodashGet(queuedRequest, 'data.shouldRetry'); + recheckConnectivity(); if (shouldRetry) { const retryCount = NetworkRequestQueue.incrementRetries(queuedRequest); getLogger().info('A retrieable request failed', false, { @@ -357,4 +363,5 @@ export { setIsReady, registerRequestSkippedHandler, registerLogHandler, + registerConnectionCheckCallback, }; diff --git a/src/libs/NetworkConnection.js b/src/libs/NetworkConnection.js index 2b9dfb9bd291..6f00be1ad333 100644 --- a/src/libs/NetworkConnection.js +++ b/src/libs/NetworkConnection.js @@ -3,7 +3,8 @@ import NetInfo from '@react-native-community/netinfo'; import AppStateMonitor from './AppStateMonitor'; import promiseAllSettled from './promiseAllSettled'; import Log from './Log'; -import * as Network from './actions/Network'; +import * as NetworkActions from './actions/Network'; +import * as NetowrkLib from './Network'; import CONFIG from '../CONFIG'; // NetInfo.addEventListener() returns a function used to unsubscribe the @@ -20,9 +21,9 @@ const reconnectionCallbacks = []; */ const triggerReconnectionCallbacks = _.throttle((reason) => { Log.info(`[NetworkConnection] Firing reconnection callbacks because ${reason}`); - Network.setIsLoadingAfterReconnect(true); + NetworkActions.setIsLoadingAfterReconnect(true); promiseAllSettled(_.map(reconnectionCallbacks, callback => callback())) - .then(() => Network.setIsLoadingAfterReconnect(false)); + .then(() => NetworkActions.setIsLoadingAfterReconnect(false)); }, 5000, {trailing: false}); /** @@ -32,7 +33,7 @@ const triggerReconnectionCallbacks = _.throttle((reason) => { * @param {Boolean} isCurrentlyOffline */ function setOfflineStatus(isCurrentlyOffline) { - Network.setIsOffline(isCurrentlyOffline); + NetworkActions.setIsOffline(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) @@ -48,9 +49,7 @@ function setOfflineStatus(isCurrentlyOffline) { * internet connectivity or not. This is more reliable than the Pusher * `disconnected` event which takes about 10-15 seconds to emit. */ -function listenForReconnect() { - Log.info('[NetworkConnection] listenForReconnect called'); - +function subscribeToNetInfo() { NetInfo.configure({ // By default, for web (including Electron) NetInfo uses `/` for `reachabilityUrl` // When App is served locally or from Electron this would respond with OK even with no internet @@ -59,19 +58,22 @@ function listenForReconnect() { reachabilityTest: response => Promise.resolve(response.status === 200), }); - unsubscribeFromAppState = AppStateMonitor.addBecameActiveListener(() => { - triggerReconnectionCallbacks('app became active'); - }); - // Subscribe to the state change event via NetInfo so we can update // whether a user has internet connectivity or not. unsubscribeFromNetInfo = NetInfo.addEventListener((state) => { Log.info('[NetworkConnection] NetInfo state', false, state); + setOfflineStatus(!state.isInternetReachable); + }); +} - // state.internetReachable can be `null` - unknown, let's assume we have internet unless it's false - const connected = _.isBoolean(state.isInternetReachable) ? state.isInternetReachable : state.isConnected; - setOfflineStatus(!connected); +function listenForReconnect() { + Log.info('[NetworkConnection] listenForReconnect called'); + + unsubscribeFromAppState = AppStateMonitor.addBecameActiveListener(() => { + triggerReconnectionCallbacks('app became active'); }); + + subscribeToNetInfo(); } /** @@ -98,6 +100,16 @@ function onReconnect(callback) { reconnectionCallbacks.push(callback); } +NetowrkLib.registerConnectionCheckCallback(_.throttle(() => { + if (!unsubscribeFromNetInfo) { + return; + } + + // Reinitializing NetInfo would recheck state and trigger listeners + unsubscribeFromNetInfo(); + subscribeToNetInfo(); +}), 10 * 1000, {trailing: false}); + export default { setOfflineStatus, listenForReconnect, From 924ef7bc5f49c53f5fbeaff78d64ec18adf0df67 Mon Sep 17 00:00:00 2001 From: Peter Velkov Date: Mon, 21 Feb 2022 15:05:09 +0200 Subject: [PATCH 08/15] Document `NetInfo.configure` serves to recheck network state --- src/libs/NetworkConnection.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libs/NetworkConnection.js b/src/libs/NetworkConnection.js index 6f00be1ad333..b53e0e638207 100644 --- a/src/libs/NetworkConnection.js +++ b/src/libs/NetworkConnection.js @@ -50,6 +50,7 @@ function setOfflineStatus(isCurrentlyOffline) { * `disconnected` event which takes about 10-15 seconds to emit. */ function subscribeToNetInfo() { + // Calling NetInfo.configure (re)checks current state. We use it to force a recheck whenever whe (re)subscribe NetInfo.configure({ // By default, for web (including Electron) NetInfo uses `/` for `reachabilityUrl` // When App is served locally or from Electron this would respond with OK even with no internet @@ -105,7 +106,6 @@ NetowrkLib.registerConnectionCheckCallback(_.throttle(() => { return; } - // Reinitializing NetInfo would recheck state and trigger listeners unsubscribeFromNetInfo(); subscribeToNetInfo(); }), 10 * 1000, {trailing: false}); From 2e9eb2277541e763b5892d9fc957859b36a8a08d Mon Sep 17 00:00:00 2001 From: Peter Velkov Date: Mon, 21 Feb 2022 15:06:21 +0200 Subject: [PATCH 09/15] Rename MAX_PENDING_TIME to MAX_PENDING_TIME_MS --- src/CONST/index.js | 2 +- src/libs/Network.js | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/CONST/index.js b/src/CONST/index.js index 22386b484858..2ec440f4262f 100755 --- a/src/CONST/index.js +++ b/src/CONST/index.js @@ -306,7 +306,7 @@ const CONST = { }, MAX_REQUEST_RETRIES: 10, PROCESS_REQUEST_DELAY_MS: 1000, - MAX_PENDING_TIME: 7 * 1000, + MAX_PENDING_TIME_MS: 7 * 1000, }, HTTP_STATUS_CODE: { SUCCESS: 200, diff --git a/src/libs/Network.js b/src/libs/Network.js index 7b164184789a..a5491ba6a426 100644 --- a/src/libs/Network.js +++ b/src/libs/Network.js @@ -44,7 +44,7 @@ function processRequest(request) { : request.data; // If request is still in processing after this time, we might be offline - const timerId = setTimeout(() => recheckConnectivity(), CONST.NETWORK.MAX_PENDING_TIME); + const timerId = setTimeout(() => recheckConnectivity(), CONST.NETWORK.MAX_PENDING_TIME_MS); onRequest(request, finalParameters); return HttpUtils.xhr(request.command, finalParameters, request.type, request.shouldUseSecure) From 8642da5d13c4f079ab3bffb4a3743edc382fae1d Mon Sep 17 00:00:00 2001 From: Peter Velkov Date: Mon, 21 Feb 2022 16:21:10 +0200 Subject: [PATCH 10/15] Add PR suggestion --- src/libs/Network.js | 5 +++-- src/libs/NetworkConnection.js | 1 + 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/src/libs/Network.js b/src/libs/Network.js index a5491ba6a426..6b2e6e7b816d 100644 --- a/src/libs/Network.js +++ b/src/libs/Network.js @@ -44,7 +44,7 @@ function processRequest(request) { : request.data; // If request is still in processing after this time, we might be offline - const timerId = setTimeout(() => recheckConnectivity(), CONST.NETWORK.MAX_PENDING_TIME_MS); + const timerId = setTimeout(recheckConnectivity, CONST.NETWORK.MAX_PENDING_TIME_MS); onRequest(request, finalParameters); return HttpUtils.xhr(request.command, finalParameters, request.type, request.shouldUseSecure) @@ -236,9 +236,10 @@ function processNetworkRequestQueue() { processRequest(queuedRequest) .then(response => onResponse(queuedRequest, response)) .catch((error) => { + recheckConnectivity(); + // When the request did not reach its destination add it back the queue to be retried const shouldRetry = lodashGet(queuedRequest, 'data.shouldRetry'); - recheckConnectivity(); if (shouldRetry) { const retryCount = NetworkRequestQueue.incrementRetries(queuedRequest); getLogger().info('A retrieable request failed', false, { diff --git a/src/libs/NetworkConnection.js b/src/libs/NetworkConnection.js index b53e0e638207..d0b24dbbef60 100644 --- a/src/libs/NetworkConnection.js +++ b/src/libs/NetworkConnection.js @@ -106,6 +106,7 @@ NetowrkLib.registerConnectionCheckCallback(_.throttle(() => { return; } + Log.info('[NetworkConnection] re-check NetInfo'); unsubscribeFromNetInfo(); subscribeToNetInfo(); }), 10 * 1000, {trailing: false}); From d9321c468b1c193bdb9be071788a788b04a90da9 Mon Sep 17 00:00:00 2001 From: Peter Velkov Date: Mon, 21 Feb 2022 16:41:45 +0200 Subject: [PATCH 11/15] Remove `trailing` from recheck `trailing: false` causes duplicate calls removing it makes just one recheck request (as intended) --- src/libs/NetworkConnection.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/libs/NetworkConnection.js b/src/libs/NetworkConnection.js index d0b24dbbef60..929ebc09cbfc 100644 --- a/src/libs/NetworkConnection.js +++ b/src/libs/NetworkConnection.js @@ -106,10 +106,10 @@ NetowrkLib.registerConnectionCheckCallback(_.throttle(() => { return; } - Log.info('[NetworkConnection] re-check NetInfo'); + Log.info('[NetworkConnection] recheck NetInfo'); unsubscribeFromNetInfo(); subscribeToNetInfo(); -}), 10 * 1000, {trailing: false}); +}), 10 * 1000); export default { setOfflineStatus, From 5cdc7efe6d7a36aea6c6e84cd6c6963efa3ac709 Mon Sep 17 00:00:00 2001 From: Peter Velkov Date: Mon, 21 Feb 2022 16:44:24 +0200 Subject: [PATCH 12/15] Fix typo --- src/libs/NetworkConnection.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libs/NetworkConnection.js b/src/libs/NetworkConnection.js index 929ebc09cbfc..9dcff1ae844a 100644 --- a/src/libs/NetworkConnection.js +++ b/src/libs/NetworkConnection.js @@ -50,7 +50,7 @@ function setOfflineStatus(isCurrentlyOffline) { * `disconnected` event which takes about 10-15 seconds to emit. */ function subscribeToNetInfo() { - // Calling NetInfo.configure (re)checks current state. We use it to force a recheck whenever whe (re)subscribe + // Calling NetInfo.configure (re)checks current state. We use it to force a recheck whenever we (re)subscribe NetInfo.configure({ // By default, for web (including Electron) NetInfo uses `/` for `reachabilityUrl` // When App is served locally or from Electron this would respond with OK even with no internet From cfec1cdd46a30fea52304aa9ad888753f73668b9 Mon Sep 17 00:00:00 2001 From: Peter Velkov Date: Fri, 25 Feb 2022 14:25:44 +0200 Subject: [PATCH 13/15] NetworkConnection only treat isInternetReachable === false as being offline --- src/libs/NetworkConnection.js | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/libs/NetworkConnection.js b/src/libs/NetworkConnection.js index 9dcff1ae844a..32b67811118f 100644 --- a/src/libs/NetworkConnection.js +++ b/src/libs/NetworkConnection.js @@ -6,6 +6,7 @@ import Log from './Log'; import * as NetworkActions from './actions/Network'; import * as NetowrkLib from './Network'; import CONFIG from '../CONFIG'; +import CONST from '../CONST'; // NetInfo.addEventListener() returns a function used to unsubscribe the // listener so we must create a reference to it and call it in stopListeningForReconnect() @@ -57,13 +58,14 @@ function subscribeToNetInfo() { // Using API url ensures reachability is tested over internet reachabilityUrl: CONFIG.EXPENSIFY.URL_API_ROOT, reachabilityTest: response => Promise.resolve(response.status === 200), + reachabilityRequestTimeout: CONST.NETWORK.MAX_PENDING_TIME_MS, }); // Subscribe to the state change event via NetInfo so we can update // whether a user has internet connectivity or not. unsubscribeFromNetInfo = NetInfo.addEventListener((state) => { - Log.info('[NetworkConnection] NetInfo state', false, state); - setOfflineStatus(!state.isInternetReachable); + Log.info('[NetworkConnection] NetInfo state change', false, state); + setOfflineStatus(state.isInternetReachable === false); }); } From 17c519eb16afbb3d1bd1cccb8b173f40df2f10c5 Mon Sep 17 00:00:00 2001 From: Peter Velkov Date: Fri, 25 Feb 2022 14:27:26 +0200 Subject: [PATCH 14/15] Replace throttle logic with lock flag The recheck was getting triggered a lot even though there was a 10sec throttle Probably because throttle also uses setTimout --- src/libs/NetworkConnection.js | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/src/libs/NetworkConnection.js b/src/libs/NetworkConnection.js index 32b67811118f..cb321713278e 100644 --- a/src/libs/NetworkConnection.js +++ b/src/libs/NetworkConnection.js @@ -13,6 +13,7 @@ import CONST from '../CONST'; let unsubscribeFromNetInfo; let unsubscribeFromAppState; let isOffline = false; +let hasPendingNetworkCheck = true; // Holds all of the callbacks that need to be triggered when the network reconnects const reconnectionCallbacks = []; @@ -66,6 +67,9 @@ function subscribeToNetInfo() { unsubscribeFromNetInfo = NetInfo.addEventListener((state) => { Log.info('[NetworkConnection] NetInfo state change', false, state); setOfflineStatus(state.isInternetReachable === false); + + // When internet state is indeterminate a check is already running. Set the flag to prevent duplicate checks + hasPendingNetworkCheck = state.isInternetReachable === null; }); } @@ -103,15 +107,18 @@ function onReconnect(callback) { reconnectionCallbacks.push(callback); } -NetowrkLib.registerConnectionCheckCallback(_.throttle(() => { - if (!unsubscribeFromNetInfo) { +function recheckNetworkConnection() { + if (hasPendingNetworkCheck) { return; } Log.info('[NetworkConnection] recheck NetInfo'); + hasPendingNetworkCheck = true; unsubscribeFromNetInfo(); subscribeToNetInfo(); -}), 10 * 1000); +} + +NetowrkLib.registerConnectionCheckCallback(recheckNetworkConnection); export default { setOfflineStatus, From 1d3536ae49729b1d340986f0d894fc2ee1ba2d5a Mon Sep 17 00:00:00 2001 From: Peter Velkov Date: Fri, 25 Feb 2022 14:29:17 +0200 Subject: [PATCH 15/15] Comment about `reachabilityRequestTimeout` and increase timeout a bit --- src/CONST/index.js | 2 +- src/libs/NetworkConnection.js | 2 ++ 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/src/CONST/index.js b/src/CONST/index.js index 2ec440f4262f..f5a93b8db7a2 100755 --- a/src/CONST/index.js +++ b/src/CONST/index.js @@ -306,7 +306,7 @@ const CONST = { }, MAX_REQUEST_RETRIES: 10, PROCESS_REQUEST_DELAY_MS: 1000, - MAX_PENDING_TIME_MS: 7 * 1000, + MAX_PENDING_TIME_MS: 10 * 1000, }, HTTP_STATUS_CODE: { SUCCESS: 200, diff --git a/src/libs/NetworkConnection.js b/src/libs/NetworkConnection.js index cb321713278e..12bff43f5aea 100644 --- a/src/libs/NetworkConnection.js +++ b/src/libs/NetworkConnection.js @@ -59,6 +59,8 @@ function subscribeToNetInfo() { // Using API url ensures reachability is tested over internet reachabilityUrl: CONFIG.EXPENSIFY.URL_API_ROOT, reachabilityTest: response => Promise.resolve(response.status === 200), + + // If a check is taking longer than this time we're considered offline reachabilityRequestTimeout: CONST.NETWORK.MAX_PENDING_TIME_MS, });