Conversation
…n the vpn is restablished just after
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
jamilbk
approved these changes
Sep 5, 2024
Member
jamilbk
left a comment
jamilbk
left a comment
There was a problem hiding this comment.
LGTM, just some tidying up suggestions. I think with the comment we can remove the logging.
Comment on lines
+25
to
+26
| Log.d("DisconnectMonitor", "properties: $linkProperties") | ||
|
|
Comment on lines
+49
to
+51
| // We delay the execution of disconnecting the tunnel when the network is lost since | ||
| // when the tunnel is rebuild we get an onLost just like with disabling the VPN and we | ||
| // can't distinguish between those save for getting an onLinkProperties changed later |
Member
There was a problem hiding this comment.
Suggested change
| // We delay the execution of disconnecting the tunnel when the network is lost since | |
| // when the tunnel is rebuild we get an onLost just like with disabling the VPN and we | |
| // can't distinguish between those save for getting an onLinkProperties changed later | |
| // We can't easily tell the difference between a user disconnecting the VPN from | |
| // system settings, and the system "disconnecting" our VPN due to the fd being swapped out during buildVpnService. Both call onLost with the VPN network ID. | |
| // If we're rebuilding the VPN, however, we'll expect an onLinkPropertiesChanged to happen soon after onLost, so delay the tunnel disconnect here and clear the scheduled disconnect if we "resume" the VPN within a couple seconds. |
| // can't distinguish between those save for getting an onLinkProperties changed later | ||
| sessionStopperHandle.postDelayed( | ||
| { | ||
| Log.d("DisconnectMonitor", "Disconnect tunnel service due to network lost $network") |
Member
There was a problem hiding this comment.
Can clean up a tiny bit.
Suggested change
| Log.d("DisconnectMonitor", "Disconnect tunnel service due to network lost $network") |
conectado
added a commit
that referenced
this pull request
Sep 5, 2024
conectado
added a commit
that referenced
this pull request
Sep 5, 2024
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
On Android when when we receive the
onLostcallback for the VPN we disconnect the tunnel, since that's the only way to find when the user has disconnected the VPN from outside Firezone, otherwise this would kill network connection when it's this happens.But
onLostwas also called when we callestablishon the tunnel to recreate it and there's no obvious way to distinguish between both cases.So we previously just stopped monitoring
onLostwhile we were executingestablish. The problem with this approach was that sometimesonLostmight have a delayed sinceestablishis called.Therefore, to fix this, we changed the
onLostcall to delay the execution ofsession.disconnectfor 2 second, if within this grace period we get alinkPropertiesChangedfor our interface, which is always called after the network is re-created, we cancel the execution, otherwise we go ahead an disconnect the session.