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

Peer is_known robustness #3040

Merged
merged 3 commits into from
Sep 12, 2019

Conversation

antiochp
Copy link
Member

Our existing impl of is_known() for peers returns false if we fail to obtain the read lock on the peers map (we don't know if its known or not so we assume its not known).

This is not robust behavior in all situations.
For example - we may be checking if we already know a particular peer address when making some connections to outbound peers at startup time.
If we experience contention of the lock around the peers map we do not want to accidentally attempt to connect to a peer a second time.

This PR changes the is_known() api to return an error if we cannot get the read lock.
This allows the caller to handle this case as they see fit.

@antiochp
Copy link
Member Author

cc @j01tz

@antiochp antiochp requested review from hashmap and garyyu September 12, 2019 09:55
Copy link
Member

@quentinlesceller quentinlesceller left a comment

Choose a reason for hiding this comment

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

👍 Looking good.

p2p/src/peers.rs Outdated Show resolved Hide resolved
servers/src/grin/seed.rs Outdated Show resolved Hide resolved
Copy link
Member

@j01tz j01tz left a comment

Choose a reason for hiding this comment

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

LGTM

@antiochp antiochp merged commit 28d5ee8 into mimblewimble:master Sep 12, 2019
@antiochp antiochp deleted the peer_is_known_robustness branch September 12, 2019 20:04
antiochp added a commit to antiochp/grin that referenced this pull request Sep 17, 2019
* add some test coverage around peers map (peer_addr hashing impl)

* make is_known a bit more robust

* fix typos
antiochp added a commit to antiochp/grin that referenced this pull request Sep 19, 2019
* add some test coverage around peers map (peer_addr hashing impl)

* make is_known a bit more robust

* fix typos
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants