From 940751adf968a6789514566281bb0817bbf07878 Mon Sep 17 00:00:00 2001 From: Robert Varga Date: Sun, 14 Dec 2014 19:20:00 +0100 Subject: [PATCH] BUG-2510: Remove all registrations when a routed rpc is closed The API contract specifies that the effects of a Registration are removed on close(). Unfortunately the routed RPC case is more funny, as it has sub-registrations, for each added path -- and we have no mention of the fact that the user should be removing them. Hence we need to handle the user expecting us to remove any and all paths which have been registered. Change-Id: Iedf451c2a481d648173819f895cccb9e63c54d99 Signed-off-by: Robert Varga --- .../impl/RpcRouterCodegenInstance.java | 46 ++++++++++++++----- .../codegen/impl/RpcRoutingTableImpl.java | 36 ++++++++------- 2 files changed, 54 insertions(+), 28 deletions(-) diff --git a/opendaylight/md-sal/sal-binding-broker/src/main/java/org/opendaylight/controller/sal/binding/codegen/impl/RpcRouterCodegenInstance.java b/opendaylight/md-sal/sal-binding-broker/src/main/java/org/opendaylight/controller/sal/binding/codegen/impl/RpcRouterCodegenInstance.java index 783e5c0cd4..d69aeed352 100644 --- a/opendaylight/md-sal/sal-binding-broker/src/main/java/org/opendaylight/controller/sal/binding/codegen/impl/RpcRouterCodegenInstance.java +++ b/opendaylight/md-sal/sal-binding-broker/src/main/java/org/opendaylight/controller/sal/binding/codegen/impl/RpcRouterCodegenInstance.java @@ -8,11 +8,14 @@ package org.opendaylight.controller.sal.binding.codegen.impl; import static org.opendaylight.controller.sal.binding.codegen.RuntimeCodeHelper.setRoutingTable; - +import com.google.common.collect.ImmutableMap; +import com.google.common.collect.ImmutableSet; +import java.util.ArrayList; +import java.util.Collection; import java.util.HashMap; import java.util.Map; import java.util.Set; - +import javax.annotation.concurrent.GuardedBy; import org.opendaylight.controller.md.sal.common.api.routing.RouteChange; import org.opendaylight.controller.md.sal.common.api.routing.RouteChangeListener; import org.opendaylight.controller.sal.binding.api.BindingAwareBroker.RoutedRpcRegistration; @@ -29,9 +32,6 @@ import org.opendaylight.yangtools.yang.binding.RpcService; import org.slf4j.Logger; import org.slf4j.LoggerFactory; -import com.google.common.collect.ImmutableMap; -import com.google.common.collect.ImmutableSet; - public class RpcRouterCodegenInstance implements // RpcRouter, RouteChangeListener, InstanceIdentifier> { @@ -133,7 +133,14 @@ RpcRouter, RouteChangeListener, InstanceIdentif return new DefaultRpcImplementationRegistration(service); } - private class RoutedRpcRegistrationImpl extends AbstractObjectRegistration implements RoutedRpcRegistration { + private final class RoutedRpcRegistrationImpl extends AbstractObjectRegistration implements RoutedRpcRegistration { + /* + * FIXME: retaining this collection is not completely efficient. We really should be storing + * a reference to this registration, as a particular listener may be registered multiple + * times -- and then this goes kaboom in various aspects. + */ + @GuardedBy("this") + private final Collection> contexts = new ArrayList<>(1); public RoutedRpcRegistrationImpl(final T instance) { super(instance); @@ -145,32 +152,49 @@ RpcRouter, RouteChangeListener, InstanceIdentif } @Override - public void registerPath(final Class context, final InstanceIdentifier path) { + public synchronized void registerPath(final Class context, final InstanceIdentifier path) { + if (isClosed()) { + LOG.debug("Closed registration of {} ignoring new path {}", getInstance(), path); + return; + } + routingTables.get(context).updateRoute(path, getInstance()); + contexts.add(context); } @Override - public void unregisterPath(final Class context, final InstanceIdentifier path) { + public synchronized void unregisterPath(final Class context, final InstanceIdentifier path) { + if (isClosed()) { + LOG.debug("Closed unregistration of {} ignoring new path {}", getInstance(), path); + return; + } + routingTables.get(context).removeRoute(path, getInstance()); + contexts.remove(context); } + @Deprecated @Override public void registerInstance(final Class context, final InstanceIdentifier instance) { registerPath(context, instance); } + @Deprecated @Override public void unregisterInstance(final Class context, final InstanceIdentifier instance) { unregisterPath(context, instance); } @Override - protected void removeRegistration() { - + protected synchronized void removeRegistration() { + for (Class ctx : contexts) { + routingTables.get(ctx).removeAllReferences(getInstance()); + } + contexts.clear(); } } - private class DefaultRpcImplementationRegistration extends AbstractObjectRegistration implements RpcRegistration { + private final class DefaultRpcImplementationRegistration extends AbstractObjectRegistration implements RpcRegistration { protected DefaultRpcImplementationRegistration(final T instance) { diff --git a/opendaylight/md-sal/sal-binding-broker/src/main/java/org/opendaylight/controller/sal/binding/codegen/impl/RpcRoutingTableImpl.java b/opendaylight/md-sal/sal-binding-broker/src/main/java/org/opendaylight/controller/sal/binding/codegen/impl/RpcRoutingTableImpl.java index ce159b8f3e..78fa88bb1b 100644 --- a/opendaylight/md-sal/sal-binding-broker/src/main/java/org/opendaylight/controller/sal/binding/codegen/impl/RpcRoutingTableImpl.java +++ b/opendaylight/md-sal/sal-binding-broker/src/main/java/org/opendaylight/controller/sal/binding/codegen/impl/RpcRoutingTableImpl.java @@ -8,10 +8,10 @@ package org.opendaylight.controller.sal.binding.codegen.impl; import java.util.Collections; +import java.util.Iterator; import java.util.Map; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.ConcurrentMap; - import org.opendaylight.controller.md.sal.common.api.routing.RouteChangeListener; import org.opendaylight.controller.md.sal.common.api.routing.RouteChangePublisher; import org.opendaylight.controller.md.sal.common.impl.routing.RoutingUtils; @@ -25,8 +25,7 @@ import org.opendaylight.yangtools.yang.binding.RpcService; import org.slf4j.Logger; import org.slf4j.LoggerFactory; -class RpcRoutingTableImpl // -implements // +final class RpcRoutingTableImpl implements Mutable, // RpcRoutingTable, // RouteChangePublisher, InstanceIdentifier> { @@ -42,7 +41,7 @@ implements // private RouteChangeListener, InstanceIdentifier> listener; private S defaultRoute; - public RpcRoutingTableImpl(String routerName,Class contextType, Class serviceType) { + public RpcRoutingTableImpl(final String routerName,final Class contextType, final Class serviceType) { super(); this.routerName = routerName; this.serviceType = serviceType; @@ -52,7 +51,7 @@ implements // } @Override - public void setDefaultRoute(S target) { + public void setDefaultRoute(final S target) { defaultRoute = target; } @@ -63,7 +62,7 @@ implements // @Override public , InstanceIdentifier>> ListenerRegistration registerRouteChangeListener( - L listener) { + final L listener) { return new SingletonListenerRegistration(listener); } @@ -74,7 +73,7 @@ implements // @Override @SuppressWarnings("unchecked") - public void updateRoute(InstanceIdentifier path, S service) { + public void updateRoute(final InstanceIdentifier path, final S service) { S previous = this.routes.put(path, service); LOGGER.debug("Route {} updated to {} in routing table {}",path,service,this); @@ -88,7 +87,7 @@ implements // @Override @SuppressWarnings("unchecked") - public void removeRoute(InstanceIdentifier path) { + public void removeRoute(final InstanceIdentifier path) { S previous = this.routes.remove(path); LOGGER.debug("Route {} to {} removed in routing table {}",path,previous,this); @SuppressWarnings("rawtypes") @@ -98,7 +97,7 @@ implements // } } - public void removeRoute(InstanceIdentifier path, S service) { + void removeRoute(final InstanceIdentifier path, final S service) { @SuppressWarnings("rawtypes") RouteChangeListener listenerCapture = listener; if (routes.remove(path, service) && listenerCapture != null) { @@ -108,7 +107,7 @@ implements // } @Override - public S getRoute(InstanceIdentifier nodeInstance) { + public S getRoute(final InstanceIdentifier nodeInstance) { S route = routes.get(nodeInstance); if (route != null) { return route; @@ -121,25 +120,28 @@ implements // return unmodifiableRoutes; } - protected void removeAllReferences(S service) { - + void removeAllReferences(final S service) { + // FIXME: replace this via properly-synchronized BiMap (or something) + final Iterator it = routes.values().iterator(); + while (it.hasNext()) { + final S s = it.next(); + if (service.equals(s)) { + it.remove(); + } + } } - - @Override public String toString() { return "RpcRoutingTableImpl [router=" + routerName + ", service=" + serviceType.getSimpleName() + ", context=" + contextType.getSimpleName() + "]"; } - - private class SingletonListenerRegistration, InstanceIdentifier>> extends AbstractObjectRegistration implements ListenerRegistration { - public SingletonListenerRegistration(L instance) { + public SingletonListenerRegistration(final L instance) { super(instance); listener = instance; } -- 2.36.6