Improve connection management #266
Merged
Conversation
This file contains hidden or 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
tnull
force-pushed
the
2024-03-introduce-connection-manager
branch
2 times, most recently
from
7b4b7d5 to
5b56a8b
Compare
Merged
jkczyz
reviewed
Mar 4, 2024
tests/integration_tests_rust.rs
Outdated
| let node_id_b = node_b.node_id(); | ||
| let node_addr_b = node_b.listening_addresses().unwrap().first().unwrap().clone(); | ||
|
|
||
| std::thread::sleep(std::time::Duration::from_secs(1)); |
Contributor
There was a problem hiding this comment.
Could you add a comment on why the sleep is needed? Is there any way to avoid it?
Collaborator
Author
There was a problem hiding this comment.
Added a comment here as well as to do_connection_restart_behavior. The sleep is necessary to allow the listening task of the connected node to spawn and bind successfully before we try connecting to it.
It could be avoidable if we introduced a mechanism to Node::start that would wait for the task to come up. However, I currently don't think it's worth it as on the contrary we try to keep Node::startas quick as possible, i.e., try to push everything possible to background tasks in order to improve startup time.
Contributor
There was a problem hiding this comment.
Could we have an is_listening method, even if only for testing?
Collaborator
Author
tnull
force-pushed
the
2024-03-introduce-connection-manager
branch
from
5b56a8b to
5c9e930
Compare
tnull
force-pushed
the
2024-03-introduce-connection-manager
branch
from
5c9e930 to
785543d
Compare
tnull
force-pushed
the
2024-03-introduce-connection-manager
branch
3 times, most recently
from
e781c08 to
d60f009
Compare
jkczyz
reviewed
Mar 8, 2024
tnull
force-pushed
the
2024-03-introduce-connection-manager
branch
from
d60f009 to
c971535
Compare
Collaborator
Author
|
Rebased after #272 was merged to resolve minor conflicts. |
tnull
force-pushed
the
2024-03-introduce-connection-manager
branch
2 times, most recently
from
2efc3b9 to
f2f1ab2
Compare
tnull
force-pushed
the
2024-03-introduce-connection-manager
branch
from
f2f1ab2 to
7d6e260
Compare
Collaborator
Author
|
Rebased on #244. |
jkczyz
reviewed
Apr 25, 2024
tnull
force-pushed
the
2024-03-introduce-connection-manager
branch
from
eac8e26 to
85a6803
Compare
.. just a minor cleanup to further modularize the codebase. Also, we'll be reusing these methods in `Event::ConnectionNeeded` soon.
.. we should consider dropping `Deref` and instead just commiting to store a `Arc<L>` everwhere, as it gets tedious to maintain. However, this is barely scraping by and the least invasive change here.
.. which is a bit more readable and in-line what we do other places, plus it allows us to drop the `futures` dependency.
... we check that we can successfully issue concurrent connection attempts, which all succeed.
Previously, concurrent calls to `do_connect_peer`/`connect_peer_if_necessary` could result in multiple connections being opened, just to be closed as redundant shortly after. Parallel connection attempt were therefore prone to fail. Here, we introduce a `ConnectionManager` that implements a pub/sub pattern: upon the initial call, the task responsible for polling the connection future to completion registers that a connection is in-flight. Any calls following will check this and register a `oneshot` channel to be notified of the `Result`.
tnull
force-pushed
the
2024-03-introduce-connection-manager
branch
from
85a6803 to
77c538b
Compare
Collaborator
Author
|
Rebased on main and squashed fixups without further changes. |
jkczyz
approved these changes
Apr 25, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Based on #243.
Based on #244.
As a pre-factor to #256, we clean up our connection handling. Namely, we move it to a new
connection.rsfile and clean up the logic a bit, allowing us to drop the dependency on thefuturescrate.Then, we introduce a
ConnectionManagerthat handles pending connection attempts. Previously, concurrent calls todo_connect_peer/connect_peer_if_necessarycould result in multiple connections being opened, just to be closed as redundant shortly after. Here, we fix this behavior by ensuring only one connection attempt is inflight at any given point in time.