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

Add burst guard to macOS route monitor #5225

Merged
merged 2 commits into from
Oct 5, 2023
Merged

Conversation

dlon
Copy link
Member

@dlon dlon commented Oct 3, 2023

The route monitor currently immediately reacts to any route change. What happens is somewhat unpredictable because we're likely changing the routing table at the same time as the OS. We're also making many unnecessary changes to the routing table, and we might potentially end up constantly updating routes in a tight loop.

The PR fixes these issues by triggering route changes only after a minimum interval of time has elapsed.

Close DES-384.


This change is Reviewable

@dlon dlon requested a review from pinkisemils October 3, 2023 13:10
@linear
Copy link

linear bot commented Oct 3, 2023

DES-384 Debounce route messages

It's difficult to get predictable results now, since we end up changing routes at the same time as the system adds or removes routes. Reusing the "burst guard" from the Windows routing code would likely help with this.

We should perhaps still synthesize a "default route removed" event whenever a network switch occurs to deal with DNS/macOS's connectivity check.

@dlon dlon force-pushed the macos-add-routing-debounce branch 3 times, most recently from cce7f10 to c192a3d Compare October 3, 2023 14:36
@raksooo raksooo requested a review from Jontified October 5, 2023 08:46
Copy link
Collaborator

@pinkisemils pinkisemils left a comment

Choose a reason for hiding this comment

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

Reviewed 6 of 6 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @dlon)


talpid-routing/src/debounce.rs line 93 at r1 (raw file):

    /// Asynchronously trigger burst
    pub fn trigger(&self) {
        self.sender.send(BurstGuardEvent::Trigger).unwrap();

This shouldn't unwrap, or stop and stop_nonblocking should take consume self instead of taking a reference.


talpid-routing/src/unix/macos/mod.rs line 331 at r1 (raw file):

            | Ok(RouteSocketMessage::ChangeRoute(_))
            | Ok(RouteSocketMessage::AddAddress(_) | RouteSocketMessage::DeleteAddress(_)) => {
                self.update_trigger.trigger();

Ideally, the routes would be coalesced and we'd only react to changes in default routes - there's no need to take action if default routes have not been changed.

@dlon dlon force-pushed the macos-add-routing-debounce branch from c192a3d to dc157f6 Compare October 5, 2023 09:58
Copy link
Collaborator

@pinkisemils pinkisemils left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r2.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @dlon)

@dlon dlon force-pushed the macos-add-routing-debounce branch 2 times, most recently from 7d367f2 to 1582b7c Compare October 5, 2023 10:56
Copy link
Member Author

@dlon dlon left a comment

Choose a reason for hiding this comment

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

Reviewable status: 4 of 6 files reviewed, 1 unresolved discussion (waiting on @pinkisemils)


talpid-routing/src/debounce.rs line 93 at r1 (raw file):

Previously, pinkisemils (Emīls Piņķis) wrote…

This shouldn't unwrap, or stop and stop_nonblocking should take consume self instead of taking a reference.

Done.

@dlon dlon force-pushed the macos-add-routing-debounce branch from 1582b7c to 156c60d Compare October 5, 2023 11:10
Copy link
Collaborator

@pinkisemils pinkisemils left a 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 r3, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

Copy link
Contributor

@Jontified Jontified 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 3 of 6 files at r1, 1 of 1 files at r2, 2 of 2 files at r3, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@dlon dlon force-pushed the macos-add-routing-debounce branch from 156c60d to 4399583 Compare October 5, 2023 13:17
@dlon dlon merged commit bb1b0d0 into main Oct 5, 2023
25 checks passed
@dlon dlon deleted the macos-add-routing-debounce branch October 5, 2023 13:18
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