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

fix(iroh-gossip): do not drop existing peer connection when we get incoming one #2310

Closed

Conversation

link2xt
Copy link
Contributor

@link2xt link2xt commented May 19, 2024

Otherwise if two peers try to connect to each other at the same time, they may end up creating two connection and then closing both of them. Better keep two one-directional connections.

deltachat-core-rust will have a downstream test that reliably triggers the problem at deltachat/deltachat-core-rust#5595
I did not manage to reproduce the problem in Rust, it seems to depend on timing and is triggered in the test which sends tickets around using SMTP.

Fixes #2307

Replaces #2308

loop {
tokio::select! {
biased;
msg = send_rx.recv() => {
msg = send_rx.recv(), if tx_open => {
Copy link
Contributor

@divagant-martian divagant-martian May 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this can be done with send_rx.is_closed()

Copy link
Contributor Author

@link2xt link2xt May 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have force-pushed already, but is_closed() looks better IMO.

@Frando
Copy link
Member

Frando commented May 21, 2024

This looks sensible to me!

When rereading the connection handling in iroh_gossip::net, I think this could need some more love, which I will try to have a look at soon. We really should have only a single connection between two peers, and disambiguate by node id order like we do in iroh-sync. Currently I think it can happen, depending on timings, that there are actually two connections between peer A and peer B (one from A dialing B, and one from B dialing A) which does not matter because they are handled in the same way but still is unfortunate. However this is independent of this concrete issue, so I am fine with merging this if it does fix the issue deltachat has. And I will look into a better way to structure the connection code independently.

@link2xt
Copy link
Contributor Author

link2xt commented May 21, 2024

This also should be merged to the main besides maint-0.16, but for delta chat would be nice to have it at least as a 0.16 bugfix release, then there is a chance we get (disabled-by-default) realtime channels in the next desktop release.

@dignifiedquire
Copy link
Contributor

maint-0.16 exist because there were issues when cutting the release, given that we are cutting a release every 2-3 weeks currently I would much more prefer to merge this into main and DC just upgrading to that commit. And you can switch over to the released version in a week.

The breaking changes on main are mostly renames which are easy to adapt to.

@link2xt
Copy link
Contributor Author

link2xt commented May 22, 2024

Replaced with #2318 which is based on main

@link2xt link2xt closed this May 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

iroh-gossip fails to establish connection if it is established simultaneously from both sides
4 participants