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

Fix race condition where a VPN is still active while detecting connetion type #543

Closed
wants to merge 1 commit into from

Conversation

alebahn
Copy link

@alebahn alebahn commented Aug 28, 2024

I was experiencing a bug where the change from metered to un-metered did not take effect. After investigating, this was caused by the VPN service continuing to be active while attempting to detect the after network. This resulted in the a VPN network being set as the active network for the VPN. This prevented other calls that determine the metered status from accurately determining the metered status and it seems it defaulted to true if the underlying network for the VPN was a VPN.

Here's an example from Logcat of the issue:
2024-08-16 20:46:55.843 9252-9296 NetGuard.Service eu.faircode.netguard I Setting underlying network=1073 [type: VPN[], state: DISCONNECTED/DISCONNECTED, reason: (unspecified), extra: (none), failover: false, available: true, roaming: false]

There's a twofold fix here:

  1. When bringing up the VPN, instead of starting the VPN then detecting/setting the active network, the active network is detected first and set on the builder object. Then the VPN is started.
  2. When the VPN is being restarted, after it is stopped, a loop checks if the active network still shows as a VPN so that when the VPN is started again, it will not be detecting the stopped VPN as the active network

I'm not happy with the change in part 2 since it uses busy-waiting. The API documentation for the network status API says that the information should be tracked by using the network status callbacks and memorizing the information. This may make waiting for the VPN to go down before setting the underlying network unnecessary.
https://developer.android.com/reference/android/net/NetworkInfo#isConnected()

@correabuscar
Copy link
Contributor

Unsure, but maybe this commit 89a57d6 was meant to address this?

@M66B
Copy link
Owner

M66B commented Sep 1, 2024

Yes, it is.

@M66B M66B closed this Sep 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants