From 0a078764e40c24d565098ddff5ec8be01b5e79f6 Mon Sep 17 00:00:00 2001 From: =?utf8?q?Jaroslav=20T=C3=B3th?= Date: Sat, 30 Mar 2019 19:12:03 +0100 Subject: [PATCH] Fixed deadlock between AsyncSshHandlerReader and AsyncSshHandler MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit First was fix in AsyncSshHandlerReader but there was another deadlock after that. Therefore a change in AsyncSshHandler is also needed. Both ABBA deadlockw occur when AsyncSshHandler.disconnect (..) and AsyncSshHandlerReader.invokeDisconnect(..) methods are executed concurrently within the same SSH session that is going to be closed. Note: Snippets from stack-traces are from Oxygen distribution, so the line numbers don't have to match exactly. Deadlock #1: "nioEventLoopGroupCloseable-3-20": at org.opendaylight.netconf.nettyutil.handler.ssh.client .AsyncSshHandlerReader.close(AsyncSshHandlerReader.java:96) - waiting to lock <0x000000072ecffff8> (a org.opendaylight .netconf.nettyutil.handler.ssh.client.AsyncSshHandlerReader) at org.opendaylight.netconf.nettyutil.handler.ssh.client .AsyncSshHandler.disconnect(AsyncSshHandler.java:235) - locked <0x000000072ed00020> (a org.opendaylight.netconf .nettyutil.handler.ssh.client.AsyncSshHandler) at org.opendaylight.netconf.nettyutil.handler.ssh.client .AsyncSshHandler.close(AsyncSshHandler.java:215) ... "sshd-SshClient[57f78db1]-nio2-thread-1": at org.opendaylight.netconf.nettyutil.handler.ssh.client .AsyncSshHandler.disconnect(AsyncSshHandler.java:221) - waiting to lock <0x000000072ed00020> (a org.opendaylight .netconf.nettyutil.handler.ssh.client.AsyncSshHandler) ... org.opendaylight.netconf.nettyutil.handler.ssh.client .AsyncSshHandlerReader.operationComplete(AsyncSshHandlerReader .java:64) - locked <0x000000072ecffff8> (a org.opendaylight.netconf .nettyutil.handler.ssh.client.AsyncSshHandlerReader) ... Deadlock #2: "nioEventLoopGroupCloseable-3-19": at org.apache.sshd.client.session.ClientSessionImpl .signalAuthFailure(ClientSessionImpl.java:138) - waiting to lock <0x000000072c800218> (a java.lang.Object) at org.apache.sshd.client.session.ClientSessionImpl .preClose(ClientSessionImpl.java:126) at org.apache.sshd.common.util.closeable.AbstractCloseable .close(AbstractCloseable.java:95) at org.opendaylight.netconf.nettyutil.handler.ssh.client .AsyncSshHandler.disconnect(AsyncSshHandler.java:251) - locked <0x000000072c800480> (a org.opendaylight.netconf .nettyutil.handler.ssh.client.AsyncSshHandler) ... "sshd-SshClient[411a406d]-nio2-thread-8": at org.opendaylight.netconf.nettyutil.handler.ssh.client .AsyncSshHandler.disconnect(AsyncSshHandler.java:221) - waiting to lock <0x000000072c800480> (a org.opendaylight .netconf.nettyutil.handler.ssh.client.AsyncSshHandler) ... org.apache.sshd.common.session.helpers.AbstractSession .handleMessage(AbstractSession.java:499) - locked <0x000000072c800218> (a java.lang.Object) ... Change-Id: I42260213cad82250832451539d6ce11035ba37e2 Signed-off-by: Martin Sunal Signed-off-by: Jaroslav Tóth Signed-off-by: Robert Varga --- .../handler/ssh/client/AsyncSshHandler.java | 15 ++++++++---- .../ssh/client/AsyncSshHandlerReader.java | 23 ++++++++++++------- 2 files changed, 26 insertions(+), 12 deletions(-) diff --git a/netconf/netconf-netty-util/src/main/java/org/opendaylight/netconf/nettyutil/handler/ssh/client/AsyncSshHandler.java b/netconf/netconf-netty-util/src/main/java/org/opendaylight/netconf/nettyutil/handler/ssh/client/AsyncSshHandler.java index 149ccf16f9..15882ded63 100644 --- a/netconf/netconf-netty-util/src/main/java/org/opendaylight/netconf/nettyutil/handler/ssh/client/AsyncSshHandler.java +++ b/netconf/netconf-netty-util/src/main/java/org/opendaylight/netconf/nettyutil/handler/ssh/client/AsyncSshHandler.java @@ -16,6 +16,7 @@ import io.netty.util.concurrent.Future; import io.netty.util.concurrent.GenericFutureListener; import java.io.IOException; import java.net.SocketAddress; +import java.util.concurrent.atomic.AtomicBoolean; import org.apache.sshd.client.SshClient; import org.apache.sshd.client.channel.ClientChannel; import org.apache.sshd.client.future.AuthFuture; @@ -52,6 +53,7 @@ public class AsyncSshHandler extends ChannelOutboundHandlerAdapter { private final AuthenticationHandler authenticationHandler; private final SshClient sshClient; + private final AtomicBoolean isDisconnected = new AtomicBoolean(); private Future negotiationFuture; private AsyncSshHandlerReader sshReadAsyncListener; @@ -213,11 +215,17 @@ public class AsyncSshHandler extends ChannelOutboundHandlerAdapter { disconnect(ctx, promise); } - @SuppressWarnings("checkstyle:IllegalCatch") @Override - public synchronized void disconnect(final ChannelHandlerContext ctx, final ChannelPromise promise) { + public void disconnect(final ChannelHandlerContext ctx, final ChannelPromise promise) { + if (isDisconnected.compareAndSet(false, true)) { + safelyDisconnect(ctx, promise); + } + } + + @SuppressWarnings("checkstyle:IllegalCatch") + private synchronized void safelyDisconnect(final ChannelHandlerContext ctx, final ChannelPromise promise) { LOG.trace("Closing SSH session on channel: {} with connect promise in state: {}", - ctx.channel(),connectPromise); + ctx.channel(), connectPromise); // If we have already succeeded and the session was dropped after, // we need to fire inactive to notify reconnect logic @@ -272,5 +280,4 @@ public class AsyncSshHandler extends ChannelOutboundHandlerAdapter { promise.setSuccess(); LOG.debug("SSH session closed on channel: {}", ctx.channel()); } - } diff --git a/netconf/netconf-netty-util/src/main/java/org/opendaylight/netconf/nettyutil/handler/ssh/client/AsyncSshHandlerReader.java b/netconf/netconf-netty-util/src/main/java/org/opendaylight/netconf/nettyutil/handler/ssh/client/AsyncSshHandlerReader.java index b905f7ad21..da5bb74828 100644 --- a/netconf/netconf-netty-util/src/main/java/org/opendaylight/netconf/nettyutil/handler/ssh/client/AsyncSshHandlerReader.java +++ b/netconf/netconf-netty-util/src/main/java/org/opendaylight/netconf/nettyutil/handler/ssh/client/AsyncSshHandlerReader.java @@ -47,12 +47,17 @@ public final class AsyncSshHandlerReader implements SshFutureListener 0) { + return true; + } else if (future.getRead() > 0) { final ByteBuf msg = Unpooled.wrappedBuffer(buf.array(), 0, future.getRead()); if (LOG.isTraceEnabled()) { LOG.trace("Reading message on channel: {}, message: {}", @@ -78,8 +80,13 @@ public final class AsyncSshHandlerReader implements SshFutureListener