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

Simplify connection closing and cleanups in TcpTransport #25250

Merged
merged 8 commits into from Jun 19, 2017

Conversation

Projects
None yet
4 participants
@s1monw
Copy link
Contributor

commented Jun 15, 2017

Today we maintain a map of open connections in order to close them when
a low level channel gets closed or handles a failure. We also spawn a thread due to some
tricky concurrency issues especially with respect to netty since they listener might
be called on a transport / boss thread. Executions on those threads must not be blocking
since otherwise we will likely deadlock the event processing which adds to the
complexity of the concurrency model in this class.

This change associates the connection with the close callback that every channel invokes
once it's closed which allows us to remove the connections map. A relaxed non-blocking
concurrency model in the connection close listener allows cleaning up connected nodes without
blocking on any lock.

Simplify connection closing and cleanups in TcpTransport
Today we maintain a map of open connections in order to close them when
a low level channel gets closed or handles a failure. We also spawn a thread due to some
tricky concurrency issues especially with respect to netty since they listener might
be called on a transport / boss thread. Executions on those threads must not be blocking
since otherwise we will likely deadlock the event processing which adds to the
complexity of the concurrency model in this class.

This change assocaites the connection with the close callback that every channel invokes
once it's closed which allows us to remove the connections map. A relaxed non-blocking
concurrency model in the connection close listener allows cleaning up connected nodes without
blocking on any lock.
@bleskes
Copy link
Member

left a comment

This is great. Thanks for pursuing it further

@@ -170,7 +169,7 @@

// this lock is here to make sure we close this transport and disconnect all the client nodes
// connections while no connect operations is going on... (this might help with 100% CPU when stopping the transport?)
protected final ReadWriteLock globalLock = new ReentrantReadWriteLock();
protected final ReadWriteLock closeLock = new ReentrantReadWriteLock();

This comment has been minimized.

Copy link
@bleskes

bleskes Jun 16, 2017

Member

++ this confused me too before.

@@ -465,6 +459,20 @@ public void connectToNode(DiscoveryNode node, ConnectionProfile connectionProfil
logger.debug("connected to node [{}]", node);
}
transportServiceAdapter.onNodeConnected(node);

This comment has been minimized.

Copy link
@bleskes

bleskes Jun 16, 2017

Member

paranoia: if we hit an exception here, we can leave the nodeChannels in the connectedNodes map. Should we move this:

  if (connectedNodes.remove(node, nodeChannels)) {   
      transportServiceAdapter.onNodeDisconnected(node);
  }

to the finally clause (under the check of success)?

s1monw added some commits Jun 16, 2017

@tbrooks8
Copy link
Contributor

left a comment

Just a few small things

@@ -953,7 +940,7 @@ protected void onException(Channel channel, Exception e) {
String reason = ExceptionsHelper.detailedMessage(e);

This comment has been minimized.

Copy link
@tbrooks8

tbrooks8 Jun 16, 2017

Contributor

Unused?


private void closeAndNotify(DiscoveryNode node, NodeChannels nodeChannels, String reason) {
private void disconnectFromNodeCloseAndNotify(DiscoveryNode node, NodeChannels nodeChannels) {
assert nodeChannels != null : "nodeChannels must note be null";

This comment has been minimized.

Copy link
@tbrooks8

tbrooks8 Jun 16, 2017

Contributor

Nit: spelling

s1monw added some commits Jun 17, 2017

@s1monw s1monw merged commit dc02b32 into elastic:master Jun 19, 2017

2 checks passed

CLA Commit author is a member of Elasticsearch
Details
elasticsearch-ci Build finished.
Details

@s1monw s1monw deleted the s1monw:simmplify_transport branch Jun 19, 2017

s1monw added a commit that referenced this pull request Jun 19, 2017

Simplify connection closing and cleanups in TcpTransport (#25250)
Today we maintain a map of open connections in order to close them when
a low level channel gets closed or handles a failure. We also spawn a thread due to some
tricky concurrency issues especially with respect to netty since they listener might
be called on a transport / boss thread. Executions on those threads must not be blocking
since otherwise we will likely deadlock the event processing which adds to the
complexity of the concurrency model in this class.

This change associates the connection with the close callback that every channel invokes
once it's closed which allows us to remove the connections map. A relaxed non-blocking
concurrency model in the connection close listener allows cleaning up connected nodes without
blocking on any lock.

@clintongormley clintongormley added v6.0.0-beta1 and removed v6.0.0 labels Jul 25, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.