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

ignore existing peers in DialPeersAsync #2327

Merged
merged 4 commits into from
Sep 5, 2018
Merged

ignore existing peers in DialPeersAsync #2327

merged 4 commits into from
Sep 5, 2018

Conversation

melekes
Copy link
Contributor

@melekes melekes commented Sep 4, 2018

Fixes #2253

  • Updated all relevant documentation in docs
  • Updated all code comments where relevant
  • Wrote tests
  • Updated CHANGELOG_PENDING.md

@codecov-io
Copy link

codecov-io commented Sep 4, 2018

Codecov Report

Merging #2327 into develop will decrease coverage by 0.02%.
The diff coverage is 33.33%.

@@             Coverage Diff             @@
##           develop    #2327      +/-   ##
===========================================
- Coverage    61.02%   60.99%   -0.03%     
===========================================
  Files          197      197              
  Lines        16296    16302       +6     
===========================================
- Hits          9945     9944       -1     
- Misses        5490     5495       +5     
- Partials       861      863       +2
Impacted Files Coverage Δ
p2p/pex/pex_reactor.go 74% <100%> (-0.18%) ⬇️
p2p/switch.go 50.5% <27.27%> (-0.88%) ⬇️
libs/clist/clist.go 66.49% <0%> (-1.53%) ⬇️
p2p/peer.go 57.95% <0%> (-1.14%) ⬇️
blockchain/pool.go 65.73% <0%> (-0.7%) ⬇️
blockchain/reactor.go 39.44% <0%> (+0.33%) ⬆️
consensus/state.go 77.52% <0%> (+0.35%) ⬆️
consensus/reactor.go 74.22% <0%> (+0.4%) ⬆️

p2p/switch.go Outdated
if addr.Same(ourAddr) {

if addr.Same(ourAddr) || sw.HasPeerWithAddress(addr) {
sw.Logger.Debug("Ignore attempt to connect to an existing peer", "addr", addr)
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want a log message if you try to connect to yourself? If so, would that warrant its own message?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not a biggie since it's Debug

Copy link
Contributor

@ValarDragon ValarDragon left a comment

Choose a reason for hiding this comment

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

LGTM, just a minor comment about log messages

@melekes
Copy link
Contributor Author

melekes commented Sep 5, 2018

Refactored code a little bit. PTAL

@ebuchman ebuchman merged commit eb5cf0f into develop Sep 5, 2018
@ebuchman ebuchman deleted the 2253-dial-peers branch September 5, 2018 15:52
if addr.Same(ourAddr) {
sw.Logger.Debug("Ignore attempt to connect to ourselves", "addr", addr, "ourAddr", ourAddr)
return
} else if sw.IsDialingOrExistingAddress(addr) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This will help, but afaict it won't fix the issue in every case because dials can still be concurrent.

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