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

p2p/conn: FlushStop. Use in pex. Closes #2092 #2802

Merged
merged 7 commits into from Nov 16, 2018
Merged

Conversation

ebuchman
Copy link
Contributor

@ebuchman ebuchman commented Nov 11, 2018

In seed mode, we call StopPeer immediately after Send.
Since flushing msgs to the peer happens in the background,
the peer connection is often closed before the messages are
actually sent out. With FlushStop, we allow all msgs to first be written and flushed out
on the conn before it is closed.

This should be a proper solve for this issue, to replace https://github.com/tendermint/tendermint/pull/2797/files#diff-8c6b1a350a161ced01d68514ccda7484R224

Closes #2092

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

In seed mode, we call StopPeer immediately after Send.
Since flushing msgs to the peer happens in the background,
the peer connection is often closed before the messages are
actually sent out. The new FlushStop method allows all msgs
to first be written and flushed out on the conn before it is closed.
p2p/pex/pex_reactor_test.go Outdated Show resolved Hide resolved
p2p/pex/pex_reactor.go Outdated Show resolved Hide resolved
p2p/conn/connection.go Show resolved Hide resolved
@codecov-io
Copy link

codecov-io commented Nov 11, 2018

Codecov Report

Merging #2802 into develop will increase coverage by 0.02%.
The diff coverage is 63.88%.

@@            Coverage Diff             @@
##           develop   #2802      +/-   ##
==========================================
+ Coverage    62.28%   62.3%   +0.02%     
==========================================
  Files          212     212              
  Lines        17182   17253      +71     
==========================================
+ Hits         10702   10750      +48     
- Misses        5582    5603      +21     
- Partials       898     900       +2
Impacted Files Coverage Δ
p2p/peer.go 58.78% <0%> (-1.64%) ⬇️
p2p/pex/pex_reactor.go 71.51% <30%> (-1.8%) ⬇️
p2p/conn/connection.go 79.89% <90.9%> (+0.45%) ⬆️
p2p/pex/addrbook.go 68.91% <0%> (-0.49%) ⬇️
consensus/reactor.go 66.31% <0%> (+0.23%) ⬆️
p2p/node_info.go 89.39% <0%> (+0.33%) ⬆️
privval/remote_signer.go 82.85% <0%> (+1.42%) ⬆️
privval/ipc_server.go 69.81% <0%> (+1.88%) ⬆️
privval/tcp_server.go 82.85% <0%> (+4.28%) ⬆️
... and 2 more

p2p/conn/connection.go Outdated Show resolved Hide resolved
p2p/conn/connection.go Outdated Show resolved Hide resolved
p2p/conn/connection.go Show resolved Hide resolved
p2p/conn/connection.go Outdated Show resolved Hide resolved
@ebuchman
Copy link
Contributor Author

@melekes @jaekwon addressed all feedback - PTAL.

I had to add some extra logic to ensure FlushStop is not called more than once.

Also realized the seed node was disconnecting from all peers as soon as it received a pex-request, rather than just inbound peers.

FlushStop is not safe to call more than once,
but we call it from Receive in a go-routine so Receive
doesn't block.

To ensure we only call it once, we use the lastReceivedRequests
map - if an entry already exists, then FlushStop should already have
been called and we can return.
Copy link
Contributor

@melekes melekes left a comment

Choose a reason for hiding this comment

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

🍃 🌴 🌤

@ebuchman ebuchman merged commit 0d5e0d2 into develop Nov 16, 2018
@ebuchman ebuchman deleted the bucky/2092-flush-stop branch November 16, 2018 22:44
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.

None yet

4 participants