This repository has been archived by the owner on Sep 29, 2024. It is now read-only.
Do not clear reasserting flag when connection is not recoverable #128
Conversation
This file contains 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
|
Have you ever experienced #167? I'm wondering if it could also relate. |
|
No I haven’t actually, so not really sure. |
|
My concern is that
I'm rather thinking of postponing |
|
Gotcha. Sounds good, cheers. |
jaroslavas
referenced
this pull request
in ProtonVPN/ios-app
May 6, 2020
0f3ee96 to
add0583
Compare
Both versions prevent clients from compiling, but this version impacts less on existing codebase.
|
@rob-patchett I rebased to Have a look at my changes, I fixed the logic behind reconnection. It should make the abrupt |
Use a different variable to signal an upcoming reconnection. Make sure that reasserting is never set to false with the meaning of "do not reconnect", because doing so would trigger a transient "connected" state in the VPN. Reverts use of cancelTunnelWithError() in sessionDidStop.
|
@keeshux, yeah this looks good. I've retested and confirmed the problem is still resolved with these changes. Good to merge from my side 👍 Thanks for improving! |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
Issue I'm trying to solve
When attempting to connect, if there is an auth failure, the host application receives a
connectedstatus update from the extension before receiving adisconnectedupdate, which leads to confusing UI changes and poor handling of the error state (in my case checking for an update in the user's VPN credentials).What triggers the transient connected state
Setting NETunnelProvider's
reassertingto false triggers a successful connection status from from OS.Proposed solution
According to NEPacketTunnelProvider's docs, upon error,
cancelTunnelWithError(_:)should be called: https://developer.apple.com/documentation/networkextension/nepackettunnelprovider/1406169-canceltunnelwitherror.This PR proposes calling
cancelTunnelWithError(_:)before reseting state inOpenVPNTunnelProvider.This solution resolves the specific problem described above, but I'm not confident as to the lack of side effects. I'm also not sure the implications of calling
cancelTunnelWithError(_:)from the error case indoReconnect. Any guidance or recommendations would be appreciated.