Fix connection drop is not detected #7825

kidroca · 2022-02-19T00:32:46Z

Details

Update NetInfo usage to test for internet connectivity rather than active network connection
Desktop specific NetInfo shim is removed - the external issue in the comment has been resolved and offline state is detected in Desktop

Remains the same:

When network is lost offline state is detected instantly (like turning off wifi or enabling airplane mode)

New:

If internet is unavailable it it takes a bit of time to be detected - up to 12 seconds - when we have a request
running for +7 seconds we trigger connection recheck that takes about 5 sec. (or less if it fails immediately)
When internet is resumed it may take up to 10 sec. to detect, depends on platform

Fixed Issues

$ #6059

Tests

Test network connection being correctly detected as offline
Test network connection being correctly detected as online

Web / Desktop

Open dev tools
Set network state to offline from dev tools or turn of wifi

iOS / Android

simulate poor network conditions (available in Android emulator)
For iOS I think it's only possible on a physical device

QA Steps

Try to simulate very poor or no network conditions

Test network connection being correctly detected as offline
Test network connection being correctly detected as online

Web / Desktop

Use on a tethered connection
Use with wifi disabled (cable unplugged)
Connected to a connection with no internet (e.g. internet disabled from router)

iOS / Android

Try in airplane mode
Try with mobile data disabled and no wifi
Connection with no internet (e.g. internet disabled from router)

Tested On

Screenshots

Web

⚠️ It was only possible to test turning network off because of the web proxy on DEV. When deployed to staging it should work like in the Desktop video

New.Expensify.-.Google.Chrome.2022-02-19.05-37-48.mp4

Mobile Web

mobile.internet.mp4

Desktop

Desktop.Internet.mp4

iOS

RPReplay_Final1645250899.mp4

Android

Android.Internet.mp4

libs/NetInfo served to provide shim for Electron due to an issue The issue is now resolved and this is no longer needed

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 (Expensify#560) (fbf7c15), closes Expensify#420 7.1.11 (2022-02-08) Bug Fixes windows: fix race condition in WiFi connection details feature (Expensify#568) (0cd8132) 7.1.10 (2022-02-07) Bug Fixes android: use registerDefaultNetworkCallback so toggling airplane mode works (Expensify#571) (e8af2de) 7.1.9 (2022-01-26) Bug Fixes android: count native listeners / correctly disable listener if count == 0 (Expensify#569) (5ae16f6) 7.1.8 (2022-01-25) Bug Fixes windows: refactor implementation to avoid crashes (Expensify#564) (cc4bfa3) 7.1.7 (2021-12-20) Bug Fixes android: populate network value during initial module startup (Expensify#553) (c05080f) 7.1.6 (2021-12-13) Bug Fixes android: avoid send event when has no listener (Expensify#548) (cad47d8) 7.1.5 (2021-12-09) Bug Fixes android: use method-local ref to instance var for multi-thread safety Expensify#549 (Expensify#550) (81bbc87) 7.1.4 (2021-12-07) Bug Fixes android: try async state fetch as stale state workaround (Expensify#547) (937cf48), closes Expensify#542

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

src/libs/Network.js

src/CONST/index.js

kidroca

Can I ask what's the problem with defining duration as 10 * 1000 is it against code conventions?

src/libs/NetworkConnection.js

`trailing: false` causes duplicate calls removing it makes just one recheck request (as intended)

kidroca · 2022-02-21T14:51:55Z

Ready for review

parasharrajat · 2022-02-21T17:36:37Z

Can I ask what's the problem with defining duration as 10 * 1000

It is very clear that 10000 is 10 sec. It is just an extra calculation. Why do you think 10 * 1000 is better?

kidroca · 2022-02-22T00:42:41Z

It is very clear that 10000 is 10 sec. It is just an extra calculation. Why do you think 10 * 1000 is better?

Maybe for you. I have hard time distinguishing thousands without some kind of visual separator:

10000 (This is looking like 100 000 to me)
vs
10,000
10 * 1000

It's common to use this pattern even for seconds: https://github.com/react-native-netinfo/react-native-netinfo/blob/6b01fee211df160d0afce70c5955681c75b24a5a/src/internal/defaultConfiguration.web.ts#L3-L12

And we used as well: https://github.com/Expensify/App/search?q=expiration+%3D+1000+*+60

parasharrajat

I don't mind 10 * 1000. I just thinking that we use 5000 ,3000 and not 5 * 1000 in other places of our app.

it makes more sense to use when you have to show the different parts of calculations.
e.g.
Secs in day = 24 * 60 * 60

src/libs/Network.js

parasharrajat · 2022-02-22T17:05:53Z

src/libs/NetworkConnection.js

+    // whether a user has internet connectivity or not.
+    unsubscribeFromNetInfo = NetInfo.addEventListener((state) => {
+        Log.info('[NetworkConnection] NetInfo state', false, state);
+        setOfflineStatus(!state.isInternetReachable);


Here we set the offline/online status based on isInternetReachable prop. I didn't find many details about it but I think this flag is set based on the result of the HTTP request.
So when the user is online it seems right to check the flag but when the user is offline it will take some time to be set.
I think we should do this.

if state.isConnected
then offline status = !state.isInternetReachable
Else if !state.isConnected
then offline status = false.

When a user is not connected then we don't have to wait for this flag state.isInternetReachable. If it behaves the same then It's OK.

isInternetReachable is not always set from the result of a request
When state.isConnected is false - state.isInternetReachable is also false

isInternetReachable is set from true to false when the check request fails or takes longer than 5sec.

kidroca · 2022-02-25T12:51:51Z

✔️ Ready for review

parasharrajat · 2022-02-26T00:26:13Z

Ok, I see. I will test it shortly.

parasharrajat · 2022-02-27T16:28:21Z

Thanks for the clarification and changes. Looks better now.

Issue 3: If you do above till 4, you will notice that wait time is very high.

Do you think we can improve this by adding a global listener for NetInfo?

 // whether a user has internet connectivity or not.
    unsubscribeFromNetInfo = NetInfo.addEventListener((state) => {
        Log.info('[NetworkConnection] NetInfo state', false, state);
        setOfflineStatus(!state.isInternetReachable);

kidroca · 2022-02-28T09:41:32Z

Issue 3: If you do above till 4, you will notice that wait time is very high.

Do you think we can improve this by adding a global listener for NetInfo?

How would that help?

parasharrajat · 2022-02-28T17:28:33Z

Oh, So we always have an active listener Didn't notice it earlier. At this point, I say we are good.

parasharrajat

LGTM.

cc: @pecanoro

🎀 👀 🎀 C+ reviewed

kidroca · 2022-03-03T16:41:45Z

@pecanoro can you review this PR, it's been waiting for some time

pecanoro · 2022-03-04T16:02:34Z

Sure, sorry for the delay, I was at a college career fair during the week so I wasn't on my computer.

OSBotify · 2022-03-04T16:32:06Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

kidroca · 2022-03-04T16:33:50Z

Thank you 👍

marcaaron · 2022-03-08T17:53:24Z

Just a heads up that this is causing CORS errors on dev where most internal engineers are using ngrok to tunnel to the local API. Is there any solution around this?

kidroca · 2022-03-08T18:19:03Z

Is there any solution around this?

Use a different URL here:

App/src/libs/NetworkConnection.js

Lines 57 to 60 in d05c56e

    
           // 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,

Or disable the internet reachability test if ENV is DEV

kidroca · 2022-03-08T18:23:03Z

Shouldn't URL_API_ROOT be the ngrok URL?

const expensifyURLRoot = useNgrok && ngrokURL ? ngrokURL : expensifyComWithProxy;
...
URL_API_ROOT: expensifyURLRoot,

marcaaron · 2022-03-08T18:24:08Z

Yes, but it seems there's some kind of CORS issue with doing that

OSBotify · 2022-03-11T01:00:29Z

🚀 Deployed to staging by @pecanoro in version: 1.1.42-0 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

marcaaron · 2022-03-11T18:14:35Z

Throwing a deploy blocker on here temporarily while we try to figure out what is happening. I have a feeling these changes may be contributing to the instability brought up in this thread

marcaaron · 2022-03-11T20:02:55Z

Ok, seems like it is only affecting dev 😄

OSBotify · 2022-03-15T20:53:12Z

🚀 Deployed to production by @chiragsalian in version: 1.1.42-6 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

roryabraham · 2022-04-06T05:10:18Z

src/libs/NetworkConnection.js

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


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

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

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

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

kidroca force-pushed the kidroca/fix/connection-drop-is-not-detected branch 2 times, most recently from ddea1cb to ca7cc09 Compare February 19, 2022 03:09

kidroca added 7 commits February 19, 2022 08:41

Delete libs/NetInfo - no longer needed

1f19665

libs/NetInfo served to provide shim for Electron due to an issue The issue is now resolved and this is no longer needed

Update connectivity check with isInternetReachable

d1181e2

Update Podfile.lock for react-native-netinfo

3b52875

Only trigger offline state if we're certain there's no internet

7ad9bc2

Faster offline detection

f5a0171

kidroca force-pushed the kidroca/fix/connection-drop-is-not-detected branch from a8b85a3 to f5a0171 Compare February 19, 2022 06:43

kidroca marked this pull request as ready for review February 19, 2022 07:00

kidroca requested a review from a team as a code owner February 19, 2022 07:00

botify requested review from parasharrajat and pecanoro February 19, 2022 07:00

MelvinBot removed the request for review from a team February 19, 2022 07:00

kidroca mentioned this pull request Feb 19, 2022

[HOLD for payment 2022-03-22] [$500] Connection drop is not detected - Reported by: @Santhosh-Sellavel #6059

Closed

parasharrajat suggested changes Feb 19, 2022

View reviewed changes

src/libs/NetworkConnection.js Outdated Show resolved Hide resolved

src/libs/NetworkConnection.js Outdated Show resolved Hide resolved

src/libs/Network.js Outdated Show resolved Hide resolved

parasharrajat suggested changes Feb 19, 2022

View reviewed changes

src/CONST/index.js Outdated Show resolved Hide resolved

kidroca commented Feb 21, 2022

View reviewed changes

src/libs/NetworkConnection.js Outdated Show resolved Hide resolved

kidroca added 5 commits February 21, 2022 15:05

Document NetInfo.configure serves to recheck network state

924ef7b

Rename MAX_PENDING_TIME to MAX_PENDING_TIME_MS

2e9eb22

Add PR suggestion

8642da5

Remove trailing from recheck

d9321c4

`trailing: false` causes duplicate calls removing it makes just one recheck request (as intended)

Fix typo

5cdc7ef

kidroca requested a review from parasharrajat February 21, 2022 14:51

parasharrajat suggested changes Feb 22, 2022

View reviewed changes

kidroca requested a review from parasharrajat February 22, 2022 19:10

kidroca requested a review from parasharrajat February 25, 2022 12:51

Merge branch 'main' into kidroca/fix/connection-drop-is-not-detected

b327db8

parasharrajat approved these changes Feb 28, 2022

View reviewed changes

pecanoro approved these changes Mar 4, 2022

View reviewed changes

pecanoro merged commit 4b80ab6 into Expensify:main Mar 4, 2022

kidroca deleted the kidroca/fix/connection-drop-is-not-detected branch March 7, 2022 09:52

marcaaron mentioned this pull request Mar 8, 2022

[No QA] Only do more specific reachability test in production #8043

Merged

1 task

PauloGasparSv mentioned this pull request Mar 9, 2022

Being offline is not detected if you use VPN (iOS) - reported by @kidroca #7975

Closed

OSBotify mentioned this pull request Mar 10, 2022

Deploy Checklist: New Expensify 2022-03-10 #8074

Closed

marcaaron added the DeployBlockerCash This issue or pull request should block deployment label Mar 11, 2022

marcaaron removed the DeployBlockerCash This issue or pull request should block deployment label Mar 11, 2022

marcaaron mentioned this pull request Mar 11, 2022

[No QA] Disable recheck against local web API when local web API is in use #8105

Merged

roryabraham reviewed Apr 6, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix connection drop is not detected #7825

Fix connection drop is not detected #7825

kidroca commented Feb 19, 2022 •

edited

Loading

kidroca left a comment

kidroca commented Feb 21, 2022

parasharrajat commented Feb 21, 2022 •

edited

Loading

kidroca commented Feb 22, 2022

parasharrajat left a comment

parasharrajat Feb 22, 2022

kidroca Feb 22, 2022

kidroca commented Feb 25, 2022

parasharrajat commented Feb 26, 2022

parasharrajat commented Feb 27, 2022 •

edited

Loading

kidroca commented Feb 28, 2022

parasharrajat commented Feb 28, 2022

parasharrajat left a comment

kidroca commented Mar 3, 2022

pecanoro commented Mar 4, 2022

OSBotify commented Mar 4, 2022

kidroca commented Mar 4, 2022

marcaaron commented Mar 8, 2022

kidroca commented Mar 8, 2022 •

edited

Loading

kidroca commented Mar 8, 2022

marcaaron commented Mar 8, 2022

OSBotify commented Mar 11, 2022

marcaaron commented Mar 11, 2022

marcaaron commented Mar 11, 2022

OSBotify commented Mar 15, 2022

roryabraham Apr 6, 2022

roryabraham Apr 6, 2022

kidroca Apr 6, 2022

roryabraham Apr 6, 2022

Fix connection drop is not detected #7825

Fix connection drop is not detected #7825

Conversation

kidroca commented Feb 19, 2022 • edited Loading

Details

Fixed Issues

Tests

Web / Desktop

iOS / Android

QA Steps

Web / Desktop

iOS / Android

Tested On

Screenshots

Web

Mobile Web

Desktop

iOS

Android

kidroca left a comment

Choose a reason for hiding this comment

kidroca commented Feb 21, 2022

parasharrajat commented Feb 21, 2022 • edited Loading

kidroca commented Feb 22, 2022

parasharrajat left a comment

Choose a reason for hiding this comment

parasharrajat Feb 22, 2022

Choose a reason for hiding this comment

kidroca Feb 22, 2022

Choose a reason for hiding this comment

kidroca commented Feb 25, 2022

parasharrajat commented Feb 26, 2022

parasharrajat commented Feb 27, 2022 • edited Loading

kidroca commented Feb 28, 2022

parasharrajat commented Feb 28, 2022

parasharrajat left a comment

Choose a reason for hiding this comment

kidroca commented Mar 3, 2022

pecanoro commented Mar 4, 2022

OSBotify commented Mar 4, 2022

kidroca commented Mar 4, 2022

marcaaron commented Mar 8, 2022

kidroca commented Mar 8, 2022 • edited Loading

kidroca commented Mar 8, 2022

marcaaron commented Mar 8, 2022

OSBotify commented Mar 11, 2022

marcaaron commented Mar 11, 2022

marcaaron commented Mar 11, 2022

OSBotify commented Mar 15, 2022

roryabraham Apr 6, 2022

Choose a reason for hiding this comment

roryabraham Apr 6, 2022

Choose a reason for hiding this comment

kidroca Apr 6, 2022

Choose a reason for hiding this comment

roryabraham Apr 6, 2022

Choose a reason for hiding this comment

kidroca commented Feb 19, 2022 •

edited

Loading

parasharrajat commented Feb 21, 2022 •

edited

Loading

parasharrajat commented Feb 27, 2022 •

edited

Loading

kidroca commented Mar 8, 2022 •

edited

Loading