Add unit tests to MTU detection

[Serock3]

Fixes DES-675.

Split MTU detection into an inner pure function max_ping_sized
and an io-dependent outer function detect_mtu. Add unit and prop-testing to
max_ping_sized.

---

This change is 

[dlon]

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

---

talpid-wireguard/src/mtu_detection.rs line 115 at r1 (raw file):

    // detection to set a low value. Here we manually increase this value, which fixes
    // the problem.
    // TODO: Make sure this fix is not needed for any other target OS

Should probably be moved to linear rather than be left as a comment here

---

talpid-wireguard/src/mtu_detection.rs line 148 at r1 (raw file):

                // Validate the received ping response
                {

Nit: Is there a point to placing this inside a block?

---

talpid-wireguard/src/mtu_detection.rs line 170 at r1 (raw file):

/// Consumes a stream of pings, and returns the largest packet size within a given timeout from the
/// first ping response. Short circuits on errors.

Might be worth clarifying that this only works as expected with FuturesUnordered? Or even have it only accept FutureUnordered. Unless that complicates the tests.

---

talpid-wireguard/src/mtu_detection.rs line 240 at r1 (raw file):

    async fn all_pings_ok() {
        let pings = stream::iter((0..=100).rev().map(Ok));
        let max = max_ping_size(pings, Duration::from_millis(10))

Might it be possible to advance the timer here without actually waiting at all? E.g. using https://docs.rs/tokio/latest/tokio/time/fn.advance.html

---

talpid-wireguard/src/mtu_detection.rs line 260 at r1 (raw file):

    }

    /// The [`PING_OFFSET_TIMEOUT`] is counted from the return of the first ping, not from the

Is this actually what is being tested? Does it not need to check whether the timeout equals offset_timeout + timeout of the first ping to accomplish that?

---

talpid-wireguard/src/mtu_detection.rs line 310 at r1 (raw file):

    /// return an error instead of trusting result.
    #[tokio::test]
    async fn max_timeout_error() {

Should the test not arguably make sure that this cannot happen instead?

[Serock3]

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @dlon)

---

talpid-wireguard/src/mtu_detection.rs line 115 at r1 (raw file):

Previously, dlon (David Lönnhager) wrote…

Should probably be moved to linear rather than be left as a comment here

You're probably right. Though we have tested it on all platforms without finding any issues with this so far. I guess I left the note just to make it easier to debug in case similar issues turn up in the future.

---

talpid-wireguard/src/mtu_detection.rs line 148 at r1 (raw file):

Previously, dlon (David Lönnhager) wrote…

Nit: Is there a point to placing this inside a block?

No. Just thought it may be more readable that it doesn't affect the return value.

---

talpid-wireguard/src/mtu_detection.rs line 170 at r1 (raw file):

Previously, dlon (David Lönnhager) wrote…

Might be worth clarifying that this only works as expected with FuturesUnordered? Or even have it only accept FutureUnordered. Unless that complicates the tests.

I see your point. The function has some intuitive behavior for arbitrary streams when it comes to the timeout. It may be possible to re-write the tests to also use FutureUnordered directly, but I'm not sure it's worth the effort.

---

talpid-wireguard/src/mtu_detection.rs line 240 at r1 (raw file):

Previously, dlon (David Lönnhager) wrote…

Might it be possible to advance the timer here without actually waiting at all? E.g. using https://docs.rs/tokio/latest/tokio/time/fn.advance.html

Good find! I thought that there should be some way to mock the timeout but couldn't find anything. I will look into using that function.

---

talpid-wireguard/src/mtu_detection.rs line 260 at r1 (raw file):

Previously, dlon (David Lönnhager) wrote…

Is this actually what is being tested? Does it not need to check whether the timeout equals offset_timeout + timeout of the first ping to accomplish that?

I would say so. It sets the ping_offset_timeout param to 5 ms and then sends all the pings in a single burst after 10 ms. If the timeout counter would have started from the fn call they would have timed out, but since they arrive close to each other in time they do not.

---

talpid-wireguard/src/mtu_detection.rs line 310 at r1 (raw file):

Previously, dlon (David Lönnhager) wrote…

Should the test not arguably make sure that this cannot happen instead?

Not sure how I would prevent it from happening. This edge-case would trigger if e.g. half of the pings arrive after 9 s and the other half are delayed more than 10 s. In that case you should not pick the largest packet size of the first half of ping responses, but instead abort. Since I use build in timeout of surge_ping for the long PING_TIMEOUT I cannot ignore this timeout.

[Serock3]

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

---

talpid-wireguard/src/mtu_detection.rs line 170 at r1 (raw file):

Previously, Serock3 (Sebastian Holmin) wrote…

I see your point. The function has some intuitive behavior for arbitrary streams when it comes to the timeout. It may be possible to re-write the tests to also use FutureUnordered directly, but I'm not sure it's worth the effort.

I made it only accept FutureUnordered

---

talpid-wireguard/src/mtu_detection.rs line 240 at r1 (raw file):

Previously, Serock3 (Sebastian Holmin) wrote…

Good find! I thought that there should be some way to mock the timeout but couldn't find anything. I will look into using that function.

Done. Turned out really well I think!

---

talpid-wireguard/src/mtu_detection.rs line 260 at r1 (raw file):

Previously, Serock3 (Sebastian Holmin) wrote…

I would say so. It sets the ping_offset_timeout param to 5 ms and then sends all the pings in a single burst after 10 ms. If the timeout counter would have started from the fn call they would have timed out, but since they arrive close to each other in time they do not.

See the new implementation after the above changes

[dlon]

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

---

talpid-wireguard/src/mtu_detection.rs line 260 at r1 (raw file):

Previously, Serock3 (Sebastian Holmin) wrote…

See the new implementation after the above changes

Nice :)

[linear]

DES-675 Add unit tests