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

flaky test: iroh::sync sync_full_basic #2512

Open
flub opened this issue Jul 16, 2024 · 0 comments
Open

flaky test: iroh::sync sync_full_basic #2512

flub opened this issue Jul 16, 2024 · 0 comments
Assignees

Comments

@flub
Copy link
Contributor

flub commented Jul 16, 2024

https://github.com/n0-computer/iroh/actions/runs/9955096511/job/27502132950?pr=2509

failure output: https://gist.github.com/flub/bbe45a7bd6c1cb92eecfd3731bfd7576

github-merge-queue bot pushed a commit that referenced this issue Jul 18, 2024
## Description

When we had any direct addresses but not yet a best address we would
randomly try to pick one and send to it on the off chance it worked.
This required to always pick the same direct addr to send to which was
not happening and a bug.

However the scenarios in which this would speed up holepunching were
very unlikely: only really if a single direct addr known to work was
added manually would this improve things.

Simply removing this feature is rather nice:

- The direct connection still happens very quickly in the best-case
  scenario: a single ping-pong roundtrip is all it costs.  No relay
  server or discovery mechanism needed so no functionality change.

- In all other scenarios there is no change.  Connection establishment
  behaves exactly the same as before.

- There are no more confusing connection type changes at startup.
  This would go relay -> mixed (-> mixed a few times more due to the
  bug) -> direct.  Now it just goes relay -> direct which is intuitively
  what people would expect.

The simplest way to test this is by running doctor accept and connect
with direct addrs only and observe the holepunching events emitted.


## Breaking Changes

none

## Notes & open questions

This is an alternative to #2487.  And my preferred solution.

Fixes #2480.

See #2512 for the flaky test.

test_two_devices_roundtrip_network_change managed to hit a timeout on
cross CI.  Give it some more time.

## Change checklist

- [x] Self-review.
- [x] Documentation updates following the [style
guide](https://rust-lang.github.io/rfcs/1574-more-api-documentation-conventions.html#appendix-a-full-conventions-text),
if relevant.
- [x] Tests if relevant.
- [x] All breaking changes documented.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: No status
Development

No branches or pull requests

2 participants