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

Adding disconnection reason and its use for ensuring better p2p performance #1154

Closed
wants to merge 166 commits into from

Conversation

Tommo-L
Copy link
Contributor

@Tommo-L Tommo-L commented Oct 11, 2019

Closes #1133

changes:

  1. add disconnect reason
  2. send disconnection message first, then close the connection
  3. MaxConnectionReached and MaxPerAddressConnectionReached will attached with a random address list.

if (MaxConnections != -1 && ConnectedPeers.Count >= MaxConnections && !TrustedIpAddresses.Contains(remote.Address))
{
Sender.Tell(Tcp.Abort.Instance);
Sender.Tell(new Tcp.Register(ActorRefs.Nobody));
Copy link
Member

Choose a reason for hiding this comment

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

Why do you send this 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.

To create a connection an actor sends a Tcp.Connect message to the TCP Manager. Once the connection is established the connection actor sends a Tcp.Connected message to the commander, which registers the connection handler by replying with a Tcp.Register message. https://getakka.net/articles/networking/io.html#getting-started

We need to register the handler, otherwise we can't send the error message back.

Copy link
Member

Choose a reason for hiding this comment

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

Get it. But then it should be sent in LocalNode. Because the Peer doesn't send the message when disconnecting.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, we also need to send the register message in normal case#L233

private void OnTcpConnected(IPEndPoint remote, IPEndPoint local)
    ........
	ConnectedAddresses[remote.Address] = count + 1;
	IActorRef connection = Context.ActorOf(ProtocolProps(Sender, remote, local), $"connection_{Guid.NewGuid()}");
	Context.Watch(connection);
	Sender.Tell(new Tcp.Register(connection));  <------ here
	ConnectedPeers.TryAdd(connection, remote);
}

Copy link
Member

Choose a reason for hiding this comment

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

But we don't need to send Register when it is disconnecting.

Copy link
Member

Choose a reason for hiding this comment

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

The disconnection reason is sent in LocalNode.

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, we send reason message in LocalNode, and create reason in Peer, as we have abstract the TcpDisconnect. I think it's okay to put Sender.Tell(new Tcp.Register(connection)); here, which is part of the akka handshake protocol.

Copy link
Member

Choose a reason for hiding this comment

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

It's illogical that register an actor and then close it immediately.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We also want a better way, like HTTP various status codes, but akka doesn't support it, and we also want to carry the address list information.

Copy link
Member

Choose a reason for hiding this comment

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

@erikzhang, perhaps it is reaching the time to merge this PR for us to start testing it together with other daily routines.

@vncoelho
Copy link
Member

@ShawnYun, with your knowledge in the Akka I believe you could give us some hand and ideas here.

@ShawnYun
Copy link
Contributor

@vncoelho Thanks. But I don't know much about this either. So I ask the akka team, according to their answer, there are only two ways : sending the Tcp.Close message directly to close, or sending the Register and then send the reason via WriteCommand before sending Close.

@Tommo-L
Copy link
Contributor Author

Tommo-L commented Jun 5, 2020

I'll split it into two prs, one is for duplicate connection, and this pr for disconnection reason.

@Tommo-L Tommo-L closed this Aug 19, 2020
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.

Send disconnection reason when closing connection with a node