Check for early message send failures

The transition to start session negotiation does not truly start if we
fail to send the hello message. Check for this condition before
manipulating any other channel state.

JIRA: NETCONF-905
Change-Id: I024a39abc8c6219019531c2abedaf124a25b3f27
Signed-off-by: Robert Varga <robert.varga@pantheon.tech>

diff --git a/netconf/netconf-client/src/test/java/org/opendaylight/netconf/client/NetconfClientSessionNegotiatorTest.java b/netconf/netconf-client/src/test/java/org/opendaylight/netconf/client/NetconfClientSessionNegotiatorTest.java
index b61073d6744c0599edde7abc2c18ed63dd7d622d..dd308eba2ea4d3269e8ac2edfdc2606fe15de516 100644 (file)
--- a/netconf/netconf-client/src/test/java/org/opendaylight/netconf/client/NetconfClientSessionNegotiatorTest.java
+++ b/netconf/netconf-client/src/test/java/org/opendaylight/netconf/client/NetconfClientSessionNegotiatorTest.java
@@ -156,6 +156,7 @@ public class NetconfClientSessionNegotiatorTest {
         NetconfClientSessionNegotiator negotiator = createNetconfClientSessionNegotiator(promise, null);
 
         negotiator.channelActive(null);
+        doReturn(null).when(future).cause();
         negotiator.handleMessage(NetconfHelloMessage.createServerHello(Set.of("a", "b"), 10));
         verify(promise).setSuccess(any());
     }
@@ -167,6 +168,7 @@ public class NetconfClientSessionNegotiatorTest {
         doReturn(promise).when(promise).setSuccess(any());
         NetconfClientSessionNegotiator negotiator = createNetconfClientSessionNegotiator(promise, null);
 
+        doReturn(null).when(future).cause();
         negotiator.handleMessage(NetconfHelloMessage.createServerHello(Set.of("a", "b"), 10));
         negotiator.channelActive(null);
         verify(promise).setSuccess(any());
@@ -179,6 +181,7 @@ public class NetconfClientSessionNegotiatorTest {
         doReturn(promise).when(promise).setSuccess(any());
         NetconfClientSessionNegotiator negotiator = createNetconfClientSessionNegotiator(promise, exiMessage);
 
+        doReturn(null).when(future).cause();
         negotiator.channelActive(null);
 
         doAnswer(invocationOnMock -> {

diff --git a/netconf/netconf-netty-util/src/main/java/org/opendaylight/netconf/nettyutil/AbstractNetconfSessionNegotiator.java b/netconf/netconf-netty-util/src/main/java/org/opendaylight/netconf/nettyutil/AbstractNetconfSessionNegotiator.java
index 88b280d1a87d366acc09a431ecc8115fbe0e8826..c72312fa79558d16110085f7e438aece60a46885 100644 (file)
--- a/netconf/netconf-netty-util/src/main/java/org/opendaylight/netconf/nettyutil/AbstractNetconfSessionNegotiator.java
+++ b/netconf/netconf-netty-util/src/main/java/org/opendaylight/netconf/nettyutil/AbstractNetconfSessionNegotiator.java
@@ -144,11 +144,19 @@ public abstract class AbstractNetconfSessionNegotiator<S extends AbstractNetconf
     }
 
     private void start() {
-        LOG.debug("Session negotiation started with hello message {} on channel {}", localHello, channel);
+        LOG.debug("Sending negotiation proposal {} on channel {}", localHello, channel);
 
         // Send the message out, but to not run listeners just yet, as we have some more state transitions to go through
         final var helloFuture = channel.writeAndFlush(localHello);
 
+        // Quick check: if the future has already failed we call it quits before negotiation even started
+        final var helloCause = helloFuture.cause();
+        if (helloCause != null) {
+            LOG.warn("Failed to send negotiation proposal on channel {}", channel, helloCause);
+            failAndClose();
+            return;
+        }
+
         channel.pipeline().addLast(NAME_OF_EXCEPTION_HANDLER, new ExceptionHandlingInboundChannelHandler());
 
         replaceHelloMessageOutboundHandler();
@@ -161,6 +169,8 @@ public abstract class AbstractNetconfSessionNegotiator<S extends AbstractNetconf
                 connectionTimeoutMillis, TimeUnit.MILLISECONDS);
         }
 
+        LOG.debug("Session negotiation started on channel {}", channel);
+
         // State transition completed, now run any additional processing
         helloFuture.addListener(this::onHelloWriteComplete);
     }
@@ -191,22 +201,27 @@ public abstract class AbstractNetconfSessionNegotiator<S extends AbstractNetconf
             if (!promise.isDone() && !promise.isCancelled()) {
                 LOG.warn("Netconf session backed by channel {} was not established after {}", channel,
                     connectionTimeoutMillis);
-                changeState(State.FAILED);
-
-                channel.close().addListener(future -> {
-                    final var cause = future.cause();
-                    if (cause != null) {
-                        LOG.warn("Channel {} closed: fail", channel, cause);
-                    } else {
-                        LOG.debug("Channel {} closed: success", channel);
-                    }
-                });
+                failAndClose();
             }
         } else if (channel.isOpen()) {
             channel.pipeline().remove(NAME_OF_EXCEPTION_HANDLER);
         }
     }
 
+    private void failAndClose() {
+        changeState(State.FAILED);
+        channel.close().addListener(this::onChannelClosed);
+    }
+
+    private void onChannelClosed(final Future<?> future) {
+        final var cause = future.cause();
+        if (cause != null) {
+            LOG.warn("Channel {} closed: fail", channel, cause);
+        } else {
+            LOG.debug("Channel {} closed: success", channel);
+        }
+    }
+
     private synchronized void cancelTimeout() {
         if (timeoutTask != null && !timeoutTask.cancel()) {
             // Late-coming cancel: make sure the task does not actually run
@@ -306,7 +321,7 @@ public abstract class AbstractNetconfSessionNegotiator<S extends AbstractNetconf
     }
 
     private static boolean isStateChangePermitted(final State state, final State newState) {
-        if (state == State.IDLE && newState == State.OPEN_WAIT) {
+        if (state == State.IDLE && (newState == State.OPEN_WAIT || newState == State.FAILED)) {
             return true;
         }
         if (state == State.OPEN_WAIT && (newState == State.ESTABLISHED || newState == State.FAILED)) {