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

Fix connection drop is not detected #7825

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
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
4 changes: 2 additions & 2 deletions ios/Podfile.lock
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
6 changes: 3 additions & 3 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,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",
Expand Down
1 change: 1 addition & 0 deletions src/CONST.js
Original file line number Diff line number Diff line change
Expand Up @@ -306,6 +306,7 @@ const CONST = {
},
MAX_REQUEST_RETRIES: 10,
PROCESS_REQUEST_DELAY_MS: 1000,
MAX_PENDING_TIME_MS: 10 * 1000,
},
HTTP_STATUS_CODE: {
SUCCESS: 200,
Expand Down
51 changes: 0 additions & 51 deletions src/libs/NetInfo/index.desktop.js

This file was deleted.

3 changes: 0 additions & 3 deletions src/libs/NetInfo/index.js

This file was deleted.

10 changes: 9 additions & 1 deletion src/libs/Network.js
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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_MS);
kidroca marked this conversation as resolved.
Show resolved Hide resolved

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() {
Expand Down Expand Up @@ -231,6 +236,8 @@ 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');
if (shouldRetry && error.name !== CONST.ERROR.REQUEST_CANCELLED) {
Expand Down Expand Up @@ -362,4 +369,5 @@ export {
setIsReady,
registerRequestSkippedHandler,
registerLogHandler,
registerConnectionCheckCallback,
};
58 changes: 47 additions & 11 deletions src/libs/NetworkConnection.js
Original file line number Diff line number Diff line change
@@ -1,15 +1,19 @@
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';
import * as Network from './actions/Network';
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()
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 = [];
Expand All @@ -19,9 +23,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});

/**
Expand All @@ -31,7 +35,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)
Expand All @@ -47,21 +51,40 @@ 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() {
// 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
// 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');
// If a check is taking longer than this time we're considered offline
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 isConnected: ${state && state.isConnected}`);
setOfflineStatus(!state.isConnected);
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;
});
}

function listenForReconnect() {
Log.info('[NetworkConnection] listenForReconnect called');

unsubscribeFromAppState = AppStateMonitor.addBecameActiveListener(() => {
triggerReconnectionCallbacks('app became active');
});

subscribeToNetInfo();
}

/**
* Tear down the event listeners when we are finished with them.
*/
Expand All @@ -86,6 +109,19 @@ function onReconnect(callback) {
reconnectionCallbacks.push(callback);
}

function recheckNetworkConnection() {
if (hasPendingNetworkCheck) {
return;
}

Log.info('[NetworkConnection] recheck NetInfo');
hasPendingNetworkCheck = true;
unsubscribeFromNetInfo();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kidroca Do you recall why you chose to unsubscribe and resubscribe to NetInfo rather than using NetInfo.fetch to recheck the network connection?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, looks like NetInfo will only trigger the recheck if you call NetInfo.fetch with a specific interface: https://github.com/react-native-netinfo/react-native-netinfo/blob/f05f7dd6e829135d907506e9270e729066b6e5d1/src/internal/state.ts#L100-L106

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, looks like NetInfo will only trigger the recheck if you call NetInfo.fetch with a specific interface

Yes, there isn't an exposed way to force a recheck, it might not be necessary, though we decided to do it if any request is taking unusually long time (more than 10-15 sec)

What triggers the recheck is actually the call to NetInfo.configure

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, there isn't an exposed way to force a recheck

Yep, I created a PR for react-native-netinfo to add this and clean up this implementation a bit: react-native-netinfo/react-native-netinfo#594

subscribeToNetInfo();
}

NetowrkLib.registerConnectionCheckCallback(recheckNetworkConnection);

export default {
setOfflineStatus,
listenForReconnect,
Expand Down