Hide Netty(Pipeline)AwareChannelSubsystem
[netconf.git] / netconf / netconf-netty-util / src / main / java / org / opendaylight / netconf / nettyutil / handler / ssh / client / AsyncSshHandler.java
index f9598210a6ec0a4772d2796958eda212baf64a4f..f6956d7c5d4e3dc39940bb96d035839c5360254b 100644 (file)
@@ -16,14 +16,20 @@ import io.netty.channel.ChannelPromise;
 import io.netty.util.concurrent.Future;
 import io.netty.util.concurrent.FutureListener;
 import java.io.IOException;
+import java.lang.invoke.MethodHandles;
+import java.lang.invoke.VarHandle;
 import java.net.SocketAddress;
+import java.time.Duration;
 import java.util.concurrent.TimeUnit;
-import java.util.concurrent.atomic.AtomicBoolean;
+import org.checkerframework.checker.lock.qual.GuardedBy;
+import org.checkerframework.checker.lock.qual.Holding;
 import org.eclipse.jdt.annotation.Nullable;
 import org.opendaylight.netconf.nettyutil.handler.ssh.authentication.AuthenticationHandler;
+import org.opendaylight.netconf.shaded.sshd.client.channel.ChannelSubsystem;
 import org.opendaylight.netconf.shaded.sshd.client.channel.ClientChannel;
 import org.opendaylight.netconf.shaded.sshd.client.future.AuthFuture;
 import org.opendaylight.netconf.shaded.sshd.client.future.ConnectFuture;
+import org.opendaylight.netconf.shaded.sshd.client.future.OpenFuture;
 import org.opendaylight.netconf.shaded.sshd.client.session.ClientSession;
 import org.opendaylight.netconf.shaded.sshd.core.CoreModuleProperties;
 import org.slf4j.Logger;
@@ -32,8 +38,17 @@ import org.slf4j.LoggerFactory;
 /**
  * Netty SSH handler class. Acts as interface between Netty and SSH library.
  */
-public class AsyncSshHandler extends ChannelOutboundHandlerAdapter {
+public final class AsyncSshHandler extends ChannelOutboundHandlerAdapter {
     private static final Logger LOG = LoggerFactory.getLogger(AsyncSshHandler.class);
+    private static final VarHandle DISCONNECTED;
+
+    static {
+        try {
+            DISCONNECTED = MethodHandles.lookup().findVarHandle(AsyncSshHandler.class, "disconnected", boolean.class);
+        } catch (NoSuchFieldException | IllegalAccessException e) {
+            throw new ExceptionInInitializerError(e);
+        }
+    }
 
     public static final String SUBSYSTEM = "netconf";
 
@@ -42,12 +57,13 @@ public class AsyncSshHandler extends ChannelOutboundHandlerAdapter {
     public static final NetconfSshClient DEFAULT_CLIENT;
 
     static {
-        final NetconfSshClient c = new NetconfClientBuilder().build();
+        final var c = new NetconfClientBuilder().build();
         // Disable default timeouts from mina sshd
-        c.getProperties().put(CoreModuleProperties.AUTH_TIMEOUT.getName(), "0");
-        c.getProperties().put(CoreModuleProperties.IDLE_TIMEOUT.getName(), "0");
-        c.getProperties().put(CoreModuleProperties.NIO2_READ_TIMEOUT.getName(), "0");
-        c.getProperties().put(CoreModuleProperties.TCP_NODELAY.getName(), true);
+        final var zero = Duration.ofMillis(0);
+        CoreModuleProperties.AUTH_TIMEOUT.set(c, zero);
+        CoreModuleProperties.IDLE_TIMEOUT.set(c, zero);
+        CoreModuleProperties.NIO2_READ_TIMEOUT.set(c, zero);
+        CoreModuleProperties.TCP_NODELAY.set(c, true);
 
         // TODO make configurable, or somehow reuse netty threadpool
         c.setNioWorkers(SSH_DEFAULT_NIO_WORKERS);
@@ -55,23 +71,33 @@ public class AsyncSshHandler extends ChannelOutboundHandlerAdapter {
         DEFAULT_CLIENT = c;
     }
 
-    private final AtomicBoolean isDisconnected = new AtomicBoolean();
     private final AuthenticationHandler authenticationHandler;
     private final Future<?> negotiationFuture;
     private final NetconfSshClient sshClient;
+    private final String name;
 
-    private AsyncSshHandlerWriter sshWriteAsyncHandler;
+    // Initialized by connect()
+    @GuardedBy("this")
+    private ChannelPromise connectPromise;
 
-    private NettyAwareChannelSubsystem channel;
+    private AsyncSshHandlerWriter sshWriteAsyncHandler;
+    private ChannelSubsystem channel;
     private ClientSession session;
-    private ChannelPromise connectPromise;
     private FutureListener<Object> negotiationFutureListener;
 
-    public AsyncSshHandler(final AuthenticationHandler authenticationHandler, final NetconfSshClient sshClient,
-            final Future<?> negotiationFuture) {
+    private volatile boolean disconnected;
+
+    private AsyncSshHandler(final AuthenticationHandler authenticationHandler, final NetconfSshClient sshClient,
+            final @Nullable Future<?> negotiationFuture, final @Nullable String name) {
         this.authenticationHandler = requireNonNull(authenticationHandler);
         this.sshClient = requireNonNull(sshClient);
         this.negotiationFuture = negotiationFuture;
+        this.name = name != null && !name.isBlank() ? name : "UNNAMED";
+    }
+
+    public AsyncSshHandler(final AuthenticationHandler authenticationHandler, final NetconfSshClient sshClient,
+            final @Nullable Future<?> negotiationFuture) {
+        this(authenticationHandler, sshClient, negotiationFuture, null);
     }
 
     /**
@@ -97,72 +123,109 @@ public class AsyncSshHandler extends ChannelOutboundHandlerAdapter {
      * @return                      {@code AsyncSshHandler}
      */
     public static AsyncSshHandler createForNetconfSubsystem(final AuthenticationHandler authenticationHandler,
-            final Future<?> negotiationFuture, final @Nullable NetconfSshClient sshClient) {
+            final Future<?> negotiationFuture, final @Nullable NetconfSshClient sshClient,
+            final @Nullable String name) {
         return new AsyncSshHandler(authenticationHandler, sshClient != null ? sshClient : DEFAULT_CLIENT,
-                negotiationFuture);
+                negotiationFuture, name);
     }
 
-    private void startSsh(final ChannelHandlerContext ctx, final SocketAddress address) throws IOException {
-        LOG.debug("Starting SSH to {} on channel: {}", address, ctx.channel());
-
-        final ConnectFuture sshConnectionFuture = sshClient.connect(authenticationHandler.getUsername(), address)
-               .verify(ctx.channel().config().getConnectTimeoutMillis(), TimeUnit.MILLISECONDS);
-        sshConnectionFuture.addListener(future -> {
-            if (future.isConnected()) {
-                handleSshSessionCreated(future, ctx);
-            } else {
-                handleSshSetupFailure(ctx, future.getException());
-            }
-        });
+    @Override
+    public synchronized void write(final ChannelHandlerContext ctx, final Object msg, final ChannelPromise promise) {
+        sshWriteAsyncHandler.write(ctx, msg, promise);
     }
 
-    private synchronized void handleSshSessionCreated(final ConnectFuture future, final ChannelHandlerContext ctx) {
-        try {
-            LOG.trace("SSH session created on channel: {}", ctx.channel());
-
-            session = future.getSession();
-            verify(session instanceof NettyAwareClientSession, "Unexpected session %s", session);
-
-            final AuthFuture authenticateFuture = authenticationHandler.authenticate(session);
-            final NettyAwareClientSession localSession = (NettyAwareClientSession) session;
-            authenticateFuture.addListener(future1 -> {
-                if (future1.isSuccess()) {
-                    handleSshAuthenticated(localSession, ctx);
-                } else {
-                    handleSshSetupFailure(ctx, new AuthenticationFailedException("Authentication failed",
-                        future1.getException()));
+    @Override
+    public synchronized void connect(final ChannelHandlerContext ctx, final SocketAddress remoteAddress,
+            final SocketAddress localAddress, final ChannelPromise promise) throws IOException {
+        LOG.debug("{}: SSH session connecting on channel {}. promise: {}", name, ctx.channel(), promise);
+        connectPromise = requireNonNull(promise);
+
+        if (negotiationFuture != null) {
+            negotiationFutureListener = future -> {
+                if (future.isSuccess()) {
+                    promise.setSuccess();
                 }
-            });
-        } catch (final IOException e) {
-            handleSshSetupFailure(ctx, e);
+            };
+            //complete connection promise with netconf negotiation future
+            negotiationFuture.addListener(negotiationFutureListener);
         }
+
+        LOG.debug("{}: Starting SSH to {} on channel: {}", name, remoteAddress, ctx.channel());
+        sshClient.connect(authenticationHandler.getUsername(), remoteAddress)
+            // FIXME: this is a blocking call, we should handle this with a concurrently-scheduled timeout. We do not
+            //        have a Timer ready, so perhaps we should be using the event loop?
+            .verify(ctx.channel().config().getConnectTimeoutMillis(), TimeUnit.MILLISECONDS)
+            .addListener(future -> onConnectComplete(future, ctx));
     }
 
-    private synchronized void handleSshAuthenticated(final NettyAwareClientSession newSession,
-            final ChannelHandlerContext ctx) {
+    private synchronized void onConnectComplete(final ConnectFuture connectFuture, final ChannelHandlerContext ctx) {
+        final var cause = connectFuture.getException();
+        if (cause != null) {
+            onOpenFailure(ctx, cause);
+            return;
+        }
+
+        final var clientSession = connectFuture.getSession();
+        LOG.trace("{}: SSH session {} created on channel: {}", name, clientSession, ctx.channel());
+        verify(clientSession instanceof NettyAwareClientSession, "Unexpected session %s", clientSession);
+
+        final var localSession = (NettyAwareClientSession) clientSession;
+        session = localSession;
+
+        final AuthFuture authFuture;
         try {
-            LOG.debug("SSH session authenticated on channel: {}, server version: {}", ctx.channel(),
-                    newSession.getServerVersion());
+            authFuture = authenticationHandler.authenticate(localSession);
+        } catch (final IOException e) {
+            onOpenFailure(ctx, e);
+            return;
+        }
 
-            channel = newSession.createSubsystemChannel(SUBSYSTEM, ctx);
-            channel.setStreaming(ClientChannel.Streaming.Async);
-            channel.open().addListener(future -> {
-                if (future.isOpened()) {
-                    handleSshChanelOpened(ctx);
-                } else {
-                    handleSshSetupFailure(ctx, future.getException());
-                }
-            });
+        authFuture.addListener(future -> onAuthComplete(future, localSession, ctx));
+    }
+
+    private synchronized void onAuthComplete(final AuthFuture authFuture, final NettyAwareClientSession clientSession,
+            final ChannelHandlerContext ctx) {
+        final var cause = authFuture.getException();
+        if (cause != null) {
+            onOpenFailure(ctx, new AuthenticationFailedException("Authentication failed", cause));
+            return;
+        }
+        if (disconnected) {
+            LOG.debug("{}: Skipping SSH subsystem allocation, channel: {}", name, ctx.channel());
+            return;
+        }
 
+        LOG.debug("{}: SSH session authenticated on channel: {}, server version: {}", name, ctx.channel(),
+            clientSession.getServerVersion());
 
+        final OpenFuture openFuture;
+        try {
+            channel = clientSession.createSubsystemChannel(SUBSYSTEM, ctx);
+            channel.setStreaming(ClientChannel.Streaming.Async);
+            openFuture = channel.open();
         } catch (final IOException e) {
-            handleSshSetupFailure(ctx, e);
+            onOpenFailure(ctx, e);
+            return;
         }
+
+        openFuture.addListener(future -> ctx.executor().execute(() -> onOpenComplete(future, ctx)));
     }
 
-    private synchronized void handleSshChanelOpened(final ChannelHandlerContext ctx) {
-        LOG.trace("SSH subsystem channel opened successfully on channel: {}", ctx.channel());
+    // This callback has to run on the channel's executor because it runs fireChannelActive(), which needs to be
+    // delivered synchronously. If we were to execute on some other thread we would end up delaying the event,
+    // potentially creating havoc in the pipeline.
+    private synchronized void onOpenComplete(final OpenFuture openFuture, final ChannelHandlerContext ctx) {
+        final var cause = openFuture.getException();
+        if (cause != null) {
+            onOpenFailure(ctx, cause);
+            return;
+        }
+        if (disconnected) {
+            LOG.trace("{}: Skipping activation, channel: {}", name, ctx.channel());
+            return;
+        }
 
+        LOG.trace("{}: SSH subsystem channel opened successfully on channel: {}", name, ctx.channel());
         if (negotiationFuture == null) {
             connectPromise.setSuccess();
         }
@@ -172,40 +235,18 @@ public class AsyncSshHandler extends ChannelOutboundHandlerAdapter {
         channel.onClose(() -> disconnect(ctx, ctx.newPromise()));
     }
 
-    private synchronized void handleSshSetupFailure(final ChannelHandlerContext ctx, final Throwable error) {
-        LOG.warn("Unable to setup SSH connection on channel: {}", ctx.channel(), error);
+    @Holding("this")
+    private void onOpenFailure(final ChannelHandlerContext ctx, final Throwable cause) {
+        LOG.warn("{}: Unable to setup SSH connection on channel: {}", name, ctx.channel(), cause);
 
         // If the promise is not yet done, we have failed with initial connect and set connectPromise to failure
         if (!connectPromise.isDone()) {
-            connectPromise.setFailure(error);
+            connectPromise.setFailure(cause);
         }
 
         disconnect(ctx, ctx.newPromise());
     }
 
-    @Override
-    public synchronized void write(final ChannelHandlerContext ctx, final Object msg, final ChannelPromise promise) {
-        sshWriteAsyncHandler.write(ctx, msg, promise);
-    }
-
-    @Override
-    public synchronized void connect(final ChannelHandlerContext ctx, final SocketAddress remoteAddress,
-                                     final SocketAddress localAddress, final ChannelPromise promise) throws Exception {
-        LOG.debug("SSH session connecting on channel {}. promise: {}", ctx.channel(), connectPromise);
-        connectPromise = promise;
-
-        if (negotiationFuture != null) {
-            negotiationFutureListener = future -> {
-                if (future.isSuccess()) {
-                    promise.setSuccess();
-                }
-            };
-            //complete connection promise with netconf negotiation future
-            negotiationFuture.addListener(negotiationFutureListener);
-        }
-        startSsh(ctx, remoteAddress);
-    }
-
     @Override
     public void close(final ChannelHandlerContext ctx, final ChannelPromise promise) {
         disconnect(ctx, promise);
@@ -213,15 +254,18 @@ public class AsyncSshHandler extends ChannelOutboundHandlerAdapter {
 
     @Override
     public void disconnect(final ChannelHandlerContext ctx, final ChannelPromise promise) {
-        if (isDisconnected.compareAndSet(false, true)) {
-            safelyDisconnect(ctx, promise);
+        if (DISCONNECTED.compareAndSet(this, false, true)) {
+            ctx.executor().execute(() -> safelyDisconnect(ctx, promise));
         }
     }
 
+    // This method has the potential to interact with the channel pipeline, for example via fireChannelInactive(). These
+    // callbacks need to complete during execution of this method and therefore this method needs to be executing on
+    // the channel's executor.
     @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);
+        LOG.trace("{}: Closing SSH session on channel: {} with connect promise in state: {}", name, ctx.channel(),
+            connectPromise);
 
         // If we have already succeeded and the session was dropped after,
         // we need to fire inactive to notify reconnect logic
@@ -265,16 +309,14 @@ public class AsyncSshHandler extends ChannelOutboundHandlerAdapter {
             // Disconnect has to be closed after inactive channel event was fired, because it interferes with it
             super.disconnect(ctx, ctx.newPromise());
         } catch (final Exception e) {
-            LOG.warn("Unable to cleanup all resources for channel: {}. Ignoring.", ctx.channel(), e);
+            LOG.warn("{}: Unable to cleanup all resources for channel: {}. Ignoring.", name, ctx.channel(), e);
         }
 
         if (channel != null) {
-            //TODO: see if calling just close() is sufficient
-            //channel.close(false);
-            channel.close();
+            channel.close(false);
             channel = null;
         }
         promise.setSuccess();
-        LOG.debug("SSH session closed on channel: {}", ctx.channel());
+        LOG.debug("{}: SSH session closed on channel: {}", name, ctx.channel());
     }
 }