-
Notifications
You must be signed in to change notification settings - Fork 361
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
Packet Tunnel stuck in blocked state after Airplane mode is toggled. #6647
Packet Tunnel stuck in blocked state after Airplane mode is toggled. #6647
Conversation
12e249e
to
004ae25
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 2 of 4 files at r1, all commit messages.
Reviewable status: 2 of 4 files reviewed, 2 unresolved discussions (waiting on @mojganii)
ios/PacketTunnelCore/Actor/PacketTunnelActor+ErrorState.swift
line 155 at r1 (raw file):
// Schedule task to reconnect. eventChannel.send(.reconnect(.current))
Won't that mean that if we're blocked because the current relay we're trying to connect to is offline, we will stay in the blocked state until the user actually selects a different relay ?
ios/PacketTunnelCore/Actor/State+Extensions.swift
line 191 at r1 (raw file):
- App update that requires settings schema migration. Packet tunnel will be automatically restarted after update but it would not be able to read settings until user opens the app which performs migration. - Packet tunnel will be automatically restarted when there is tunnel adapter error.
nit
when there is a tunnel adapter error
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 2 of 4 files reviewed, 2 unresolved discussions (waiting on @buggmagnet)
ios/PacketTunnelCore/Actor/PacketTunnelActor+ErrorState.swift
line 155 at r1 (raw file):
Previously, buggmagnet wrote…
Won't that mean that if we're blocked because the current relay we're trying to connect to is offline, we will stay in the blocked state until the user actually selects a different relay ?
it means whenever WireGuardAdapterError
happens then let's retry to connect the current one after a time interval . do you think if there us any cases in WireGuardAdapterError
that wether it happens we need to consider reconnecting to the new one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 2 of 4 files reviewed, 2 unresolved discussions (waiting on @mojganii)
ios/PacketTunnelCore/Actor/PacketTunnelActor+ErrorState.swift
line 155 at r1 (raw file):
Previously, mojganii wrote…
it means whenever
WireGuardAdapterError
happens then let's retry to connect the current one after a time interval . do you think if there us any cases inWireGuardAdapterError
that wether it happens we need to consider reconnecting to the new one?
Let's say the user has specifically selected gothenburg
and was connected to se-got-wg-001
, but it went offline due to maintenance.
Now we are going to try to connect to se-got-wg-001
despite it being offline, and the user will be in the blocked state despite having relays available.
My question is : Do we need to reconnect to the current relay to fix the problem ? Or can we fix the solution by selecting a random relay (within constraints) ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 2 of 4 files reviewed, 2 unresolved discussions (waiting on @buggmagnet)
ios/PacketTunnelCore/Actor/PacketTunnelActor+ErrorState.swift
line 155 at r1 (raw file):
Previously, buggmagnet wrote…
Let's say the user has specifically selected
gothenburg
and was connected tose-got-wg-001
, but it went offline due to maintenance.
Now we are going to try to connect tose-got-wg-001
despite it being offline, and the user will be in the blocked state despite having relays available.My question is : Do we need to reconnect to the current relay to fix the problem ? Or can we fix the solution by selecting a random relay (within constraints) ?
I see what you mean, when the current relay is out of order, with reconnecting to the current one we are captivating user while the rest relays are available but when user has no internet connection I believe it has to retry to connect to the current one,I think we have to have 2 different approaches:
1- user is offline due to bad or connection, then we have to connect to the current one for recovery
2- if internet is reachable but the relay doesn't respond try to go for the new one.
the current approach doesn't support both scenarios.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 4 of 4 files at r1, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @buggmagnet and @mojganii)
ios/PacketTunnelCore/Actor/PacketTunnelActor+ErrorState.swift
line 155 at r1 (raw file):
Previously, mojganii wrote…
I see what you mean, when the current relay is out of order, with reconnecting to the current one we are captivating user while the rest relays are available but when user has no internet connection I believe it has to retry to connect to the current one,I think we have to have 2 different approaches:
1- user is offline due to bad or connection, then we have to connect to the current one for recovery
2- if internet is reachable but the relay doesn't respond try to go for the new one.the current approach doesn't support both scenarios.
Until we make a complete solution that supports both scenarios I think we should try to connect to a new relay to make sure we cover scenario 2. It's a little unnecessary to change relay when the connection is bad, but it doesn't matter too much.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @buggmagnet)
ios/PacketTunnelCore/Actor/PacketTunnelActor+ErrorState.swift
line 155 at r1 (raw file):
Previously, rablador (Jon Petersson) wrote…
Until we make a complete solution that supports both scenarios I think we should try to connect to a new relay to make sure we cover scenario 2. It's a little unnecessary to change relay when the connection is bad, but it doesn't matter too much.
I agree. regardless if user's connection went bad or relay is unavailable, let app try to connect to the new one. @buggmagnet what do you think if we do reconnecting tor the new one for every scenarios?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @mojganii and @rablador)
ios/PacketTunnelCore/Actor/PacketTunnelActor+ErrorState.swift
line 155 at r1 (raw file):
Previously, mojganii wrote…
I agree. regardless if user's connection went bad or relay is unavailable, let app try to connect to the new one. @buggmagnet what do you think if we do reconnecting tor the new one for every scenarios?
I think this is the least likely to break other scenarios. So we put it back to .random
right ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @buggmagnet)
ios/PacketTunnelCore/Actor/PacketTunnelActor+ErrorState.swift
line 155 at r1 (raw file):
Previously, buggmagnet wrote…
I think this is the least likely to break other scenarios. So we put it back to
.random
right ?
yes.
004ae25
to
92a8787
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @buggmagnet)
ios/PacketTunnelCore/Actor/State+Extensions.swift
line 191 at r1 (raw file):
Previously, buggmagnet wrote…
nit
when there is a tunnel adapter error
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @buggmagnet)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 4 files at r1, 2 of 2 files at r2.
Reviewable status: complete! all files reviewed, all discussions resolved
92a8787
to
ecee6b6
Compare
🚨 End to end tests failed. Please check the failed workflow run. |
When the reason for the blocked state is a tunnel adapter error, we need to automatically recover the connection to attempt reconnection to the current relay, keeping the tunnel up to date. This PR introduces that functionality.
This change is