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

Ensure atomicity between (re)connection attempts #5273

Merged
merged 1 commit into from
Oct 13, 2023
Merged

Conversation

pronebird
Copy link
Contributor

@pronebird pronebird commented Oct 11, 2023

Currently reconnection is a two fold operation:

  • First we increment attempt counter
  • Then schedule command for reconnect to leverage coalescing system.

This approach produces two individual changes to state which can be misleading when observed from the outside. For instance:

  • If the tunnel is currently in .connected state, the first mutation increments attempt counter updating it to 1.
  • The observer then receives a .connected state with attempt counter updated to 1.
  • The second mutation changes state to .reconnecting with attempt counter set to 1.
  • The observer then receives a .reconnecting state with attempt counter set to 1.

Because connected, reconnecting, connecting were handled the same way, the tunnel losing connectivity also starts connection attempt counter at 1 which is suboptimal.

This PR aims to fix the aforementioned issues by introducing ReconnectReason which is passed along with reconnect() command in place of shouldStopTunnelMonitor parameter. tryStart() then is responsible for incrementing the attempt counter following these rules:

  • The counter is not incremented If current state is .connected. That's because the tunnel is going to transition to .reconnecting phase and attempt counter should start at zero during the first attempt, then increment with each subsequent attempt.
  • The counter is incremented if current state is either .connecting or .reconnecting as normal.

Two tests added to ensure that state transitions for (re)connection attempts happen as expected.


This change is Reviewable

@pronebird pronebird force-pushed the atomic-reconnect branch 3 times, most recently from c0051b8 to 9ab1183 Compare October 11, 2023 15:05
@pronebird pronebird added the iOS Issues related to iOS label Oct 12, 2023
Copy link
Contributor

@rablador rablador left a comment

Choose a reason for hiding this comment

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

Reviewed 3 of 6 files at r1, 3 of 3 files at r2, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

Copy link
Contributor

@buggmagnet buggmagnet left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 6 of 6 files at r1, 3 of 3 files at r2, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@buggmagnet buggmagnet merged commit 17296fc into main Oct 13, 2023
4 checks passed
@buggmagnet buggmagnet deleted the atomic-reconnect branch October 13, 2023 13:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
iOS Issues related to iOS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants