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

[Android] Add NetworkChange implementation using PeriodicTimer #80548

Merged
merged 4 commits into from
Jan 19, 2023

Conversation

simonrozsival
Copy link
Member

@simonrozsival simonrozsival commented Jan 12, 2023

We can't use netlink sockets to monitor network changes on Android anymore due to changes to permissions in recent Android versions. There doesn't seem to be a native Android API that could be used to implement the NetworkChange API in .NET. Instead, this PR implements the functionality using PeriodicTimer and manually diffing the IP addresses of all network interfaces. The period is set to 2s to limit the impact on battery life of mobile devices.

Ref #77822

@ghost
Copy link

ghost commented Jan 12, 2023

Tagging subscribers to 'arch-android': @steveisok, @akoeplinger
See info in area-owners.md if you want to be subscribed.

Issue Details

WIP - this code needs more testing before it will be ready for review.

We can't use netlink sockets to monitor network changes on Android anymore due to changes to permissions in recent Android versions. There doesn't seem to be a native Android API that could be used to implement the NetworkChange API in .NET. Instead, this PR attempts to implement the functionality using PeriodicTimer.

Ref #77822

Author: simonrozsival
Assignees: -
Labels:

area-System.Net, os-android

Milestone: -

@stephentoub stephentoub added the NO-REVIEW Experimental/testing PR, do NOT review it label Jan 12, 2023
@simonrozsival simonrozsival marked this pull request as ready for review January 17, 2023 13:51
@simonrozsival
Copy link
Member Author

/azp run runtime-android

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@simonrozsival simonrozsival removed the NO-REVIEW Experimental/testing PR, do NOT review it label Jan 17, 2023
Copy link
Member

@steveisok steveisok left a comment

Choose a reason for hiding this comment

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

LGTM. I think a 2 sec poll is reasonable for those who want to opt in. I don't think there are any other good alternatives.

Copy link
Member

@mdh1418 mdh1418 left a comment

Choose a reason for hiding this comment

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

A few nits, but overall looks good to me!
Out of curiosity, were there statistics to use 2 seconds polling? I'm not familiar, but is there a general frequency to network changes, and is the impact on battery life mainly from CheckIfAddressChanged firing? How long does it take for CheckIfAddressChanged to finish?

@simonrozsival
Copy link
Member Author

@mdh1418 The 2s is my estimate of what is "almost immediately" but at the same time "not all the time". I tried being as conservative as possible with the frequency.

I didn't measure how long CheckIfAddressChanged CheckIfNetworkChanged runs because it will be different on different devices. The function enumerates the network interfaces present on the phone/tablet and that will behave a little differently on each device. I can make some measurements tomorrow.

@mdh1418
Copy link
Member

mdh1418 commented Jan 18, 2023

Gotcha, I was wondering if the impact of battery life would be more directly related to the duty cycle of CheckIfNetworkChanged. If it takes a whole second to run (50% duty cycle/active time), then 2 seconds or even longer seems to make sense (without considering how important it is to not miss network changes), and on the other hand if it takes 1ns to run, then we can probably check more frequently. But I'm not sure if thats exactly how the battery is directly impacted.

What might be more helpful to determine polling frequency than duty cycle might be what happens if we miss a network change? Is it problematic if there were multiple network changes within the 2 seconds? Is it problematic if there was a change that cancelled itself out within 2 seconds (i.e. a network becomes available and then becomes unavailable all before the next poll)?

@simonrozsival
Copy link
Member Author

@mdh1418 So on my Samsung Galaxy phone running Android 13 CheckIfNetworkChanged usually takes around 2-8 milliseconds when there isn't any change since the previous call and around 40-50 ms whenever there is a change. So it isn't on the order of seconds, but neither is it on the order of nanoseconds.

I don't think it's too important to detect these short network availability spikes, although there might be customers who might need this kind of information. The use case for this API in a mobile app I can imagine is to wait for network availability and only when the device is online, attempt to send a HTTP request. This implementation would IMO be suitable for that use case. it wouldn't be useful to monitor and diagnose network stability of an Android device. I think there are more suitable native APIs for that use case.

@simonrozsival
Copy link
Member Author

/azp run runtime-android

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@simonrozsival
Copy link
Member Author

Failing tests are unrelated to this PR.

@simonrozsival simonrozsival merged commit b6f12dd into dotnet:main Jan 19, 2023
@simonrozsival simonrozsival deleted the android-network-change branch January 19, 2023 20:09
mdh1418 pushed a commit to mdh1418/runtime that referenced this pull request Jan 24, 2023
…t#80548)

* Add NetworkChange implementation using PeriodicTimer

* Code cleanup

* Rename functions

* Update csproj
@simonrozsival
Copy link
Member Author

/backport to release/7.0

@github-actions
Copy link
Contributor

Started backporting to release/7.0: https://github.com/dotnet/runtime/actions/runs/4043009267

@github-actions
Copy link
Contributor

@simonrozsival backporting to release/7.0 failed, the patch most likely resulted in conflicts:

$ git am --3way --ignore-whitespace --keep-non-patch changes.patch

Applying: Add NetworkChange implementation using PeriodicTimer
Using index info to reconstruct a base tree...
M	src/libraries/System.Net.NetworkInformation/src/System.Net.NetworkInformation.csproj
Falling back to patching base and 3-way merge...
Auto-merging src/libraries/System.Net.NetworkInformation/src/System.Net.NetworkInformation.csproj
CONFLICT (content): Merge conflict in src/libraries/System.Net.NetworkInformation/src/System.Net.NetworkInformation.csproj
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Add NetworkChange implementation using PeriodicTimer
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Error: The process '/usr/bin/git' failed with exit code 128

Please backport manually!

@github-actions
Copy link
Contributor

@simonrozsival an error occurred while backporting to release/7.0, please check the run log for details!

Error: git am failed, most likely due to a merge conflict.

simonrozsival added a commit to simonrozsival/runtime that referenced this pull request Jan 30, 2023
…t#80548)

* Add NetworkChange implementation using PeriodicTimer

* Code cleanup

* Rename functions

* Update csproj
simonrozsival added a commit to simonrozsival/runtime that referenced this pull request Jan 30, 2023
…t#80548)

* Add NetworkChange implementation using PeriodicTimer

* Code cleanup

* Rename functions

* Update csproj
carlossanlop pushed a commit that referenced this pull request Feb 8, 2023
… (#81349)

* Add NetworkChange implementation using PeriodicTimer

* Code cleanup

* Rename functions

* Update csproj
carlossanlop pushed a commit that referenced this pull request Feb 9, 2023
… (#81350)

* Add NetworkChange implementation using PeriodicTimer

* Code cleanup

* Rename functions

* Update csproj
@ghost ghost locked as resolved and limited conversation to collaborators Mar 1, 2023
@karelz karelz added this to the 8.0.0 milestone Mar 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants