core/: Concurrent connection attempts #2248
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This was referenced Sep 26, 2021
Open
Thanks @AgeManning for testing! That is great to hear. Thanks @thomaseizinger and @elenaf9 for the reviews. Much appreciated! Good to have additional input on these changes. I addressed all your comments. Let me know in case you would like to see any additional changes. |
mxinden
commented
Oct 14, 2021
This was referenced Oct 14, 2021
mxinden
added a commit
to mxinden/rust-libp2p
that referenced
this pull request
Oct 20, 2021
With libp2p#2248 a connection task `await`s sending an event to the behaviour before polling for new events from the behaviour [1]. When `Swarm::poll` is unable to deliver an event to a connection task it returns `Poll::Pending` even though (a) polling `Swarm::network` might be able to make progress (`network_not_ready` being `false`) and (b) it does not register a waker to be woken up [2]. In combination this can lead to a deadlock where a connection task waits to send an event to the behaviour and `Swarm::poll` returns `Poll::Pending` failing to send an event to the connection task, not registering a waiker in order to be polled again. With this commit `Swarm::poll` will only return `Poll::Pending`, when failing to deliver an event to a connection task, if the network is unable to make progress (i.e. `network_not_ready` being `true`). In the long-run `Swarm::poll` should likely be redesigned, prioritizing the behaviour over the network, given the former is the control plane and the latter potentially yields new work from the outside. [1]: https://github.com/libp2p/rust-libp2p/blob/ca1b7cf043b4264c69b19fe75de488330a7a1f2f/core/src/connection/pool/task.rs#L224-L232 [2]: https://github.com/libp2p/rust-libp2p/blob/ca1b7cf043b4264c69b19fe75de488330a7a1f2f/swarm/src/lib.rs#L756-L783
10 tasks
mxinden
added a commit
that referenced
this pull request
Oct 26, 2021
) With #2248 a connection task `await`s sending an event to the behaviour before polling for new events from the behaviour [1]. When `Swarm::poll` is unable to deliver an event to a connection task it returns `Poll::Pending` even though (a) polling `Swarm::network` might be able to make progress (`network_not_ready` being `false`) and (b) it does not register a waker to be woken up [2]. In combination this can lead to a deadlock where a connection task waits to send an event to the behaviour and `Swarm::poll` returns `Poll::Pending` failing to send an event to the connection task, not registering a waiker in order to be polled again. With this commit `Swarm::poll` will only return `Poll::Pending`, when failing to deliver an event to a connection task, if the network is unable to make progress (i.e. `network_not_ready` being `true`). In the long-run `Swarm::poll` should likely be redesigned, prioritizing the behaviour over the network, given the former is the control plane and the latter potentially yields new work from the outside. [1]: https://github.com/libp2p/rust-libp2p/blob/ca1b7cf043b4264c69b19fe75de488330a7a1f2f/core/src/connection/pool/task.rs#L224-L232 [2]: https://github.com/libp2p/rust-libp2p/blob/ca1b7cf043b4264c69b19fe75de488330a7a1f2f/swarm/src/lib.rs#L756-L783
mxinden
added a commit
to mxinden/rust-libp2p
that referenced
this pull request
Jan 12, 2022
Since libp2p#2248 dial attempts are no longer reported per address, but instead reported for all addresses of a single dial at once. This commit updates the comment accordingly.
mxinden
added a commit
that referenced
this pull request
Jan 14, 2022
Since #2248 dial attempts are no longer reported per address, but instead reported for all addresses of a single dial at once. This commit updates the comment accordingly.
mxinden
added a commit
to mxinden/rust-libp2p
that referenced
this pull request
Jan 15, 2022
Since libp2p#2248 dial attempts are no longer reported per address, but instead reported for all addresses of a single dial at once. This commit updates the comment accordingly.
santos227
pushed a commit
to santos227/rustlib
that referenced
this pull request
Jun 20, 2022
…304) With libp2p/rust-libp2p#2248 a connection task `await`s sending an event to the behaviour before polling for new events from the behaviour [1]. When `Swarm::poll` is unable to deliver an event to a connection task it returns `Poll::Pending` even though (a) polling `Swarm::network` might be able to make progress (`network_not_ready` being `false`) and (b) it does not register a waker to be woken up [2]. In combination this can lead to a deadlock where a connection task waits to send an event to the behaviour and `Swarm::poll` returns `Poll::Pending` failing to send an event to the connection task, not registering a waiker in order to be polled again. With this commit `Swarm::poll` will only return `Poll::Pending`, when failing to deliver an event to a connection task, if the network is unable to make progress (i.e. `network_not_ready` being `true`). In the long-run `Swarm::poll` should likely be redesigned, prioritizing the behaviour over the network, given the former is the control plane and the latter potentially yields new work from the outside. [1]: https://github.com/libp2p/rust-libp2p/blob/20e22ae696237ccd4e32f044d508ab1a88f53a9b/core/src/connection/pool/task.rs#L224-L232 [2]: https://github.com/libp2p/rust-libp2p/blob/20e22ae696237ccd4e32f044d508ab1a88f53a9b/swarm/src/lib.rs#L756-L783
santos227
added a commit
to santos227/rustlib
that referenced
this pull request
Jun 20, 2022
Since libp2p/rust-libp2p#2248 dial attempts are no longer reported per address, but instead reported for all addresses of a single dial at once. This commit updates the comment accordingly.
24 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Main feature
Concurrently dial candidates within a single dial attempt.
For the concrete interface changes, see the changelog entries in
core/CHANGELOG.mdandswarm/CHANGELOG.md.Main motivation for this feature is to increase success rate on hole punching (see #1896 (comment) for details). Though, as a nice side effect, as one would expect, it does improve connection establishment time.
95th percentile duration of a Kademlia GetClosestPeers request:
50th percentile duration of a Kademlia GetClosestPeers request:
You can explore this data and more yourself at the URL below. On the left you have a Kademlia Exporter running with this feature, on the right you have a Kademlia Exporter running without.
https://kademlia-exporter.max-inden.de/d/Pfr0Fj6Mk/rust-libp2p?orgId=1&var-data_source=Prometheus&var-instance=kademlia-exporter-ipfs-concurrent:8080&var-instance=kademlia-exporter-ipfs:8080&from=now-3h&to=now&refresh=10s
For now, the concurrency factor is not configurable for the user. I think a value of 5 is a sane default. In the future I am fine exposing this value as a configuration parameter, though I would suggest we stick with this value as a first iteration.See #2248 (comment).
Cleanups and fixes done along the way
Merge
pool.rsandmanager.rs.Instead of manually implementing state machines in
task.rsuseasync/await.Fix bug where
NetworkBehaviour::inject_connection_closedis called without a previousNetworkBehaviour::inject_connection_established(see Invalid Swarm behaviour #2242).Return handler to behaviour on incoming connection limit error. I missed this case in Invalid Swarm behaviour #2242.
Additional notes
I am sorry this ended up as such a large change. In my eyes it simplifies many of the somewhat dated structures in
libp2p-core.