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>
(cherry picked from commit 815f175ed380271ef32d04bf223f4a1b2a70bcb1)

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 7185accfb912cbfc60c0763afb19bc7eb0300f53..00d8f9ede5352e873538e60bbcfb8c6b66d20c37 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
@@ -158,6 +158,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());
     }
@@ -169,6 +170,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());
@@ -182,6 +184,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 088a33ad0e3f70605adb42230bc23bb019de23fe..54fd4146ec1fb4264f5afa613eea03e27eae47fc 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
@@ -145,11 +145,19 @@ public abstract class AbstractNetconfSessionNegotiator<P extends NetconfSessionP
 
     private void start() {
         final var localHello = sessionPreferences.getHelloMessage();
-        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();
@@ -162,6 +170,8 @@ public abstract class AbstractNetconfSessionNegotiator<P extends NetconfSessionP
                 connectionTimeoutMillis, TimeUnit.MILLISECONDS);
         }
 
+        LOG.debug("Session negotiation started on channel {}", channel);
+
         // State transition completed, now run any additional processing
         helloFuture.addListener(this::onHelloWriteComplete);
     }
@@ -193,22 +203,27 @@ public abstract class AbstractNetconfSessionNegotiator<P extends NetconfSessionP
             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
@@ -310,7 +325,7 @@ public abstract class AbstractNetconfSessionNegotiator<P extends NetconfSessionP
     }
 
     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)) {