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

do not panic if the peer does not have a state in Receive #3346

Closed
wants to merge 1 commit into from

Conversation

melekes
Copy link
Contributor

@melekes melekes commented Feb 22, 2019

Peer does not have a state yet. We set it in AddPeer, but because the
peer is started before we add it to reactors, we can receive a message
before AddPeer is called.

Refs #3304

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

Peer does not have a state yet. We set it in AddPeer, but because the
peer is started before we add it to reactors, we can receive a message
before AddPeer is called.

Refs #3304
@melekes melekes requested a review from ebuchman as a code owner February 22, 2019 09:29
@codecov-io
Copy link

Codecov Report

Merging #3346 into develop will decrease coverage by 0.02%.
The diff coverage is 0%.

@@             Coverage Diff             @@
##           develop    #3346      +/-   ##
===========================================
- Coverage    64.03%   64.01%   -0.03%     
===========================================
  Files          215      215              
  Lines        17738    17725      -13     
===========================================
- Hits         11358    11346      -12     
+ Misses        5452     5445       -7     
- Partials       928      934       +6
Impacted Files Coverage Δ
consensus/reactor.go 71.78% <0%> (+0.47%) ⬆️
privval/client.go 75.55% <0%> (-10%) ⬇️
libs/db/remotedb/remotedb.go 36.84% <0%> (-4.69%) ⬇️
privval/socket.go 92% <0%> (-4%) ⬇️
privval/remote_signer.go 81.51% <0%> (-1.69%) ⬇️
mempool/reactor.go 68.35% <0%> (-1.65%) ⬇️
blockchain/pool.go 78.83% <0%> (-0.69%) ⬇️
p2p/pex/pex_reactor.go 79.24% <0%> (-0.32%) ⬇️
libs/events/event_cache.go 100% <0%> (ø) ⬆️
... and 2 more

panic(fmt.Sprintf("Peer %v has no state", src))
// Peer does not have a state yet. We set it in AddPeer, but because the
// peer is started before we add it to reactors, we can receive a message
// before AddPeer is called.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it difficult to write a test that exposes this behaviour?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes

@unclezoro
Copy link
Contributor

seems we try to fix same issue #3361

@ebuchman
Copy link
Contributor

ebuchman commented Mar 2, 2019

Ok let's close this for #3361 (comment) and figure out how to fix from there

@ebuchman ebuchman closed this Mar 2, 2019
@ebuchman ebuchman deleted the 3304-peer-has-no-state branch March 2, 2019 20:38
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.

5 participants