Airplane mode bug [Android]

[akinyele]

Addressing issue #554

Overview

It was noticed that Android 7 and up when ever the devices airplane mode was enabled then disabled the isConnected value would be false even though internet was present. This only happened when the device has both Wifi and LTE enabled and came out of airplane mode.

After looking into the onLost method, I switch over to using registerDefaultNetworkCallback for the callback register. This prevent the onLost methods from being fired when there was Network access from at least on off the connection i.e. Wifi/LTE.

Let me know what you guys think.

Test Plan

Not sure if this counts. But using the example Android app the changes were verified by running over the scenario described above.

[mikehardy]

Looking at the API page there on the android developer site gives me a headache 😆 there are something like 7 APIs all added at different API versions.

I'll defer to @cristianoccazinsp on this one, but I did verify the API you chose is available from API24 on (that is when it was added exactly) and the object you added the API too is only used for API24+ so this shouldn't cause any sort of obvious compatibility issues, assuming it functions as desired

[mikehardy]

For what it's worth @cristianoccazinsp I just invited you to the org, no pressure at all - it's open source! - just that you have done really good work trying to track down this truly frustrating status issue and with that work should come the ability to actually make change here. Also it increases the bus factor just in case, something I am always interested in doing

[cristianoccazinsp]

@mikehardy thanks for the invitation, too much responsibility for me :D

@akinyele have you tested if the fetch method would still work with this change when used as NetInfo.fetch('wifi') and NetInfo.fetch('cellular') ? If I understand the API correctly, using registerDefaultNetworkCallback will only give you callbacks for the currently active/default network, so if you are on mobile data, you will only get info for the mobile data network, and vice-versa.

I'm sorry I haven't been able to actually test this since I'm having trouble with the stupid eSim implementation here and I cannot switch phones for the eSims I have, and haven't had time to buy a new physical sim to test :'(

For reference, here's the PR that changed the above code to what you are testing: 7084771 #443

I don't know for sure, but these changes may break apps that rely on detection the currently connected wifi network for apps that connect to IoT devices (which don't have internet access).

[akinyele]

Oh, I didn't test those directly, Only the NetInfo.fetch(). Will look into the fetch methods testing for NetInfo.fetch('wifi') and NetInfo.fetch('cellular')

[akinyele]

From my testing, I'm getting these results.

For NetInfo.fetch('wifi') with wifi disabled data enabled I get back the following.

 {"details": {...}, "isConnected": true, "isInternetReachable": false, "isWifiEnabled": false, "type": "wifi"}

For NetInfo.fetch('wifi') with wifi enabled data enabled I get back the following.

{"details": {...}, "isConnected": true, "isInternetReachable": true, "isWifiEnabled": true, "type": "wifi"}

For NetInfo.fetch('cellular') with wifi enabled data enabled I get back the following.

{"details": {"carrier": "Digicel-StaySafe | DIGICEL", "isConnectionExpensive": false}, "isConnected": true, "isInternetReachable": false, "isWifiEnabled": true, "type": "cellular"}

For NetInfo.fetch('cellular') with wifi disabled data enabled I get back the following:

{"details": {"carrier": "Digicel-StaySafe | DIGICEL", "cellularGeneration": "4g", "isConnectionExpensive": true}, "isConnected": true, "isInternetReachable": true, "isWifiEnabled": false, "type": "cellular"}

In all the instances above the isConnected remained true. However, isInternetreachable returned false in some instances, which is not that case.

[cristianoccazinsp]

Did you have a typo in the first two and you actually did fetch('wifi') ?

Do you get the SSID correctly even when connected to both wifi and data? If so, I'm not sure what the above PR fixed in the past, but your PR basically reverts the previous PR I linked above. So this may fix one issue while breaking another one.

[akinyele]

Yeah, that was a typo.

I haven't gone over the Other PR as yet. But yeah the SSID was correct.

{"details": {"bssid": "02:00:00:00:00:00", "frequency": 2432, "ipAddress": "192.168.0.154", "isConnectionExpensive": false, "strength": 96, "subnet": "255.255.255.0"}, "isConnected": true, "isInternetReachable": true, "isWifiEnabled": true, "type": "wifi"}

I don't think I understand the issue fully, I'm not getting the results described in the PR above when I test. The hook and the event listeners are updated when my network changes.

Sorry, but could explain what am I testing for exactly?

[cristianoccazinsp]

@akinyele I've just got my hands on a phone that I can use to test with both mobile and wifi and I confirmed both the initial bug/issue, and that this change does in fact solve it. It happens pretty easily actually whenever you do something that changes networks.

However, merging this change is most likely going to break what #443 tried to fix.

The above PR, basically fixed an issue where the network state change event is not fired for wifi networks if you switch from networks that do not have internet access (because the app only notifies of the current default/internet capable network). The bright side is that you can still manually call fetch to get either wifi or cellular connection info without issues.

In short, merging this PR closes #554 (and possibly the other related ones), but will also break #443.

Either way, I think that for general use cases, it's better to merge this and fix #554 which is a really annoying bug, than trying to support a possibly rarely use case. An ideal fix would probably involve either handling the race conditions of the current implementation, or having the addEventListener support adding listeners to specific interfaces.

[cristianoccazinsp]

@mikehardy I would say your call whether to merge this to close and important bug and re-open a less important one, or wait still for an even better fix.

[mikehardy]

While I haven't been testing personally I have been following along, and I trust your judgement (it really does seem well-reasoned...) and your testing. Let's do it

[matt-oakes]

🎉 This PR is included in version 7.1.10 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀

[mikehardy]

I just want to state more explicitly: thanks to both of you (and any others that worked on this) for sticking with it, I know the netinfo status issues for the last long while have been frustrating, but you've made good progress here I think, at least with understanding what can go wrong and what we think the best balance of issues is here for now. Thanks