Incorporate hasPendingNetworkCheck #44565
Conversation
|
Cool, I'll test it out tomorrow and we could merge. Also cc @OlimpiaZurek since you chimed in here |
src/libs/NetworkConnection.ts
Outdated
| @@ -123,6 +124,13 @@ function subscribeToNetInfo(): () => void { | |||
|
|
|||
| // If a check is taking longer than this time we're considered offline | |||
| reachabilityRequestTimeout: CONST.NETWORK.MAX_PENDING_TIME_MS, | |||
| reachabilityShouldRun: () => { | |||
| if (!hasPendingNetworkCheck) { | |||
There was a problem hiding this comment.
I guess we can simplify this condition a bit:
reachabilityShouldRun: () => !hasPendingNetworkCheck && (hasPendingNetworkCheck = true),
There was a problem hiding this comment.
tbh I find the initial implementation provided easier to follow (less cognitively demanding).
What I'd think about though is limiting the nesting further more by introducing an early return (reverting the boolean check).
reachabilityShouldRun: () => {
- if (!hasPendingNetworkCheck) {
- hasPendingNetworkCheck = true;
- return true;
- }
- return false;
+ if (hasPendingNetworkCheck) {
+ return false;
+ }
+
+ hasPendingNetworkCheck = true;
+ return true;
},
melvin-bot
bot
requested review from
rushatgabhane
and removed request for
a team
|
@rushatgabhane Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
| @@ -112,6 +112,7 @@ function subscribeToNetInfo(): () => void { | |||
| reachabilityUrl: `${CONFIG.EXPENSIFY.DEFAULT_API_ROOT}api/Ping?accountID=${accountID || 'unknown'}`, | |||
| reachabilityMethod: 'GET', | |||
| reachabilityTest: (response) => { | |||
| hasPendingNetworkCheck = false; | |||
There was a problem hiding this comment.
I see this variable is used in recheckNetworkConnection... I think that's just a manual call to check for the connection, is that correct?
And if so, why would we need to update this variable both here and in the current place we are updating it?
There was a problem hiding this comment.
Ah yeah you're right. I'll fix that since any check including NetInfo.refresh() will now check for the state first.
There was a problem hiding this comment.
Fixed this.
I kept
if (hasPendingNetworkCheck) {
return;
}
so that we only see the [NetworkConnection] recheck NetInfo Log when necessary. Otherwise, we would see the log every 60s. But I can remove it if we want to add the Log to reachabilityShouldRun. The only problem with this is we won't be able to differentiate whether it came from the automatic or manual recheck.
There was a problem hiding this comment.
hmmmm actually, testing this locally (using the staging server + web proxy), and I've found that even though I get a response to Ping, the reachabilityTest callback is never being executed, and we're getting stuck offline.
@srikarparsi did you test this out? It kind of seems like it breaks the whole app, but maybe I'm just holding something wrong...
There was a problem hiding this comment.
Also, I just realized another edge-case we missed. We have reachabilityRequestTimeout set to 10 seconds, but if a request times out then I don't think that the reachabilityTest callback will run, because no response is received. In that case, we'd get stuck with hasPendingNetworkCheck = true, and be stuck offline.
|
So I think we'd need to:
But overall, thinking about this change, I wonder if it would make more sense to make the change upstream in react-native-netinfo? It seems like a library issue if it's sending multiple reachability requests in parallel? |
|
I checked this solution and encountered the same problem – I'm getting stuck offline. I also tried solution with timer, but have the same result. @roryabraham Can you elaborate more about this idea? For me Netinfo seems to work stable. |
I left this comment in the description because I wasn't sure how to. Do you think you could point me to instructions or an example of using the staging server + web proxy to test this? I agree, I also think we should make any changes upstream. Because NetInfo should not send multiple requests in parallel. It should also already send a request after a period of time when the request is failing (which is insinuated by reachabilityShortTimeout so I don't think we should have ever needed to create this PR |
|
Closing this PR as explained a little in this comment. First, I think we should take out redundant functionality (the recheck interval) because that is what is causing the problem. And figure out if it's a problem with our implementation or NetInfo and make an upstream fix if needed. |
Details
Fixed Issues
$ #44269
PROPOSAL:
Tests
@OlimpiaZurek and I weren't able to reproduce the issue by closing and opening our laptops. I also don't believe dev calls ping so I think we can test this in staging since the change is relatively straightforward?
Offline tests
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop