feat: select a working iroh relay among all candidates from published relays - #8490
feat: select a working iroh relay among all candidates from published relays#8490hpk42 wants to merge 1 commit into
Conversation
| // The endpoint is initialized again on the next use | ||
| // because it has no working relay and is not in use yet. | ||
| announce_relay(alice, "three@example.net", "https://192.0.2.1").await?; | ||
| announce_relay(alice, "four@example.net", "https://nine.testrun.org").await?; |
There was a problem hiding this comment.
This test relies on nine.testrun.org running?
There was a problem hiding this comment.
i force-pushed and changed it to ci-chatmail.testrun.org with a comment -- it's an online dependency but i think it's ok. Python core tests also depend on it.
There was a problem hiding this comment.
Four of the existing peer_channels tests (on main) also run forever without network on main, so, this online dependency is not new, and it's good that all of them are in the same module
78707ba to
37c46b6
Compare
… relays If there are no Iroh relay candidates from the chatmail relays, try nine.testrun.org's iroh relay (for now, soon to be dropped as well) instead of falling back to Iroh's default Number0 relay. Also try to minimize holding locks everywhere. Note that iroh endpoints do not provide any way to determine if the relay is alive and connected if there is no /ping endpoint which currently deployed relays do not offer, so for now and the next couple of months, probing /generate_204 endpoint for http success is the only discriminator i found for a relay working.
37c46b6 to
4c7ad15
Compare
Hocuri
left a comment
Hocuri
left a comment
There was a problem hiding this comment.
I must say, I find it hard to think through all the cases where a deadlock or some other problem might arise.
I have three comments, two of them are minor, one is a concern about iroh potentially continuing to run forever (or, until the Delta Chat process is killed).
| const PUBLIC_KEY_STUB: &[u8] = "static_string".as_bytes(); | ||
|
|
||
| /// Timeout for probing an iroh relay candidate. | ||
| const RELAY_PROBE_TIMEOUT: Duration = Duration::from_secs(5); |
There was a problem hiding this comment.
5 seconds seems pretty short? Especially since we don't wait for all probes to finish, and just use the one that returns first, so that as long as any connection works, everything works fine.
Also, some of the test runners (macOS I think) were sometimes randomly stopped, which caused tests with short timeouts to be flaky, but idk whether that's a problem here.
What about increasing this to 30 or 60, so that iroh works fine on a very slow internet connection?
There was a problem hiding this comment.
i thought about a longer timeout (like we usually do) but it's only for probing, and chatmail.at listed relays responded within a second to my queries, and as it's about "realtime" i am not sure if we even want to try setting up realtime if the probing http request already fails. But ok, i guess we can still wait. Maybe the internet is bad, but people are in a local WiFI and it's nice if it eventually works. As far as i know, btw, iroh might not be happy with slow networks at all.
| stale.close().await.log_err(self).ok(); | ||
| } | ||
|
|
||
| let iroh = self.init_peer_channels().await?; |
There was a problem hiding this comment.
I'm concerned that initializing without holding a lock on iroh may lead to a bug where iroh sometimes continues running forever:
Previously, the lock was held while init_peer_channels() ran. Now it's not, which means that calling stop_io() or leave_webxdc_realtime() while initializing peer channels will lead to an open iroh channel even after io was stopped. I'm not deep into iroh, but it seems to me like the previous behavior was better, in order to prevent weird bug reports where iroh is still running even though it should not. (IIRC, in the past we did have a bug like this, and some users complained about it, so it's not just a problem in theory not to stop iroh connections)
There was a problem hiding this comment.
I agree the Iroh lock/guard/state situation is tricky. It should become simpler and safer to reason about it. Also "peer_channels" is a strange name, when it really is the iroh end point that might have some channels. I am putting the PR to draft for now.
| let mut tcm = TestContextManager::new(); | ||
| let alice = &mut tcm.alice().await; |
There was a problem hiding this comment.
Nit: Alice doesn't need to be mutable
| let mut tcm = TestContextManager::new(); | |
| let alice = &mut tcm.alice().await; | |
| let alice = &TestContext::new_alice().await; |
3 participants
If there are no working Iroh relay candidates from the chatmail relays, try nine.testrun.org's iroh relay (for now, soon to be dropped as well) instead of falling back to Iroh's default Number0 relay.
Note that iroh endpoints do not provide any way to determine if the relay is alive and connected -- the endpoint will happily connect to broken URLs and determine a home_server() that doesn't work. I think it's because there is no /ping endpoint which currently deployed relays do not offer (fixed in chatmail/relay#1029 )
So for now and the next couple of months, probing the /generate_204 endpoint for http success
is the only discriminator i found for a relay working. Currently known to be broken iroh relays from the chatmail.at list (even though they return metadata values for it) are: chat.adminforge.de and chatmail.woodpeckersnest.space. I tried creating a profile with these and a working relay, and realtime channel connected (via a working realy, ignoring the broken ones).
The PR also tries to minimize holding locks everywhere, and restricts iroh relay candidates to come from published relays. Also, hidden/unpublished relays are ignored.