From e4e519e6c1c6db47d9c9cd96108f9206d1ca8062 Mon Sep 17 00:00:00 2001 From: Robert Varga Date: Tue, 16 Jan 2024 14:51:32 +0100 Subject: [PATCH] Implement registerNotificationListeners() Bulk listener registration, as introduced a long time ago, should be supported for all clients. Make sure we have an implementation. While we are here, also fix thread safety hazards around registration/unregistration -- with minimal critical sections. Optimize retained memory by specializing empty/one/multiple registrations, which in turn allows for some code reuse. JIRA: NETCONF-1224 Change-Id: I8e40a6c149dbc8bea63d840cbd4cd0a807860175 Signed-off-by: Robert Varga --- .../spi/NetconfDeviceNotificationService.java | 127 ++++++++++++++---- .../netconf/client/mdsal/spi/NC1224Test.java | 78 +++++++++++ .../NetconfDeviceNotificationServiceTest.java | 41 +++--- 3 files changed, 200 insertions(+), 46 deletions(-) create mode 100644 plugins/netconf-client-mdsal/src/test/java/org/opendaylight/netconf/client/mdsal/spi/NC1224Test.java diff --git a/plugins/netconf-client-mdsal/src/main/java/org/opendaylight/netconf/client/mdsal/spi/NetconfDeviceNotificationService.java b/plugins/netconf-client-mdsal/src/main/java/org/opendaylight/netconf/client/mdsal/spi/NetconfDeviceNotificationService.java index 44f9dd03fc..63dbc30d2c 100644 --- a/plugins/netconf-client-mdsal/src/main/java/org/opendaylight/netconf/client/mdsal/spi/NetconfDeviceNotificationService.java +++ b/plugins/netconf-client-mdsal/src/main/java/org/opendaylight/netconf/client/mdsal/spi/NetconfDeviceNotificationService.java @@ -7,68 +7,145 @@ */ package org.opendaylight.netconf.client.mdsal.spi; +import static java.util.Objects.requireNonNull; + +import com.google.common.annotations.VisibleForTesting; import com.google.common.collect.HashMultimap; -import com.google.common.collect.Lists; import com.google.common.collect.Multimap; +import java.util.Arrays; import java.util.Collection; import java.util.Map; +import java.util.Objects; +import org.eclipse.jdt.annotation.NonNull; import org.opendaylight.mdsal.dom.api.DOMNotification; import org.opendaylight.mdsal.dom.api.DOMNotificationListener; import org.opendaylight.mdsal.dom.api.DOMNotificationService; import org.opendaylight.yangtools.concepts.AbstractListenerRegistration; +import org.opendaylight.yangtools.concepts.AbstractRegistration; import org.opendaylight.yangtools.concepts.ListenerRegistration; import org.opendaylight.yangtools.concepts.Registration; import org.opendaylight.yangtools.yang.model.api.stmt.SchemaNodeIdentifier.Absolute; import org.slf4j.Logger; import org.slf4j.LoggerFactory; -public class NetconfDeviceNotificationService implements DOMNotificationService { - +public final class NetconfDeviceNotificationService implements DOMNotificationService { private static final Logger LOG = LoggerFactory.getLogger(NetconfDeviceNotificationService.class); private final Multimap listeners = HashMultimap.create(); // Notification publish is very simple and hijacks the thread of the caller - // TODO shouldnt we reuse the implementation for notification router from sal-broker-impl ? + // TODO: should we not reuse the implementation for notification router from mdsal-dom-broker ? @SuppressWarnings("checkstyle:IllegalCatch") public synchronized void publishNotification(final DOMNotification notification) { - for (final DOMNotificationListener domNotificationListener : listeners.get(notification.getType())) { + for (var listener : listeners.get(notification.getType())) { try { - domNotificationListener.onNotification(notification); - } catch (final Exception e) { - LOG.warn("Listener {} threw an uncaught exception during processing notification {}", - domNotificationListener, notification, e); + listener.onNotification(notification); + } catch (Exception e) { + LOG.warn("Listener {} threw an uncaught exception during processing notification {}", listener, + notification, e); } } } @Override - public synchronized ListenerRegistration registerNotificationListener( - final T listener, final Collection types) { - for (final Absolute type : types) { - listeners.put(type, listener); - } + public ListenerRegistration registerNotificationListener(final T listener, + final Collection types) { + final var lsnr = requireNonNull(listener); + final var typesArray = types.stream().map(Objects::requireNonNull).distinct().toArray(Absolute[]::new); + return switch (typesArray.length) { + case 0 -> new AbstractListenerRegistration<>(lsnr) { + @Override + protected void removeRegistration() { + // No-op + } + }; + case 1 -> registerOne(lsnr, typesArray[0]); + default -> registerMultiple(lsnr, typesArray); + }; + } + + @Override + public ListenerRegistration registerNotificationListener(final T listener, + final Absolute... types) { + return registerNotificationListener(listener, Arrays.asList(types)); + } + @Override + public Registration registerNotificationListeners(final Map typeToListener) { + final var copy = Map.copyOf(typeToListener); + return switch (copy.size()) { + case 0 -> () -> { + // No-op + }; + case 1 -> { + final var entry = copy.entrySet().iterator().next(); + yield registerOne(entry.getValue(), entry.getKey()); + } + default -> registerMultiple(copy); + }; + } + + @VisibleForTesting + synchronized int size() { + return listeners.size(); + } + + private synchronized @NonNull ListenerRegistration registerOne( + final @NonNull T listener, final Absolute type) { + listeners.put(type, listener); return new AbstractListenerRegistration<>(listener) { @Override protected void removeRegistration() { - for (final Absolute type : types) { - listeners.remove(type, listener); + synchronized (NetconfDeviceNotificationService.this) { + listeners.remove(type, getInstance()); } } }; } - @Override - public synchronized ListenerRegistration registerNotificationListener( - final T listener, final Absolute... types) { - return registerNotificationListener(listener, Lists.newArrayList(types)); + private synchronized @NonNull ListenerRegistration registerMultiple( + final @NonNull T listener, final Absolute[] types) { + for (var type : types) { + listeners.put(type, listener); + } + return new AbstractListenerRegistration<>(listener) { + @Override + protected void removeRegistration() { + synchronized (NetconfDeviceNotificationService.this) { + for (var type : types) { + listeners.remove(type, getInstance()); + } + } + } + }; } - @Override - public synchronized Registration registerNotificationListeners( - final Map typeToListener) { - // FIXME: implement this - throw new UnsupportedOperationException(); + private synchronized @NonNull Registration registerMultiple(final Map toReg) { + // we have at least two entries, which we will save as an array of 4 objects + int idx = 0; + final var array = new Object[toReg.size() * 2]; + for (var entry : toReg.entrySet()) { + final var type = entry.getKey(); + final var listener = entry.getValue(); + + listeners.put(type, listener); + array[idx++] = type; + array[idx++] = listener; + } + + return new AbstractRegistration() { + @Override + protected void removeRegistration() { + synchronized (NetconfDeviceNotificationService.this) { + for (int i = 0, length = array.length; i < length; ) { + final var type = array[i++]; + final var listener = array[i++]; + if (!listeners.remove(type, listener)) { + LOG.warn("Failed to remove {} listener {}, very weird", type, listener, new Throwable()); + } + } + } + } + }; } } diff --git a/plugins/netconf-client-mdsal/src/test/java/org/opendaylight/netconf/client/mdsal/spi/NC1224Test.java b/plugins/netconf-client-mdsal/src/test/java/org/opendaylight/netconf/client/mdsal/spi/NC1224Test.java new file mode 100644 index 0000000000..725e1e3da5 --- /dev/null +++ b/plugins/netconf-client-mdsal/src/test/java/org/opendaylight/netconf/client/mdsal/spi/NC1224Test.java @@ -0,0 +1,78 @@ +/* + * Copyright (c) 2024 PANTHEON.tech, s.r.o. and others. All rights reserved. + * + * This program and the accompanying materials are made available under the + * terms of the Eclipse Public License v1.0 which accompanies this distribution, + * and is available at http://www.eclipse.org/legal/epl-v10.html + */ +package org.opendaylight.netconf.client.mdsal.spi; + +import static org.junit.jupiter.api.Assertions.assertEquals; + +import java.util.Map; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.Mock; +import org.mockito.junit.jupiter.MockitoExtension; +import org.opendaylight.mdsal.dom.api.DOMNotificationListener; +import org.opendaylight.yangtools.yang.common.QName; +import org.opendaylight.yangtools.yang.model.api.stmt.SchemaNodeIdentifier.Absolute; + +@ExtendWith(MockitoExtension.class) +class NC1224Test { + private static final Absolute ONE = Absolute.of(QName.create("test", "one")); + private static final Absolute TWO = Absolute.of(QName.create("test", "two")); + + private final NetconfDeviceNotificationService svc = new NetconfDeviceNotificationService(); + + @Mock + private DOMNotificationListener listener; + + @AfterEach + void afterEach() { + assertEquals(0, svc.size()); + } + + @Test + void registerEmpty() { + try (var reg = svc.registerNotificationListener(listener)) { + assertEquals(0, svc.size()); + } + } + + @Test + void registerEmptyMap() { + try (var reg = svc.registerNotificationListeners(Map.of())) { + assertEquals(0, svc.size()); + } + } + + @Test + void registerOne() { + try (var reg = svc.registerNotificationListener(listener, ONE)) { + assertEquals(1, svc.size()); + } + } + + @Test + void registerOneMap() { + try (var reg = svc.registerNotificationListeners(Map.of(ONE, listener))) { + assertEquals(1, svc.size()); + } + } + + @Test + void registerOneTwo() { + try (var reg = svc.registerNotificationListener(listener, ONE, TWO)) { + assertEquals(2, svc.size()); + } + } + + @Test + void registerOneTwoMap() { + try (var reg = svc.registerNotificationListeners(Map.of(ONE, listener, TWO, listener))) { + assertEquals(2, svc.size()); + } + } +} diff --git a/plugins/netconf-client-mdsal/src/test/java/org/opendaylight/netconf/client/mdsal/spi/NetconfDeviceNotificationServiceTest.java b/plugins/netconf-client-mdsal/src/test/java/org/opendaylight/netconf/client/mdsal/spi/NetconfDeviceNotificationServiceTest.java index 9c965736f7..2d3fc9b6a5 100644 --- a/plugins/netconf-client-mdsal/src/test/java/org/opendaylight/netconf/client/mdsal/spi/NetconfDeviceNotificationServiceTest.java +++ b/plugins/netconf-client-mdsal/src/test/java/org/opendaylight/netconf/client/mdsal/spi/NetconfDeviceNotificationServiceTest.java @@ -7,25 +7,28 @@ */ package org.opendaylight.netconf.client.mdsal.spi; +import static org.junit.jupiter.api.Assertions.assertEquals; import static org.mockito.Mockito.doReturn; import static org.mockito.Mockito.never; import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; -import org.junit.Assert; -import org.junit.Before; -import org.junit.Test; -import org.junit.runner.RunWith; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; import org.mockito.Mock; -import org.mockito.junit.MockitoJUnitRunner; +import org.mockito.junit.jupiter.MockitoExtension; import org.opendaylight.mdsal.dom.api.DOMNotification; import org.opendaylight.mdsal.dom.api.DOMNotificationListener; import org.opendaylight.yangtools.concepts.ListenerRegistration; import org.opendaylight.yangtools.yang.common.QName; import org.opendaylight.yangtools.yang.model.api.stmt.SchemaNodeIdentifier.Absolute; -@RunWith(MockitoJUnitRunner.StrictStubs.class) -public class NetconfDeviceNotificationServiceTest { +@ExtendWith(MockitoExtension.class) +class NetconfDeviceNotificationServiceTest { + private static final Absolute PATH1 = Absolute.of(QName.create("namespace1", "path1")); + private static final Absolute PATH2 = Absolute.of(QName.create("namespace2", "path2")); + @Mock private DOMNotificationListener listener1; @Mock @@ -35,24 +38,20 @@ public class NetconfDeviceNotificationServiceTest { @Mock private DOMNotification notification2; - private NetconfDeviceNotificationService service; + private final NetconfDeviceNotificationService service = new NetconfDeviceNotificationService(); private ListenerRegistration registration; + @BeforeEach + void beforeEach() throws Exception { + service.registerNotificationListener(listener1, PATH1); + registration = service.registerNotificationListener(listener2, PATH2); - @Before - public void setUp() throws Exception { - final Absolute path1 = Absolute.of(QName.create("namespace1", "path1")); - final Absolute path2 = Absolute.of(QName.create("namespace2", "path2")); - service = new NetconfDeviceNotificationService(); - service.registerNotificationListener(listener1, path1); - registration = service.registerNotificationListener(listener2, path2); - - doReturn(path1).when(notification1).getType(); - doReturn(path2).when(notification2).getType(); + doReturn(PATH2).when(notification2).getType(); } @Test - public void testPublishNotification() throws Exception { + void testPublishNotification() throws Exception { + doReturn(PATH1).when(notification1).getType(); service.publishNotification(notification1); verify(listener1).onNotification(notification1); @@ -64,9 +63,9 @@ public class NetconfDeviceNotificationServiceTest { } @Test - public void testCloseRegistration() throws Exception { + void testCloseRegistration() throws Exception { service.publishNotification(notification2); - Assert.assertEquals(listener2, registration.getInstance()); + assertEquals(listener2, registration.getInstance()); registration.close(); service.publishNotification(notification2); verify(listener2, times(1)).onNotification(notification2); -- 2.36.6