UesleyCarossi commented 4 years ago

Bug Report

Problem

I need to run an event when connecting to wifi, but it ends up running at unexpected times.

What is expected to happen?

The expected is to occur only when connecting to the internet.

What does actually happen?

The onConnect subscription is firing when the application is opened, minimized> open, when connected to the internet.

Information

Open the application, minimize and open that the onConnect event will be triggered.

Command or Code

https://github.com/UesleyCarossi/test-network

Environment, Platform, Device

Version information

Ionic:

   Ionic CLI                     : 5.4.1 (C:\Users\****\AppData\Roaming\npm\node_modules\ionic)
   Ionic Framework               : @ionic/angular 5.1.1
   @angular-devkit/build-angular : 0.901.7
   @angular-devkit/schematics    : 9.1.7
   @angular/cli                  : 9.1.7
   @ionic/angular-toolkit        : 2.2.0

Cordova:

   Cordova CLI       : 9.0.0 (cordova-lib@9.0.1)
   Cordova Platforms : android 8.0.0
   Cordova Plugins   : cordova-plugin-ionic-keyboard 2.2.0, cordova-plugin-ionic-webview 4.2.1, (and 5 other plugins)

Utility:

   cordova-res : 0.6.0
   native-run  : 0.2.8

System:

   Android SDK Tools : 26.1.1 (C:\Android\sdk)
   NodeJS            : v10.16.0 (C:\Program Files\nodejs\node.exe)
   npm               : 6.9.0
   OS                : Windows 10

Plugins:

cordova-plugin-device 2.0.2 "Device" cordova-plugin-ionic-keyboard 2.2.0 "cordova-plugin-ionic-keyboard" cordova-plugin-ionic-webview 4.2.1 "cordova-plugin-ionic-webview" cordova-plugin-network-information 2.0.2 "Network Information" cordova-plugin-splashscreen 5.0.2 "Splashscreen" cordova-plugin-statusbar 2.4.2 "StatusBar" cordova-plugin-whitelist 1.3.3 "Whitelist"

Checklist

[x] I searched for existing GitHub issues
[ ] I updated all Cordova tooling to most recent version
[x] I included all the necessary information above

breautek commented 4 years ago

Which platforms are you experiencing this on?

UesleyCarossi commented 4 years ago

Which platforms are you experiencing this on?

Tested only on Android

breautek commented 4 years ago

I've reproduced the described behaviour on an Android API 29 simulator using the latest cordova tooling and plugin.

Just posting some information... Couple of different theories.

Theory 1

Broadcast event is firing because we unregister and register to the native listener onPause/onResume. I assume the act of registering the event causes the event to fire at least once, producing an event to the javascript.

It didn't always unattach/re-attach the listener on pause/resume. This was changed by https://github.com/apache/cordova-plugin-network-information/pull/74 for https://github.com/apache/cordova-plugin-network-information/issues/64

Theory 2

The native broadcast simply fires when the activity is resumed.

I'm not really sure on the behaviour of the Intent broadcaster, but if theory 2 is actually what is happening, then I don't think this is a bug.

But if I were to make a bet, I think Theory 1 is why we are seeing these events on pause/resume. If we consider this a bug, then I think we should keep track of connection types, and only propagate the event to the javascript if the connection type actually changes. I'd support a PR that makes this change.

I'd also like to see if this behaviour is observable on iOS, but I don't have mac equipment to use to test this.

UesleyCarossi commented 4 years ago

The problem is that the plugin behaves differently, based on the day it was cloned and executed npm i, cordova platform add android@8.0.0, cordova build android.

A few weeks ago we were looking for some way to identify when the user connected to wifi, we tested and confirmed that the cordova-plugin-network-information plugin would perfectly fulfill this purpose, so analysis and implementations were made in the project, but when we went integrating with the plugin its behavior was different from what we had tested weeks ago, we have 2 apks to prove it.

UesleyCarossi commented 4 years ago

I downgraded from version 2.0.2 to 2.0.1 and it works perfectly. 😉

breautek commented 4 years ago

I downgraded from version 2.0.2 to 2.0.1 and it works perfectly.

well that's definitely interesting. I don't have time to poke around right now, but having a list of change commits between 2.0.1 and 2.0.2 would be helpful. Sounds like there was a regression that was introduced.

UesleyCarossi commented 4 years ago

Maybe it's helpful, I use cordova-plugin-network-information in conjunction with cordova-plugin-background-mode.

PieterVanPoyer commented 4 years ago

I've reproduced the described behaviour on an Android API 29 simulator using the latest cordova tooling and plugin.

Just posting some information... Couple of different theories.

Theory 1

Broadcast event is firing because we unregister and register to the native listener onPause/onResume. I assume the act of registering the event causes the event to fire at least once, producing an event to the javascript.

It didn't always unattach/re-attach the listener on pause/resume. This was changed by #74 for #64

Theory 2

The native broadcast simply fires when the activity is resumed.

I'm not really sure on the behaviour of the Intent broadcaster, but if theory 2 is actually what is happening, then I don't think this is a bug.

But if I were to make a bet, I think Theory 1 is why we are seeing these events on pause/resume. If we consider this a bug, then I think we should keep track of connection types, and only propagate the event to the javascript if the connection type actually changes. I'd support a PR that makes this change.

I'd also like to see if this behaviour is observable on iOS, but I don't have mac equipment to use to test this.

Hi

Thanks for the great work with Cordova!

I think Theory 1 is correct. By reattatching the native listener it fires. This reattaching helped to fix the bug where the connection state seems to be wrong after an amount of time when the app was idle.

I admit, it would be an improvement if we could keep the state of te connection and only propagate a change when the online/offline state effectivily changes.

In my opinion, this plugin is very usefull, but aims to do too much. It was originally developed when there we're a lot less restrictions and rules in the Android system.

Now the Android OS can stop the listeners for working when the app is idle.
Now the Android OS is much more watching the battery drain, and stopping apps when they are idle.
Now there are a lot of more restrictions to privacy, and this affects the detection of the effective Network.
The ios system has even more restrictions about privacy.

In the documentation of this plugin there is an example of an upload of a file in the background https://github.com/apache/cordova-plugin-network-information#sample-upload-a-file-depending-on-your-network-state- , this used to work great, but now you have to work with WorkManager and stuff like that to do work in the background.

A suggestion

Loosen the requirements of the plugin to it's barebone's essentials.

Requirements

Detect online/offline only when the app is active.
When online, only detect following types: WIFI, MOBILE
If possible, only fire when the state effectivily changes. So keep track of the previous state before the app went in the background. And the app is not killed by the system.

Stop with next requirements

Detect online/offline when the app is not active.
Don't try to detect other network type's beside WIFI and MOBILE.
Do not let this plugin support background work like fileupload's and datasyncs. Remove it from the doc's.

A suggestion to do on short notice

If https://github.com/apache/cordova-plugin-network-information/issues/91#issuecomment-531343312 this comments work for the state. Maybe it should be mentioned in the README of this plugin.

Kind regards Pieter

PieterVanPoyer commented 4 years ago

Hey

I did check the Android code and found something interesting.

On next lines, there is a check on the equality of the network state before and after a change. The code seems to try to send an update only after a change. But this code goes long way back.

https://github.com/apache/cordova-plugin-network-information/blob/9f8527062776ead04982d7fab12875a66bf69855/src/android/NetworkManager.java#L221-L233

The thisInfo and lastInfo are JSONObjects.

But when I run following code in a testproject. 2 JSONObjects with the same values are not seen as equal.

try {
            JSONObject connectionInfo = new JSONObject();
            connectionInfo.put("type", "myType");
            connectionInfo.put("extraInfo", "myExtraInfo");

            JSONObject otherConnectionInfo = new JSONObject();
            otherConnectionInfo.put("type", "myType");
            otherConnectionInfo.put("extraInfo", "myExtraInfo");

            prependToNetworkOutputState(this.tvNetworkStateOutputKeepsListening, "equals JSONObjects " + connectionInfo.equals(otherConnectionInfo)); // THIS EQUALS RETURNS FALSE
        } catch (JSONException e) {

        }

I found something like this on StackOverflow: the Android's built-in JSONObject does not define hashCode() and equals() in any sensible way.
Check also the Android Docs: Android Docs

Can anyone confirm this? If yes, then we should just correct the equality check.

There is also another path we have to check for state, but that's for later.

Kind regards, Pieter

gidhin commented 2 years ago

Any update on this issue. I'm facing the same in 3.0.0 in ANDROID.

apache / cordova-plugin-network-information

bug: 'onConnect' does not work as expected #110

Bug Report

Problem

What is expected to happen?

What does actually happen?

Information

Command or Code

Environment, Platform, Device

Version information

Checklist

Theory 1

Theory 2

Theory 1

Theory 2

A suggestion

A suggestion to do on short notice