Retry fetching out IP DES-118

[Serock3]

Add periodic retry for GetCurrentLocation.

I can not reproduce the original error so I haven't been able to make the new code actually run. We need to see if this fixes the problem, and decide what an appropriate retry interval and max attempts are.

I tested by blocking traffic to am.i.mullvad.net, and it seems to work as expected.

---

This change is 

[linear]

DES-118 Out IP is sometimes missing

I've had this happen on both Windows 11 and macOS.

In a lot of situations the out-IP was missing from the connection info. My guess is that the am.i.mullvad.net check failed?

I get the following error as well:

[2023-01-09 14:47:11.636][mullvad_api::rest][ERROR] Error: HTTP request failed
Caused by: Request timed out
Caused by: deadline has elapsed

I am able to curl https://am.i.mullvad.net.

To reproduce:

1. Connect
2. Open the connection panel

The out IP is not missing all of the time but is most of the time both in my Windows 11 VM and on the office NUC

Proposed solution

David in a comment:

The daemon could handle the retries. I think that this would be preferable to forcing frontends to do so, and it would be consistent across platforms.

For a short/immediate strategy, GetCurrentLocation could just retry a couple of times when there's a timeout/network error, just like many other RPCs do.

For a long/back off strategy, the daemon should probably send an event to update the location/IP. No need for the RPC.

[dlon]

Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @Serock3)

---

mullvad-daemon/src/lib.rs line 97 at r1 (raw file):

/// Retry interval for fetching location
const RETRY_LOCATION_STRATEGY: ConstantInterval =
    ConstantInterval::new(Duration::from_secs(2), Some(3));

The reason for setting the interval to 0 in other cases is that there's also a timeout interval on the REST requests themselves. It's a bit confusing, but that means that it's already only retrying once every few seconds, if the request fails due to a timeout/network error.

So it should be fine do so the same thing here.

---

mullvad-daemon/src/lib.rs line 1394 at r1 (raw file):

        let use_ipv6 = self.settings.tunnel_options.generic.enable_ipv6;
        let api_handle = self.api_handle.availability.clone();
        retry_future(

We should avoid blocking the daemon from handling further commands here. So it would be better to spawn a new tokio task and handle the requests there.

[Serock3]

Reviewable status: 0 of 2 files reviewed, 3 unresolved discussions (waiting on @dlon)

---

mullvad-api/src/lib.rs line 355 at r2 (raw file):

    /// Returns a new request service handle
    pub async fn rest_handle(&mut self) -> rest::RequestServiceHandle {

This change is really unrelated, but I snuck it in.

---

mullvad-daemon/src/lib.rs line 97 at r1 (raw file):

Previously, dlon (David Lönnhager) wrote…

The reason for setting the interval to 0 in other cases is that there's also a timeout interval on the REST requests themselves. It's a bit confusing, but that means that it's already only retrying once every few seconds, if the request fails due to a timeout/network error.

So it should be fine do so the same thing here.

Alright. After some testing it feels like the REST API adds ~5 secs on its own, which should do.

This may be another argument for writing how retrys work (in addition to handling the Reply-after header`). The delay timer you give should be respected.

---

mullvad-daemon/src/lib.rs line 1394 at r1 (raw file):

Previously, dlon (David Lönnhager) wrote…

We should avoid blocking the daemon from handling further commands here. So it would be better to spawn a new tokio task here and handle the requests there.

I knew the nested future that I removed had some purpose! Good catch. I have made the RPC non-blocking again, but hopefully with a slightly more readable implementation. I also tested that the daemon remains responsive (through the CLI) while it is performing the retrys.

[dlon]

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

---

mullvad-daemon/src/lib.rs line 1394 at r1 (raw file):

Previously, Serock3 (Sebastian Holmin) wrote…

I knew the nested future that I removed had some purpose! Good catch. I have made the RPC non-blocking again, but hopefully with a slightly more readable implementation. I also tested that the daemon remains responsive (through the CLI) while it is performing the retrys.

___ *[`mullvad-daemon/src/lib.rs` line 1407 at r2](https://reviewable.io/reviews/mullvad/mullvadvpn-app/5486#-NkGhJKz1qxgT_QuB5Le:-NkGhJKz1qxgT_QuB5Lf:bje0qp2) ([raw file](https://github.com/mullvad/mullvadvpn-app/blob/c035bd5b19726e7d08d005fa74b2a2d597c81b63/mullvad-daemon/src/lib.rs#L1407)):* > ```XML > log::debug!("Fetching GeoIpLocation"); > match retry_future( > move || geoip::send_location_request(rest_service.clone(), use_ipv6), > ```

I wonder if we shouldn't move the retry logic into geoip. Just a thought.

[Serock3]

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

---

mullvad-daemon/src/lib.rs line 1407 at r2 (raw file):

Previously, dlon (David Lönnhager) wrote…

I wonder if we shouldn't move the retry logic into geoip. Just a thought.

I agree. I moved the retry logic to geoip and refactored mullvad-daemon/lib a bit to make it a bit more concise. The behavior should be the same, but you may have to look at it again.

[dlon]

Reviewed 2 of 2 files at r3, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved