Bug 5558 - Closing session after a large RIB is sent kills BGPCEP
[bgpcep.git] / bgp / rib-impl / src / main / java / org / opendaylight / protocol / bgp / rib / impl / AbstractBGPSessionNegotiator.java
index ba3085462b91549e311ec67573af5dd2a6e95a4a..4f00c9af2ef3b2747af82e77eae545884998e540 100644 (file)
@@ -11,6 +11,10 @@ package org.opendaylight.protocol.bgp.rib.impl;
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Preconditions;
 import io.netty.channel.Channel;
+import io.netty.channel.ChannelFuture;
+import io.netty.channel.ChannelFutureListener;
+import io.netty.channel.ChannelHandlerContext;
+import io.netty.channel.ChannelInboundHandlerAdapter;
 import io.netty.util.concurrent.Promise;
 import java.util.concurrent.TimeUnit;
 import javax.annotation.concurrent.GuardedBy;
@@ -18,9 +22,8 @@ import org.opendaylight.protocol.bgp.parser.BGPDocumentedException;
 import org.opendaylight.protocol.bgp.parser.BGPError;
 import org.opendaylight.protocol.bgp.rib.impl.spi.BGPPeerRegistry;
 import org.opendaylight.protocol.bgp.rib.impl.spi.BGPSessionPreferences;
-import org.opendaylight.protocol.bgp.rib.impl.spi.BGPSessionValidator;
 import org.opendaylight.protocol.bgp.rib.spi.BGPSessionListener;
-import org.opendaylight.protocol.framework.AbstractSessionNegotiator;
+import org.opendaylight.protocol.bgp.rib.spi.SessionNegotiator;
 import org.opendaylight.protocol.util.Values;
 import org.opendaylight.yang.gen.v1.urn.ietf.params.xml.ns.yang.ietf.inet.types.rev100924.IpAddress;
 import org.opendaylight.yang.gen.v1.urn.ietf.params.xml.ns.yang.ietf.inet.types.rev100924.Ipv4Address;
@@ -35,12 +38,12 @@ import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
 /**
- * Bgp Session negotiator. Common for local -> remote and remote -> local connections.
+ * Bgp Session negotiator. Common for local-to-remote and remote-to-local connections.
  * One difference is session validation performed by injected BGPSessionValidator when OPEN message is received.
  */
-public abstract class AbstractBGPSessionNegotiator extends AbstractSessionNegotiator<Notification, BGPSessionImpl> {
+public abstract class AbstractBGPSessionNegotiator extends ChannelInboundHandlerAdapter implements SessionNegotiator {
     // 4 minutes recommended in http://tools.ietf.org/html/rfc4271#section-8.2.2
-    protected static final int INITIAL_HOLDTIMER = 4;
+    private static final int INITIAL_HOLDTIMER = 4;
 
     /**
      * @see <a href="http://tools.ietf.org/html/rfc6793">BGP Support for 4-Octet AS Number Space</a>
@@ -70,8 +73,8 @@ public abstract class AbstractBGPSessionNegotiator extends AbstractSessionNegoti
 
     private static final Logger LOG = LoggerFactory.getLogger(AbstractBGPSessionNegotiator.class);
     private final BGPPeerRegistry registry;
-    private final BGPSessionValidator sessionValidator;
-
+    private final Promise<BGPSessionImpl> promise;
+    private final Channel channel;
     @GuardedBy("this")
     private State state = State.IDLE;
 
@@ -79,34 +82,33 @@ public abstract class AbstractBGPSessionNegotiator extends AbstractSessionNegoti
     private BGPSessionImpl session;
 
     public AbstractBGPSessionNegotiator(final Promise<BGPSessionImpl> promise, final Channel channel,
-            final BGPPeerRegistry registry, final BGPSessionValidator sessionValidator) {
-        super(promise, channel);
+            final BGPPeerRegistry registry) {
+        this.promise = Preconditions.checkNotNull(promise);
+        this.channel = Preconditions.checkNotNull(channel);
         this.registry = registry;
-        this.sessionValidator = sessionValidator;
     }
 
-    @Override
-    protected synchronized void startNegotiation() {
-        Preconditions.checkState(this.state == State.IDLE);
+    private synchronized void startNegotiation() {
+        // Open can be sent first either from ODL (IDLE) or from peer (OPEN_CONFIRM)
+        Preconditions.checkState(this.state == State.IDLE || this.state == State.OPEN_CONFIRM);
+        final IpAddress remoteIp = getRemoteIp();
 
         // Check if peer is configured in registry before retrieving preferences
-        if (!this.registry.isPeerConfigured(getRemoteIp())) {
+        if (!this.registry.isPeerConfigured(remoteIp)) {
             final BGPDocumentedException cause = new BGPDocumentedException(
-                    "BGP peer with ip: " + getRemoteIp()
-                    + " not configured, check configured peers in : "
-                    + this.registry, BGPError.CONNECTION_REJECTED);
+                String.format("BGP peer with ip: %s not configured, check configured peers in : %s", remoteIp, this.registry), BGPError.CONNECTION_REJECTED);
             negotiationFailed(cause);
             return;
         }
 
-        final BGPSessionPreferences preferences = getPreferences();
+        final BGPSessionPreferences preferences = this.registry.getPeerPreferences(remoteIp);
 
         int as = preferences.getMyAs().getValue().intValue();
         // Set as AS_TRANS if the value is bigger than 2B
         if (as > Values.UNSIGNED_SHORT_MAX_VALUE) {
             as = AS_TRANS;
         }
-        this.sendMessage(new OpenBuilder().setMyAsNumber(as).setHoldTimer(preferences.getHoldTime()).setBgpIdentifier(
+        sendMessage(new OpenBuilder().setMyAsNumber(as).setHoldTimer(preferences.getHoldTime()).setBgpIdentifier(
                 preferences.getBgpId()).setBgpParameters(preferences.getParams()).build());
         if (this.state != State.FINISHED) {
             this.state = State.OPEN_SENT;
@@ -124,22 +126,24 @@ public abstract class AbstractBGPSessionNegotiator extends AbstractSessionNegoti
         }
     }
 
-    private BGPSessionPreferences getPreferences() {
-        return this.registry.getPeerPreferences(getRemoteIp());
-    }
-
     private IpAddress getRemoteIp() {
         return StrictBGPPeerRegistry.getIpAddress(this.channel.remoteAddress());
     }
 
-    @Override
     protected synchronized void handleMessage(final Notification msg) {
         LOG.debug("Channel {} handling message in state {}", this.channel, this.state);
 
         switch (this.state) {
         case FINISHED:
+            sendMessage(buildErrorNotify(BGPError.FSM_ERROR));
+            return;
         case IDLE:
-            this.sendMessage(buildErrorNotify(BGPError.FSM_ERROR));
+            // to avoid race condition when Open message was sent by the peer before startNegotiation could be executed
+            if (msg instanceof Open) {
+                handleOpen((Open) msg);
+                return;
+            }
+            sendMessage(buildErrorNotify(BGPError.FSM_ERROR));
             return;
         case OPEN_CONFIRM:
             if (msg instanceof Keepalive) {
@@ -153,8 +157,7 @@ public abstract class AbstractBGPSessionNegotiator extends AbstractSessionNegoti
             return;
         case OPEN_SENT:
             if (msg instanceof Open) {
-                final Open openObj = (Open) msg;
-                handleOpen(openObj);
+                handleOpen((Open) msg);
                 return;
             }
             break;
@@ -164,7 +167,7 @@ public abstract class AbstractBGPSessionNegotiator extends AbstractSessionNegoti
 
         // Catch-all for unexpected message
         LOG.warn("Channel {} state {} unexpected message {}", this.channel, this.state, msg);
-        this.sendMessage(buildErrorNotify(BGPError.FSM_ERROR));
+        sendMessage(buildErrorNotify(BGPError.FSM_ERROR));
         negotiationFailed(new BGPDocumentedException("Unexpected message", BGPError.FSM_ERROR));
         this.state = State.FINISHED;
     }
@@ -182,17 +185,12 @@ public abstract class AbstractBGPSessionNegotiator extends AbstractSessionNegoti
     }
 
     private void handleOpen(final Open openObj) {
+        final IpAddress remoteIp = getRemoteIp();
+        final BGPSessionPreferences preferences = this.registry.getPeerPreferences(remoteIp);
         try {
-            this.sessionValidator.validate(openObj, getPreferences());
-        } catch (final BGPDocumentedException e) {
-            negotiationFailed(e);
-            return;
-        }
-
-        try {
-            final BGPSessionListener peer = this.registry.getPeer(getRemoteIp(), getSourceId(openObj, getPreferences()), getDestinationId(openObj, getPreferences()));
-            this.sendMessage(new KeepaliveBuilder().build());
-            this.session = new BGPSessionImpl(peer, this.channel, openObj, getPreferences());
+            final BGPSessionListener peer = this.registry.getPeer(remoteIp, getSourceId(openObj, preferences), getDestinationId(openObj, preferences), openObj);
+            sendMessage(new KeepaliveBuilder().build());
+            this.session = new BGPSessionImpl(peer, this.channel, openObj, preferences, this.registry);
             this.state = State.OPEN_CONFIRM;
             LOG.debug("Channel {} moved to OpenConfirm state with remote proposal {}", this.channel, openObj);
         } catch (final BGPDocumentedException e) {
@@ -201,30 +199,94 @@ public abstract class AbstractBGPSessionNegotiator extends AbstractSessionNegoti
         }
     }
 
-    @Override
-    protected void negotiationFailed(final Throwable e) {
+    private void negotiationFailed(final Throwable e) {
         LOG.warn("Channel {} negotiation failed: {}", this.channel, e.getMessage());
         if (e instanceof BGPDocumentedException) {
             // although sendMessage() can also result in calling this method, it won't create a cycle. In case sendMessage() fails to
             // deliver the message, this method gets called with different exception (definitely not with BGPDocumentedException).
-            this.sendMessage(buildErrorNotify(((BGPDocumentedException)e).getError(), ((BGPDocumentedException) e).getData()));
+            sendMessage(buildErrorNotify(((BGPDocumentedException)e).getError(), ((BGPDocumentedException) e).getData()));
+        }
+        if (this.state == State.OPEN_CONFIRM) {
+            this.registry.removePeerSession(getRemoteIp());
         }
-        this.registry.removePeerSession(getRemoteIp());
-        super.negotiationFailed(e);
+        negotiationFailedCloseChannel(e);
         this.state = State.FINISHED;
     }
 
     /**
+     * @param openMsg Open message received from remote BGP speaker
+     * @param preferences Local BGP speaker preferences
      * @return BGP Id of device that accepted the connection
      */
     protected abstract Ipv4Address getDestinationId(final Open openMsg, final BGPSessionPreferences preferences);
 
     /**
-     * @return BGP Id of device that initiated the connection
+     * @param openMsg Open message received from remote BGP speaker
+     * @param preferences Local BGP speaker preferences
+     * @return BGP Id of device that accepted the connection
      */
     protected abstract Ipv4Address getSourceId(final Open openMsg, final BGPSessionPreferences preferences);
 
     public synchronized State getState() {
         return this.state;
     }
+
+    private void negotiationSuccessful(final BGPSessionImpl session) {
+        LOG.debug("Negotiation on channel {} successful with session {}", this.channel, session);
+        this.channel.pipeline().replace(this, "session", session);
+        this.promise.setSuccess(session);
+    }
+
+    private void negotiationFailedCloseChannel(final Throwable cause) {
+        LOG.debug("Negotiation on channel {} failed", this.channel, cause);
+        this.channel.close();
+        this.promise.setFailure(cause);
+    }
+
+    private void sendMessage(final Notification msg) {
+        this.channel.writeAndFlush(msg).addListener(new ChannelFutureListener() {
+            @Override
+            public void operationComplete(final ChannelFuture f) {
+                if (!f.isSuccess()) {
+                    LOG.warn("Failed to send message {}", msg, f.cause());
+                    negotiationFailedCloseChannel(f.cause());
+                } else {
+                    LOG.trace("Message {} sent to socket", msg);
+                }
+
+            }
+        });
+    }
+
+    @Override
+    public final void channelActive(final ChannelHandlerContext ctx) {
+        LOG.debug("Starting session negotiation on channel {}", this.channel);
+
+        try {
+            startNegotiation();
+        } catch (final Exception e) {
+            LOG.warn("Unexpected negotiation failure", e);
+            negotiationFailedCloseChannel(e);
+        }
+
+    }
+
+    @Override
+    public final void channelRead(final ChannelHandlerContext ctx, final Object msg) {
+        LOG.debug("Negotiation read invoked on channel {}", this.channel);
+
+        try {
+            handleMessage((Notification) msg);
+        } catch (final Exception e) {
+            LOG.debug("Unexpected error while handling negotiation message {}", msg, e);
+            negotiationFailedCloseChannel(e);
+        }
+
+    }
+
+    @Override
+    public void exceptionCaught(final ChannelHandlerContext ctx, final Throwable cause) {
+        LOG.info("Unexpected error during negotiation", cause);
+        negotiationFailedCloseChannel(cause);
+    }
 }