From 5340b4b16c2332db9b474dfd1a345070d0829b8a Mon Sep 17 00:00:00 2001 From: Maros Marsalek Date: Fri, 5 Dec 2014 14:28:31 +0100 Subject: [PATCH] Fix channelInactive event handling in the netty pipeline for netconf. Reoreding events from AsyncSshHandler. + Moving ClosedChannelHandler from Reconnect promise at the end of the pipeline. + Add channel id to toString() of abstract netconf session Change-Id: I9884c2b8d8b2d89878e2fe5cb43fda7a98be5b23 Signed-off-by: Maros Marsalek --- .../framework/AbstractProtocolSession.java | 6 ++++ .../protocol/framework/ReconnectPromise.java | 15 ++++----- .../NetconfMonitoringServiceImplTest.java | 1 + .../operations/DefaultStopExiTest.java | 1 + .../nettyutil/AbstractNetconfSession.java | 1 + .../handler/ssh/client/AsyncSshHandler.java | 31 ++++++++++--------- .../nettyutil/AbstractNetconfSessionTest.java | 1 + 7 files changed, 33 insertions(+), 23 deletions(-) diff --git a/opendaylight/commons/protocol-framework/src/main/java/org/opendaylight/protocol/framework/AbstractProtocolSession.java b/opendaylight/commons/protocol-framework/src/main/java/org/opendaylight/protocol/framework/AbstractProtocolSession.java index 47e96d1ff4..af196a941a 100644 --- a/opendaylight/commons/protocol-framework/src/main/java/org/opendaylight/protocol/framework/AbstractProtocolSession.java +++ b/opendaylight/commons/protocol-framework/src/main/java/org/opendaylight/protocol/framework/AbstractProtocolSession.java @@ -37,6 +37,12 @@ public abstract class AbstractProtocolSession extends SimpleChannelInboundHan public final void channelInactive(final ChannelHandlerContext ctx) { LOG.debug("Channel {} inactive.", ctx.channel()); endOfInput(); + try { + // Forward channel inactive event, all handlers in pipeline might be interested in the event e.g. close channel handler of reconnect promise + super.channelInactive(ctx); + } catch (final Exception e) { + throw new RuntimeException("Failed to delegate channel inactive event on channel " + ctx.channel(), e); + } } @Override diff --git a/opendaylight/commons/protocol-framework/src/main/java/org/opendaylight/protocol/framework/ReconnectPromise.java b/opendaylight/commons/protocol-framework/src/main/java/org/opendaylight/protocol/framework/ReconnectPromise.java index b2ab27a826..aaec95a74b 100644 --- a/opendaylight/commons/protocol-framework/src/main/java/org/opendaylight/protocol/framework/ReconnectPromise.java +++ b/opendaylight/commons/protocol-framework/src/main/java/org/opendaylight/protocol/framework/ReconnectPromise.java @@ -47,13 +47,12 @@ final class ReconnectPromise, L extends SessionList pending = this.dispatcher.createClient(this.address, cs, b, new AbstractDispatcher.PipelineInitializer() { @Override public void initializeChannel(final SocketChannel channel, final Promise promise) { - // add closed channel handler - // This handler has to be added before initializer.initializeChannel is called - // Initializer might add some handlers using addFirst e.g. AsyncSshHandler and in that case - // closed channel handler is before the handler that invokes channel inactive event - channel.pipeline().addFirst(new ClosedChannelHandler(ReconnectPromise.this)); - initializer.initializeChannel(channel, promise); + // add closed channel handler + // This handler has to be added as last channel handler and the channel inactive event has to be caught by it + // Handlers in front of it can react to channelInactive event, but have to forward the event or the reconnect will not work + // This handler is last so all handlers in front of it can handle channel inactive (to e.g. resource cleanup) before a new connection is started + channel.pipeline().addLast(new ClosedChannelHandler(ReconnectPromise.this)); } }); } @@ -91,9 +90,7 @@ final class ReconnectPromise, L extends SessionList @Override public void channelInactive(final ChannelHandlerContext ctx) throws Exception { - // Pass info about disconnect further and then reconnect - super.channelInactive(ctx); - + // This is the ultimate channel inactive handler, not forwarding if (promise.isCancelled()) { return; } diff --git a/opendaylight/netconf/netconf-impl/src/test/java/org/opendaylight/controller/netconf/impl/NetconfMonitoringServiceImplTest.java b/opendaylight/netconf/netconf-impl/src/test/java/org/opendaylight/controller/netconf/impl/NetconfMonitoringServiceImplTest.java index 395e5c0338..0d296c5f52 100644 --- a/opendaylight/netconf/netconf-impl/src/test/java/org/opendaylight/controller/netconf/impl/NetconfMonitoringServiceImplTest.java +++ b/opendaylight/netconf/netconf-impl/src/test/java/org/opendaylight/controller/netconf/impl/NetconfMonitoringServiceImplTest.java @@ -98,6 +98,7 @@ public class NetconfMonitoringServiceImplTest { NetconfServerSessionListener sessionListener = mock(NetconfServerSessionListener.class); Channel channel = mock(Channel.class); + doReturn("mockChannel").when(channel).toString(); NetconfHelloMessageAdditionalHeader header = new NetconfHelloMessageAdditionalHeader("name", "addr", "2", "tcp", "id"); NetconfServerSession sm = new NetconfServerSession(sessionListener, channel, 10, header); doNothing().when(sessionListener).onSessionUp(any(NetconfServerSession.class)); diff --git a/opendaylight/netconf/netconf-impl/src/test/java/org/opendaylight/controller/netconf/impl/mapping/operations/DefaultStopExiTest.java b/opendaylight/netconf/netconf-impl/src/test/java/org/opendaylight/controller/netconf/impl/mapping/operations/DefaultStopExiTest.java index c06e78aa99..aaaf5991d4 100644 --- a/opendaylight/netconf/netconf-impl/src/test/java/org/opendaylight/controller/netconf/impl/mapping/operations/DefaultStopExiTest.java +++ b/opendaylight/netconf/netconf-impl/src/test/java/org/opendaylight/controller/netconf/impl/mapping/operations/DefaultStopExiTest.java @@ -31,6 +31,7 @@ public class DefaultStopExiTest { DefaultStopExi exi = new DefaultStopExi(""); Document doc = XmlUtil.newDocument(); Channel channel = mock(Channel.class); + doReturn("mockChannel").when(channel).toString(); ChannelPipeline pipeline = mock(ChannelPipeline.class); doReturn(pipeline).when(channel).pipeline(); ChannelHandler channelHandler = mock(ChannelHandler.class); diff --git a/opendaylight/netconf/netconf-netty-util/src/main/java/org/opendaylight/controller/netconf/nettyutil/AbstractNetconfSession.java b/opendaylight/netconf/netconf-netty-util/src/main/java/org/opendaylight/controller/netconf/nettyutil/AbstractNetconfSession.java index efa1c731c8..fd11ce8c51 100644 --- a/opendaylight/netconf/netconf-netty-util/src/main/java/org/opendaylight/controller/netconf/nettyutil/AbstractNetconfSession.java +++ b/opendaylight/netconf/netconf-netty-util/src/main/java/org/opendaylight/controller/netconf/nettyutil/AbstractNetconfSession.java @@ -87,6 +87,7 @@ public abstract class AbstractNetconfSession() { @Override @@ -216,13 +215,17 @@ public class AsyncSshHandler extends ChannelOutboundHandlerAdapter { }); } - // If we have already succeeded and the session was dropped after, we need to fire inactive to notify reconnect logic - if(connectPromise.isSuccess()) { - ctx.fireChannelInactive(); + // Super disconnect is necessary in this case since we are using NioSocketChannel and it needs to cleanup its resources + // e.g. Socket that it tries to open in its constructor (https://bugs.opendaylight.org/show_bug.cgi?id=2430) + // TODO better solution would be to implement custom ChannelFactory + Channel that will use mina SSH lib internally: port this to custom channel implementation + try { + // 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); } channel = null; - promise.setSuccess(); LOG.debug("SSH session closed on channel: {}", ctx.channel()); } diff --git a/opendaylight/netconf/netconf-netty-util/src/test/java/org/opendaylight/controller/netconf/nettyutil/AbstractNetconfSessionTest.java b/opendaylight/netconf/netconf-netty-util/src/test/java/org/opendaylight/controller/netconf/nettyutil/AbstractNetconfSessionTest.java index 8199963c81..7946afdbf5 100644 --- a/opendaylight/netconf/netconf-netty-util/src/test/java/org/opendaylight/controller/netconf/nettyutil/AbstractNetconfSessionTest.java +++ b/opendaylight/netconf/netconf-netty-util/src/test/java/org/opendaylight/controller/netconf/nettyutil/AbstractNetconfSessionTest.java @@ -60,6 +60,7 @@ public class AbstractNetconfSessionTest { doReturn(mock(ChannelFuture.class)).when(channel).writeAndFlush(any(NetconfMessage.class)); doReturn(pipeline).when(channel).pipeline(); + doReturn("mockChannel").when(channel).toString(); doReturn(mock(ChannelFuture.class)).when(channel).close(); doReturn(null).when(pipeline).replace(anyString(), anyString(), any(ChannelHandler.class)); -- 2.36.6