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

Rework offline detection on Linux #2377

Merged
merged 2 commits into from Jan 11, 2021
Merged

Conversation

pinkisemils
Copy link
Collaborator

@pinkisemils pinkisemils commented Jan 7, 2021

To improve offline detection on Linux and desktop in general, I've done two things:

  • Add an environment variable to allow disabling the offline check entirely, leaving it in an open state. This could later be reworked to being a proper option.
  • Change offline detection on Linux to query the routing table for a path to a public IPv4 address to infer connectivity.

This change is Reviewable

@pinkisemils pinkisemils requested review from faern and dlon January 7, 2021 10:52
@pinkisemils pinkisemils force-pushed the linux-improve-offline-detection branch 2 times, most recently from e3643b1 to b2c8b3d Compare January 7, 2021 11:32
Copy link
Member

@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.

:lgtm:

Reviewed 4 of 4 files at r1.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @pinkisemils)


talpid-core/src/offline/linux.rs, line 118 at r1 (raw file):

async fn public_ip_not_reachable(handle: &Handle) -> Result<bool> {

I think this should be public_ip_reachable (i.e., not negated).

Copy link
Collaborator Author

@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.

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @dlon)


talpid-core/src/offline/linux.rs, line 118 at r1 (raw file):

Previously, dlon (David Lönnhager) wrote…

I think this should be public_ip_reachable (i.e., not negated).

public_ip_unreachable() would be better still IMO, since every time this function is called, it's return value is assigned to is_offline in one way or another. Or do you think that !pubilc_ip_reachable() is better?

Copy link
Member

@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: all files reviewed, 1 unresolved discussion (waiting on @pinkisemils)


talpid-core/src/offline/linux.rs, line 118 at r1 (raw file):

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

public_ip_unreachable() would be better still IMO, since every time this function is called, it's return value is assigned to is_offline in one way or another. Or do you think that !pubilc_ip_reachable() is better?

You're right.

Copy link
Member

@faern faern 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 4 files at r1.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @pinkisemils)

Copy link
Member

@faern faern 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: all files reviewed, 2 unresolved discussions (waiting on @pinkisemils)


talpid-core/src/offline/linux.rs, line 52 at r1 (raw file):

}

const PUBLIC_INTERNET_ADDRESS: Ipv4Addr = Ipv4Addr::new(193, 138, 218, 78);

Can we please leave a comment on such a magical constant? One that explains that any public IP should do, but we stick to one owned my Mullvad.

Copy link
Member

@faern faern 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: all files reviewed, 2 unresolved discussions (waiting on @dlon and @pinkisemils)


talpid-core/src/offline/linux.rs, line 118 at r1 (raw file):

Previously, dlon (David Lönnhager) wrote…

You're right.

I vote for anything that does not have a negation in the name. Evaluating "is not reachable: true" in my head always leads to smoke coming out of my ears. "Is unreachable: true" or "is reachable: false" is way easier to process.

Copy link
Member

@faern faern 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: all files reviewed, 3 unresolved discussions (waiting on @dlon and @pinkisemils)


talpid-core/src/offline/mod.rs, line 42 at r1 (raw file):

    #[cfg(target_os = "android")] android_context: AndroidContext,
) -> Result<MonitorHandle, Error> {
    let monitor = match std::env::var(TALPID_DISABLE_OFFLINE_MONITOR)

Maybe can be made simpler and more consistent if implemented with a lazy_static like the other env variable checks? Then the constant can contain the actual value and not just the name of the env var to check

static ref FORCE_USERSPACE_WIREGUARD: bool = env::var("TALPID_FORCE_USERSPACE_WIREGUARD")
.map(|v| v != "0")
.unwrap_or(false);

Also easier to verify that the logic is the same as the others

@pinkisemils pinkisemils force-pushed the linux-improve-offline-detection branch from b2c8b3d to b673a75 Compare January 7, 2021 16:19
Copy link
Collaborator Author

@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.

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @faern)


talpid-core/src/offline/linux.rs, line 52 at r1 (raw file):

Previously, faern (Linus Färnstrand) wrote…

Can we please leave a comment on such a magical constant? One that explains that any public IP should do, but we stick to one owned my Mullvad.

Done.

Copy link
Member

@faern faern 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: 2 of 4 files reviewed, 1 unresolved discussion (waiting on @dlon and @pinkisemils)


talpid-core/src/offline/mod.rs, line 25 at r2 (raw file):

lazy_static::lazy_static! {
    /// Disables offline monitor
    static ref FORCE_DISABLE_OFFLINE_MONITOR: bool = std::env::var("TALPID_FORCE_USERSPACE_WIREGUARD")

USERSPACE_WIREGUARD?

Copy link
Collaborator Author

@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.

Reviewable status: 2 of 4 files reviewed, 1 unresolved discussion (waiting on @dlon and @faern)


talpid-core/src/offline/mod.rs, line 25 at r2 (raw file):

Previously, faern (Linus Färnstrand) wrote…

USERSPACE_WIREGUARD?

Done.

Copy link
Member

@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.

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

Copy link
Member

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

@pinkisemils pinkisemils force-pushed the linux-improve-offline-detection branch from 59b0c3b to 4d95fa2 Compare January 11, 2021 10:29
Copy link
Member

@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.

Reviewed 2 of 2 files at r4.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@pinkisemils pinkisemils force-pushed the linux-improve-offline-detection branch from 4d95fa2 to b29cd20 Compare January 11, 2021 12:33
@pinkisemils pinkisemils merged commit 6c03c63 into master Jan 11, 2021
@pinkisemils pinkisemils deleted the linux-improve-offline-detection branch January 11, 2021 12:33
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.

None yet

3 participants