From: Robert Varga Date: Mon, 27 Apr 2020 11:42:12 +0000 (+0200) Subject: Fix unsubscribe checks in DOMNotificationRouterEvent X-Git-Tag: v4.0.14~4 X-Git-Url: https://git.opendaylight.org/gerrit/gitweb?p=mdsal.git;a=commitdiff_plain;h=355f88c4b2096ac7aa7e6a32d4707eefa8fe0ef9 Fix unsubscribe checks in DOMNotificationRouterEvent The checks here are based on old assumption that the listener would be set to null, which does not hold true for quite some time. Update the check to close a simple race. JIRA: MDSAL-545 Change-Id: I950ffcbcf732d4c24dcad675b81c2889465d9045 Signed-off-by: Robert Varga (cherry picked from commit 91749a5a5fb089e74306f288d786acb8d3c450ae) --- diff --git a/dom/mdsal-dom-broker/src/main/java/org/opendaylight/mdsal/dom/broker/DOMNotificationRouter.java b/dom/mdsal-dom-broker/src/main/java/org/opendaylight/mdsal/dom/broker/DOMNotificationRouter.java index c609f3389c..67562a8a33 100644 --- a/dom/mdsal-dom-broker/src/main/java/org/opendaylight/mdsal/dom/broker/DOMNotificationRouter.java +++ b/dom/mdsal-dom-broker/src/main/java/org/opendaylight/mdsal/dom/broker/DOMNotificationRouter.java @@ -82,7 +82,7 @@ public class DOMNotificationRouter implements AutoCloseable, DOMNotificationPubl private final ScheduledThreadPoolExecutor observer; private final ExecutorService executor; - private volatile Multimap> listeners = + private volatile Multimap> listeners = ImmutableMultimap.of(); @VisibleForTesting @@ -113,7 +113,7 @@ public class DOMNotificationRouter implements AutoCloseable, DOMNotificationPubl @Override public synchronized ListenerRegistration registerNotificationListener( final T listener, final Collection types) { - final ListenerRegistration reg = new AbstractListenerRegistration(listener) { + final AbstractListenerRegistration reg = new AbstractListenerRegistration(listener) { @Override protected void removeRegistration() { synchronized (DOMNotificationRouter.this) { @@ -124,7 +124,7 @@ public class DOMNotificationRouter implements AutoCloseable, DOMNotificationPubl }; if (!types.isEmpty()) { - final Builder> b = + final Builder> b = ImmutableMultimap.builder(); b.putAll(listeners); @@ -150,7 +150,7 @@ public class DOMNotificationRouter implements AutoCloseable, DOMNotificationPubl * @param newListeners is used to notify listenerTypes changed */ private void replaceListeners( - final Multimap> newListeners) { + final Multimap> newListeners) { listeners = newListeners; notifyListenerTypesChanged(newListeners.keySet()); } @@ -180,7 +180,7 @@ public class DOMNotificationRouter implements AutoCloseable, DOMNotificationPubl } private ListenableFuture publish(final long seq, final DOMNotification notification, - final Collection> subscribers) { + final Collection> subscribers) { final DOMNotificationRouterEvent event = disruptor.get(seq); final ListenableFuture future = event.initialize(notification, subscribers); disruptor.getRingBuffer().publish(seq); @@ -190,7 +190,7 @@ public class DOMNotificationRouter implements AutoCloseable, DOMNotificationPubl @Override public ListenableFuture putNotification(final DOMNotification notification) throws InterruptedException { - final Collection> subscribers = + final Collection> subscribers = listeners.get(notification.getType()); if (subscribers.isEmpty()) { return NO_LISTENERS; @@ -203,7 +203,7 @@ public class DOMNotificationRouter implements AutoCloseable, DOMNotificationPubl @SuppressWarnings("checkstyle:IllegalCatch") @VisibleForTesting ListenableFuture tryPublish(final DOMNotification notification, - final Collection> subscribers) { + final Collection> subscribers) { final long seq; try { seq = disruptor.getRingBuffer().tryNext(); @@ -216,7 +216,7 @@ public class DOMNotificationRouter implements AutoCloseable, DOMNotificationPubl @Override public ListenableFuture offerNotification(final DOMNotification notification) { - final Collection> subscribers = + final Collection> subscribers = listeners.get(notification.getType()); if (subscribers.isEmpty()) { return NO_LISTENERS; @@ -228,7 +228,7 @@ public class DOMNotificationRouter implements AutoCloseable, DOMNotificationPubl @Override public ListenableFuture offerNotification(final DOMNotification notification, final long timeout, final TimeUnit unit) throws InterruptedException { - final Collection> subscribers = + final Collection> subscribers = listeners.get(notification.getType()); if (subscribers.isEmpty()) { return NO_LISTENERS; diff --git a/dom/mdsal-dom-broker/src/main/java/org/opendaylight/mdsal/dom/broker/DOMNotificationRouterEvent.java b/dom/mdsal-dom-broker/src/main/java/org/opendaylight/mdsal/dom/broker/DOMNotificationRouterEvent.java index 6eafd0696f..8743fc90d7 100644 --- a/dom/mdsal-dom-broker/src/main/java/org/opendaylight/mdsal/dom/broker/DOMNotificationRouterEvent.java +++ b/dom/mdsal-dom-broker/src/main/java/org/opendaylight/mdsal/dom/broker/DOMNotificationRouterEvent.java @@ -15,7 +15,7 @@ import com.lmax.disruptor.EventFactory; import java.util.Collection; import org.opendaylight.mdsal.dom.api.DOMNotification; import org.opendaylight.mdsal.dom.api.DOMNotificationListener; -import org.opendaylight.yangtools.concepts.ListenerRegistration; +import org.opendaylight.yangtools.concepts.AbstractListenerRegistration; /** * A single notification event in the disruptor ringbuffer. These objects are reused, @@ -24,7 +24,7 @@ import org.opendaylight.yangtools.concepts.ListenerRegistration; final class DOMNotificationRouterEvent { static final EventFactory FACTORY = DOMNotificationRouterEvent::new; - private Collection> subscribers; + private Collection> subscribers; private DOMNotification notification; private SettableFuture future; @@ -34,7 +34,7 @@ final class DOMNotificationRouterEvent { @SuppressWarnings("checkstyle:hiddenField") ListenableFuture initialize(final DOMNotification notification, - final Collection> subscribers) { + final Collection> subscribers) { this.notification = requireNonNull(notification); this.subscribers = requireNonNull(subscribers); this.future = SettableFuture.create(); @@ -42,10 +42,9 @@ final class DOMNotificationRouterEvent { } void deliverNotification() { - for (ListenerRegistration r : subscribers) { - final DOMNotificationListener l = r.getInstance(); - if (l != null) { - l.onNotification(notification); + for (AbstractListenerRegistration r : subscribers) { + if (r.notClosed()) { + r.getInstance().onNotification(notification); } } } diff --git a/dom/mdsal-dom-broker/src/test/java/org/opendaylight/mdsal/dom/broker/DOMNotificationRouterTest.java b/dom/mdsal-dom-broker/src/test/java/org/opendaylight/mdsal/dom/broker/DOMNotificationRouterTest.java index 4a21a4416e..3717b8a521 100644 --- a/dom/mdsal-dom-broker/src/test/java/org/opendaylight/mdsal/dom/broker/DOMNotificationRouterTest.java +++ b/dom/mdsal-dom-broker/src/test/java/org/opendaylight/mdsal/dom/broker/DOMNotificationRouterTest.java @@ -30,7 +30,7 @@ import org.opendaylight.mdsal.dom.api.DOMNotification; import org.opendaylight.mdsal.dom.api.DOMNotificationListener; import org.opendaylight.mdsal.dom.api.DOMNotificationPublishService; import org.opendaylight.mdsal.dom.spi.DOMNotificationSubscriptionListener; -import org.opendaylight.yangtools.concepts.ListenerRegistration; +import org.opendaylight.yangtools.concepts.AbstractListenerRegistration; import org.opendaylight.yangtools.util.ListenerRegistry; import org.opendaylight.yangtools.yang.model.api.SchemaPath; @@ -164,7 +164,7 @@ public class DOMNotificationRouterTest extends TestUtils { @Override protected ListenableFuture tryPublish(final DOMNotification notification, - final Collection> subscribers) { + final Collection> subscribers) { return DOMNotificationPublishService.REJECTED; }