Make sure we're connected before starting requests #2297
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
![mergify[bot]](https://avatars.githubusercontent.com/in/10562?s=60&v=4){kind=small}
twiggy diff reportDifference in .wasm size before and after this pull request. |
melekes
reviewed
May 18, 2022
src/libp2p/peers.rs
Outdated
| @@ -1171,6 +1171,20 @@ where | |||
| self.inner.respond_in_request(id.0, response) | |||
| } | |||
|
|
|||
| /// Returns `true` if there exists an established connection with the given peer. | |||
| pub fn has_established_connection(&self, peer: &PeerId) -> bool { | |||
There was a problem hiding this comment.
Suggested change
| pub fn has_established_connection(&self, peer: &PeerId) -> bool { | |
| pub fn has_established_connection(&self, peer_id: &PeerId) -> bool { |
| None => return false, | ||
| }; | ||
|
|
||
| self.connections_by_peer |
There was a problem hiding this comment.
why would we want to have multiple connections to the same peer?
There was a problem hiding this comment.
This can happen accidentally if two peers connect to each other at the same time.
Another situation is if you connect to an IP address and expect to find peer A but actually connect to peer B, even though you're already connected to B.
In that case, from the point of view of peer B, maybe you have accidentally connected to it (the example that I just gave), but maybe the first connection you had with B is actually dead and you're reopening a new one.
The best way to handle these situations is to allow multiple connections between peers, and automatically close unused ones.
src/network/service.rs
Outdated
| @@ -1919,6 +1919,11 @@ where | |||
| None | |||
| } | |||
|
|
|||
| /// Returns `true` if there exists an established connection with the given peer. | |||
| pub fn has_established_connection(&self, peer: &PeerId) -> bool { | |||
There was a problem hiding this comment.
Suggested change
| pub fn has_established_connection(&self, peer: &PeerId) -> bool { | |
| pub fn has_established_connection(&self, peer_id: &PeerId) -> bool { |
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.
Fix #2295
After #2264, we now panic if starting a request on a peer we're not connected to because this is an invalid usage of the API.
The reason why this wouldn't panic before is because there was always a chance of a race condition where a connection started being closed in parallel of the request being started.
We simply do the check for connectivity in the network service, as it is now possible to do so.