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

Remove error_channel and simplify how we close peer connections #2796

Merged
merged 1 commit into from
May 3, 2019

Conversation

antiochp
Copy link
Member

@antiochp antiochp commented May 2, 2019

Replaces #2794 with a simpler solution.

Existing behavior -

  • try_break! on certain errors will send an error event to the error_channel
  • peers.check_connection tries to read off the error_channel
    • on receiving an error event it will then put a close event on the close_channel
  • the peer connection poll loop then tries to read off the close_channel
    • on receiving a close event it closes the connection

So we have -

error -> error_channel -> close_channel -> close

In addition try_break! also breaks out of the connection poll loop and immediately closes the connection.
So none of the above actually affects anything. We have already broken out of the poll loop and we will never read from the peer specific close_channel.

This PR simplifies all of the above by replacing the "put an error event on the error_channel" with a simple "stop (close connection) and remove peer".

There is no need for the error_channel as we can close peer connection directly without this intermediate layer of indirection.

There are 3 places where we actually use these connections -

  1. When we receive a msg from a peer
  2. When we broadcast a msg out to peer(s)
  3. When we ping all our peers (similar but slightly different to above)

For (1) we want to close the connection if try_break! encounters a serious enough peer related error.
We already handle this by breaking out of the poll loop (error_channel never actually used).
For (2) and (3) we now simply close() and remove the peer from our list of peers.

Previously we were relying on the complex is_connected() logic to identify a closed or error condition peer connection (and for it to internally clean up the peer and connection).

We now simply attempt to send the msg and on error close the connection (for both ping and broadcast paths).

Related - #2801 removes option wrapper on the peer connection which will make peer.is_connected() here even simpler as we no longer need to handle the case where a peer has no initialized connection.

p2p/src/peer.rs Show resolved Hide resolved
p2p/src/peers.rs Show resolved Hide resolved
@antiochp antiochp marked this pull request as ready for review May 3, 2019 11:31
@antiochp antiochp merged commit 6c54c90 into mimblewimble:master May 3, 2019
@antiochp antiochp deleted the cleanup_error_channel branch May 3, 2019 14:35
@antiochp antiochp added this to the 1.1.0 milestone Jun 5, 2019
@antiochp antiochp added the release notes To be included in release notes (of relevant milestone). label Jun 5, 2019
@antiochp antiochp changed the title remove the error_channel and simplify how we close peer connections Remove error_channel and simplify how we close peer connections Jun 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release notes To be included in release notes (of relevant milestone).
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants