Fix AbstractDOMRoutingTable.{add,remove}All()

Our naive implementation breaks down if we are registering multiple
items, as we end up adding unmodified entries each and every time.

Fix this up by decomposing identifiers in the caller, keeping them in a
table and then have addAll()/removeAll() work on the table, with careful
checks.

JIRA: MDSAL-796
Change-Id: I0a52763e047026c1a641d9a25d82ee1d193fd573
Signed-off-by: Robert Varga <robert.varga@pantheon.tech>

diff --git a/dom/mdsal-dom-broker/src/main/java/org/opendaylight/mdsal/dom/broker/AbstractDOMRoutingTable.java b/dom/mdsal-dom-broker/src/main/java/org/opendaylight/mdsal/dom/broker/AbstractDOMRoutingTable.java
index 35b2f2202ca0d922a24f4609803699ff12ffa64f..ebf5125bd244e7d137169265fce3a043dc86c992 100644 (file)
--- a/dom/mdsal-dom-broker/src/main/java/org/opendaylight/mdsal/dom/broker/AbstractDOMRoutingTable.java
+++ b/dom/mdsal-dom-broker/src/main/java/org/opendaylight/mdsal/dom/broker/AbstractDOMRoutingTable.java
@@ -11,11 +11,14 @@ import static java.util.Objects.requireNonNull;
 
 import com.google.common.annotations.Beta;
 import com.google.common.annotations.VisibleForTesting;
+import com.google.common.collect.HashBasedTable;
 import com.google.common.collect.HashMultimap;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableMap.Builder;
+import com.google.common.collect.ImmutableTable;
 import com.google.common.collect.ListMultimap;
+import com.google.common.collect.Lists;
 import com.google.common.collect.Maps;
 import com.google.common.collect.Multimaps;
 import java.util.ArrayList;
@@ -69,25 +72,19 @@ abstract class AbstractDOMRoutingTable<I, D, M, L extends EventListener, K,
             return this;
         }
 
-        final Builder<K, E> mb = ImmutableMap.builder();
-        add(implementation, oprsToAdd, mb);
-
-        return newInstance(mb.build(), schemaContext);
-    }
-
-    private void add(final M implementation, final Set<I> oprsToAdd,
-            final Builder<K, E> tableInputBuilder) {
         // First decompose the identifiers to a multimap
         final ListMultimap<K, D> toAdd = decomposeIdentifiers(oprsToAdd);
 
+        final Builder<K, E> mb = ImmutableMap.builder();
+
         // Now iterate over existing entries, modifying them as appropriate...
         for (Entry<K, E> re : this.operations.entrySet()) {
             List<D> newOperations = new ArrayList<>(toAdd.removeAll(re.getKey()));
             if (!newOperations.isEmpty()) {
                 final E ne = (E) re.getValue().add(implementation, newOperations);
-                tableInputBuilder.put(re.getKey(), ne);
+                mb.put(re.getKey(), ne);
             } else {
-                tableInputBuilder.put(re);
+                mb.put(re);
             }
         }
 
@@ -101,24 +98,49 @@ abstract class AbstractDOMRoutingTable<I, D, M, L extends EventListener, K,
 
             final E entry = createOperationEntry(schemaContext, e.getKey(), vb.build());
             if (entry != null) {
-                tableInputBuilder.put(e.getKey(), entry);
+                mb.put(e.getKey(), entry);
             }
         }
+
+        return newInstance(mb.build(), schemaContext);
     }
 
-    AbstractDOMRoutingTable<I, D, M, L, K, E> addAll(final Map<I, M> map) {
-        if (map.isEmpty()) {
+    AbstractDOMRoutingTable<I, D, M, L, K, E> addAll(final ImmutableTable<K, D, M> impls) {
+        if (impls.isEmpty()) {
             return this;
         }
 
-        HashMultimap<M, I> inverted = invertImplementationsMap(map);
+        // Create a temporary map, which we will mutatate
+        final var toAdd = HashBasedTable.create(impls);
+        final var mb = ImmutableMap.<K, E>builder();
+
+        // Now iterate over existing entries, modifying them as appropriate...
+        for (Entry<K, E> re : this.operations.entrySet()) {
+            final var newImpls = toAdd.rowMap().remove(re.getKey());
+            if (newImpls == null) {
+                mb.put(re);
+                continue;
+            }
+
+            var ne = re;
+            for (var oper : newImpls.entrySet()) {
+                @SuppressWarnings("unchecked")
+                final E newVal = (E) ne.getValue().add(oper.getValue(), Lists.newArrayList(oper.getKey()));
+                ne = Map.entry(ne.getKey(), newVal);
+            }
+            mb.put(ne);
+        }
 
-        final Builder<K, E> tableInputBuilder = ImmutableMap.builder();
-        for (M impl : inverted.keySet()) {
-            add(impl, inverted.get(impl), tableInputBuilder);
+        // Finally add whatever is left in the decomposed multimap
+        for (Entry<K, Map<D, M>> e : toAdd.rowMap().entrySet()) {
+            final E entry = createOperationEntry(schemaContext, e.getKey(), ImmutableMap.copyOf(
+                Maps.<D, M, List<M>>transformValues(e.getValue(), ImmutableList::of)));
+            if (entry != null) {
+                mb.put(e.getKey(), entry);
+            }
         }
 
-        return newInstance(tableInputBuilder.build(), schemaContext);
+        return newInstance(mb.build(), schemaContext);
     }
 
     AbstractDOMRoutingTable<I, D, M, L, K, E> remove(final M implementation, final Set<I> instances) {
@@ -147,36 +169,42 @@ abstract class AbstractDOMRoutingTable<I, D, M, L extends EventListener, K,
         return newInstance(b.build(), schemaContext);
     }
 
+    AbstractDOMRoutingTable<I, D, M, L, K, E> removeAll(final ImmutableTable<K, D, M> impls) {
+        if (impls.isEmpty()) {
+            return this;
+        }
 
-    private void remove(final M implementation, final Set<I> oprsToRemove,
-                        final Builder<K, E> tableInputBuilder) {
-        final ListMultimap<K, D> toRemove = decomposeIdentifiers(oprsToRemove);
+        // Create a temporary map, which we will mutatate
+        final var toRemove = HashBasedTable.create(impls);
+        final var mb = ImmutableMap.<K, E>builder();
 
-        for (Entry<K, E> e : this.operations.entrySet()) {
-            final List<D> removed = new ArrayList<>(toRemove.removeAll(e.getKey()));
-            if (!removed.isEmpty()) {
-                final E ne = (E) e.getValue().remove(implementation, removed);
+        // Now iterate over existing entries, modifying them as appropriate...
+        for (Entry<K, E> re : this.operations.entrySet()) {
+            final var oldImpls = toRemove.rowMap().remove(re.getKey());
+            if (oldImpls == null) {
+                mb.put(re);
+                continue;
+            }
+
+            var ne = re;
+            for (var oper : oldImpls.entrySet()) {
                 if (ne != null) {
-                    tableInputBuilder.put(e.getKey(), ne);
+                    @SuppressWarnings("unchecked")
+                    final E newVal = (E) ne.getValue().remove(oper.getValue(), Lists.newArrayList(oper.getKey()));
+                    if (newVal != null) {
+                        ne = Map.entry(ne.getKey(), newVal);
+                    } else {
+                        ne = null;
+                    }
                 }
-            } else {
-                tableInputBuilder.put(e);
             }
-        }
-    }
-
-    AbstractDOMRoutingTable<I, D, M, L, K, E> removeAll(final Map<I, M> map) {
-        if (map.isEmpty()) {
-            return this;
-        }
-        HashMultimap<M, I> inverted = invertImplementationsMap(map);
-
-        final Builder<K, E> tableInputBuilder = ImmutableMap.builder();
-        for (M impl : inverted.keySet()) {
-            remove(impl, inverted.get(impl), tableInputBuilder);
+            if (ne != null) {
+                mb.put(ne);
+            }
         }
 
-        return newInstance(tableInputBuilder.build(), schemaContext);
+        // All done, whatever is in toRemove, was not there in the first place
+        return newInstance(mb.build(), schemaContext);
     }
 
     static final <K, V> HashMultimap<V, K> invertImplementationsMap(final Map<K, V> map) {

diff --git a/dom/mdsal-dom-broker/src/main/java/org/opendaylight/mdsal/dom/broker/DOMRpcRouter.java b/dom/mdsal-dom-broker/src/main/java/org/opendaylight/mdsal/dom/broker/DOMRpcRouter.java
index 46406f83abad70adf2aeaeacd9ed46c2d900c3bd..a3524547c1ed95bc6170986ee0d8dabafba2597d 100644 (file)
--- a/dom/mdsal-dom-broker/src/main/java/org/opendaylight/mdsal/dom/broker/DOMRpcRouter.java
+++ b/dom/mdsal-dom-broker/src/main/java/org/opendaylight/mdsal/dom/broker/DOMRpcRouter.java
@@ -16,8 +16,8 @@ import com.google.common.collect.ClassToInstanceMap;
 import com.google.common.collect.Collections2;
 import com.google.common.collect.ImmutableClassToInstanceMap;
 import com.google.common.collect.ImmutableList;
-import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.ImmutableTable;
 import com.google.common.collect.MapDifference;
 import com.google.common.collect.MapDifference.ValueDifference;
 import com.google.common.collect.Maps;
@@ -165,12 +165,13 @@ public final class DOMRpcRouter extends AbstractRegistration
         listenerNotifier.execute(() -> notifyRemoved(newTable, implementation));
     }
 
-    private synchronized void removeRpcImplementations(final Map<DOMRpcIdentifier, DOMRpcImplementation> map) {
+    private synchronized void removeRpcImplementations(
+            final ImmutableTable<QName, YangInstanceIdentifier, DOMRpcImplementation> implTable) {
         final DOMRpcRoutingTable oldTable = routingTable;
-        final DOMRpcRoutingTable newTable = (DOMRpcRoutingTable) oldTable.removeAll(map);
+        final DOMRpcRoutingTable newTable = (DOMRpcRoutingTable) oldTable.removeAll(implTable);
         routingTable = newTable;
 
-        listenerNotifier.execute(() -> notifyRemoved(newTable, map.values()));
+        listenerNotifier.execute(() -> notifyRemoved(newTable, implTable.values()));
     }
 
     private synchronized void removeActionImplementation(final DOMActionImplementation implementation,
@@ -526,21 +527,27 @@ public final class DOMRpcRouter extends AbstractRegistration
         @Override
         public org.opendaylight.yangtools.concepts.Registration registerRpcImplementations(
                 final Map<DOMRpcIdentifier, DOMRpcImplementation> map) {
-            final ImmutableMap<DOMRpcIdentifier, DOMRpcImplementation> defensive = ImmutableMap.copyOf(map);
             checkArgument(!map.isEmpty());
 
+            final var builder = ImmutableTable.<QName, YangInstanceIdentifier, DOMRpcImplementation>builder();
+            for (var entry : map.entrySet()) {
+                final var id = entry.getKey();
+                builder.put(id.getType(), id.getContextReference(), entry.getValue());
+            }
+            final var implTable = builder.build();
+
             synchronized (DOMRpcRouter.this) {
                 final DOMRpcRoutingTable oldTable = routingTable;
-                final DOMRpcRoutingTable newTable = (DOMRpcRoutingTable) oldTable.addAll(defensive);
+                final DOMRpcRoutingTable newTable = (DOMRpcRoutingTable) oldTable.addAll(implTable);
                 routingTable = newTable;
 
-                listenerNotifier.execute(() -> notifyAdded(newTable, defensive.values()));
+                listenerNotifier.execute(() -> notifyAdded(newTable, implTable.values()));
             }
 
             return new AbstractRegistration() {
                 @Override
                 protected void removeRegistration() {
-                    removeRpcImplementations(defensive);
+                    removeRpcImplementations(implTable);
                 }
             };
         }

diff --git a/dom/mdsal-dom-broker/src/test/java/org/opendaylight/mdsal/dom/broker/DOMRpcRouterTest.java b/dom/mdsal-dom-broker/src/test/java/org/opendaylight/mdsal/dom/broker/DOMRpcRouterTest.java
index 164f4e545854dd7a4d2f459c8a530963b600380d..0cf9118cbdc64248a4d242086f30bd65d6c9c6b3 100644 (file)
--- a/dom/mdsal-dom-broker/src/test/java/org/opendaylight/mdsal/dom/broker/DOMRpcRouterTest.java
+++ b/dom/mdsal-dom-broker/src/test/java/org/opendaylight/mdsal/dom/broker/DOMRpcRouterTest.java
@@ -25,6 +25,7 @@ import static org.opendaylight.mdsal.dom.broker.TestUtils.getTestRpcImplementati
 import com.google.common.util.concurrent.Futures;
 import com.google.common.util.concurrent.ListenableFuture;
 import java.util.List;
+import java.util.Map;
 import java.util.Set;
 import java.util.concurrent.ExecutionException;
 import java.util.concurrent.RejectedExecutionException;
@@ -88,6 +89,29 @@ public class DOMRpcRouterTest {
         }
     }
 
+    @Test
+    public void registerRpcImplementations() {
+        try (DOMRpcRouter rpcRouter = rpcsRouter()) {
+            assertOperationKeys(rpcRouter);
+
+            final Registration fooReg = rpcRouter.getRpcProviderService().registerRpcImplementations(
+                Map.of(DOMRpcIdentifier.create(Rpcs.FOO, null), getTestRpcImplementation()));
+            assertOperationKeys(rpcRouter, Rpcs.FOO);
+
+            final Registration barReg = rpcRouter.getRpcProviderService().registerRpcImplementations(
+                Map.of(
+                    DOMRpcIdentifier.create(Rpcs.BAR, null), getTestRpcImplementation(),
+                    DOMRpcIdentifier.create(Rpcs.BAZ, null), getTestRpcImplementation()));
+            assertOperationKeys(rpcRouter, Rpcs.FOO, Rpcs.BAR, Rpcs.BAZ);
+
+            fooReg.close();
+            assertOperationKeys(rpcRouter, Rpcs.BAR, Rpcs.BAZ);
+            barReg.close();
+            assertOperationKeys(rpcRouter);
+        }
+    }
+
+
     private static void assertOperationKeys(final DOMRpcRouter router, final QName... keys) {
         assertEquals(Set.of(keys), router.routingTable().getOperations().keySet());
     }
@@ -223,13 +247,9 @@ public class DOMRpcRouterTest {
         return router;
     }
 
-    private static void assertAvailable(final DOMActionService actionService, final YangInstanceIdentifier path) {
-        final DOMActionResult result;
-        try {
-            result = Futures.getDone(invokeBaz(actionService, path));
-        } catch (ExecutionException e) {
-            throw new AssertionError("Unexpected invocation failure", e);
-        }
+    private static void assertAvailable(final DOMActionService actionService, final YangInstanceIdentifier path)
+            throws ExecutionException {
+        final DOMActionResult result = Futures.getDone(invokeBaz(actionService, path));
         assertEquals(List.of(), result.getErrors());
     }