From: Jaroslav Tóth <jtoth@frinx.io>
Date: Sat, 30 Mar 2019 18:12:03 +0000 (+0100)
Subject: Fixed deadlock between AsyncSshHandlerReader and AsyncSshHandler
X-Git-Tag: release/neon-sr1~1^2
X-Git-Url: https://git.opendaylight.org/gerrit/gitweb?p=netconf.git;a=commitdiff_plain;h=0a078764e40c24d565098ddff5ec8be01b5e79f6

Fixed deadlock between AsyncSshHandlerReader and AsyncSshHandler

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 <msunal@frinx.io>
Signed-off-by: Jaroslav Tóth <jtoth@frinx.io>
Signed-off-by: Robert Varga <robert.varga@pantheon.tech>
---

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<IoReadFutu
     }
 
     @Override
-    public synchronized void operationComplete(final IoReadFuture future) {
-        if (future.getException() != null) {
+    public void operationComplete(final IoReadFuture future) {
+        if (checkDisconnect(future)) {
+            invokeDisconnect();
+        }
+    }
 
+    private synchronized boolean checkDisconnect(final IoReadFuture future) {
+        if (future.getException() != null) {
             //if asyncout is already set to null by close method, do nothing
             if (asyncOut == null) {
-                return;
+                return false;
             }
 
             if (asyncOut.isClosed() || asyncOut.isClosing()) {
@@ -61,11 +66,8 @@ public final class AsyncSshHandlerReader implements SshFutureListener<IoReadFutu
             } else {
                 LOG.warn("Exception while reading from SSH remote on channel {}", channelId, future.getException());
             }
-            invokeDisconnect();
-            return;
-        }
-
-        if (future.getRead() > 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<IoReadFutu
             currentReadFuture = asyncOut.read(buf);
             currentReadFuture.addListener(this);
         }
+        return false;
     }
 
+    /**
+     * Closing of the {@link AsyncSshHandlerReader}. This method should never be called with any locks held since
+     * call to {@link AutoCloseable#close()} can be a source of ABBA deadlock.
+     */
     @SuppressWarnings("checkstyle:IllegalCatch")
     private void invokeDisconnect() {
         try {