From cf2dc3617d3a66fb1a22285b2a6bc867a6014d4e Mon Sep 17 00:00:00 2001 From: Dana Kutenicsova Date: Wed, 1 Oct 2014 12:54:32 +0200 Subject: [PATCH] BUG-2109 : cleaned BGP sessions on session closed. Added proper close from BGP peer. Negotiation failed method now called from both sub and superclass. Change-Id: I83e922df826c7fbb9130ca637d2dfe99d3e93a27 Signed-off-by: Dana Kutenicsova --- .../impl/AbstractBGPSessionNegotiator.java | 49 ++++++++++--------- .../protocol/bgp/rib/impl/BGPPeer.java | 14 ++---- 2 files changed, 31 insertions(+), 32 deletions(-) diff --git a/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/AbstractBGPSessionNegotiator.java b/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/AbstractBGPSessionNegotiator.java index 0192bb9daa..e865690bbe 100644 --- a/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/AbstractBGPSessionNegotiator.java +++ b/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/AbstractBGPSessionNegotiator.java @@ -10,14 +10,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.util.concurrent.Promise; - import java.util.concurrent.TimeUnit; - import javax.annotation.concurrent.GuardedBy; - import org.opendaylight.protocol.bgp.parser.BGPDocumentedException; import org.opendaylight.protocol.bgp.parser.BGPError; import org.opendaylight.protocol.bgp.rib.impl.spi.BGPPeerRegistry; @@ -94,11 +90,11 @@ public abstract class AbstractBGPSessionNegotiator extends AbstractSessionNegoti Preconditions.checkState(this.state == State.Idle); // Check if peer is configured in registry before retrieving preferences - if (!registry.isPeerConfigured(getRemoteIp())) { + if (!this.registry.isPeerConfigured(getRemoteIp())) { final BGPDocumentedException cause = new BGPDocumentedException( "BGP peer with ip: " + getRemoteIp() + " not configured, check configured peers in : " - + registry, BGPError.CEASE); + + this.registry, BGPError.CEASE); negotiationFailed(cause); return; } @@ -112,26 +108,28 @@ public abstract class AbstractBGPSessionNegotiator extends AbstractSessionNegoti } this.sendMessage(new OpenBuilder().setMyAsNumber(as).setHoldTimer(preferences.getHoldTime()).setBgpIdentifier( preferences.getBgpId()).setBgpParameters(preferences.getParams()).build()); - this.state = State.OpenSent; - - this.channel.eventLoop().schedule(new Runnable() { - @Override - public void run() { - if (AbstractBGPSessionNegotiator.this.state != State.Finished) { - AbstractBGPSessionNegotiator.this.sendMessage(buildErrorNotify(BGPError.HOLD_TIMER_EXPIRED)); - negotiationFailed(new BGPDocumentedException("HoldTimer expired", BGPError.FSM_ERROR)); - AbstractBGPSessionNegotiator.this.state = State.Finished; + if (this.state != State.Finished) { + this.state = State.OpenSent; + + this.channel.eventLoop().schedule(new Runnable() { + @Override + public void run() { + if (AbstractBGPSessionNegotiator.this.state != State.Finished) { + AbstractBGPSessionNegotiator.this.sendMessage(buildErrorNotify(BGPError.HOLD_TIMER_EXPIRED)); + negotiationFailed(new BGPDocumentedException("HoldTimer expired", BGPError.FSM_ERROR)); + AbstractBGPSessionNegotiator.this.state = State.Finished; + } } - } - }, INITIAL_HOLDTIMER, TimeUnit.MINUTES); + }, INITIAL_HOLDTIMER, TimeUnit.MINUTES); + } } private BGPSessionPreferences getPreferences() { - return registry.getPeerPreferences(getRemoteIp()); + return this.registry.getPeerPreferences(getRemoteIp()); } private IpAddress getRemoteIp() { - return StrictBGPPeerRegistry.getIpAddress(channel.remoteAddress()); + return StrictBGPPeerRegistry.getIpAddress(this.channel.remoteAddress()); } @Override @@ -175,14 +173,14 @@ public abstract class AbstractBGPSessionNegotiator extends AbstractSessionNegoti private void handleOpen(final Open openObj) { try { - sessionValidator.validate(openObj, getPreferences()); + this.sessionValidator.validate(openObj, getPreferences()); } catch (final BGPDocumentedException e) { negotiationFailed(e); return; } try { - final BGPSessionListener peer = registry.getPeer(getRemoteIp(), getSourceId(openObj, getPreferences()), getDestinationId(openObj, getPreferences())); + 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().getHoldTime()); this.state = State.OpenConfirm; @@ -193,9 +191,14 @@ public abstract class AbstractBGPSessionNegotiator extends AbstractSessionNegoti } } - private void negotiationFailed(final BGPDocumentedException e) { + @Override + protected void negotiationFailed(final Throwable e) { LOG.warn("Channel {} negotiation failed: {}", this.channel, e.getMessage()); - this.sendMessage(buildErrorNotify(e.getError())); + 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())); + } super.negotiationFailed(e); this.state = State.Finished; } diff --git a/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/BGPPeer.java b/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/BGPPeer.java index a3949d4d87..266a32d2ff 100644 --- a/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/BGPPeer.java +++ b/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/BGPPeer.java @@ -12,12 +12,9 @@ import com.google.common.base.Objects; import com.google.common.base.Objects.ToStringHelper; import com.google.common.base.Preconditions; import com.google.common.net.InetAddresses; - import java.util.HashSet; import java.util.Set; - import javax.annotation.concurrent.GuardedBy; - import org.opendaylight.protocol.bgp.rib.impl.spi.AdjRIBsOutRegistration; import org.opendaylight.protocol.bgp.rib.impl.spi.RIB; import org.opendaylight.protocol.bgp.rib.impl.spi.ReusableBGPPeer; @@ -97,25 +94,24 @@ public class BGPPeer implements ReusableBGPPeer, Peer, AutoCloseable { this.rib.clearTable(this, key); } - if (reg != null) { - reg.close(); - reg = null; + if (this.reg != null) { + this.reg.close(); + this.reg = null; } this.tables.clear(); - this.session = null; } @Override public void onSessionDown(final BGPSession session, final Exception e) { LOG.info("Session with peer {} went down", this.name, e); - cleanup(); + releaseConnection(); } @Override public void onSessionTerminated(final BGPSession session, final BGPTerminationReason cause) { LOG.info("Session with peer {} terminated: {}", this.name, cause); - cleanup(); + releaseConnection(); } @Override -- 2.36.6