From 38402d3e9b8976a40d7926efe47a06243210562f Mon Sep 17 00:00:00 2001 From: Tom Pantelis Date: Tue, 14 Jun 2016 10:18:30 -0400 Subject: [PATCH] Bug 5913: Fix ISE in DefaultShardDataChangeListenerPublisher The publishChanges method is only called from the ShardDataTreeNotificationPublisherActor which is single-threaded so publishChanges can't be called concurrently. However the DefaultShardDataChangeListenerPublisher instance is passed via the PublishNotifications message so the Stopwatch isn't thread safe wrt thread visibility of its internal state. Therefore it's possible the change in state done on thread 1 isn't immediately visible to a subsequent thread. To alleviate this, I moved the Stopwatch and the elapsed time check to the ShardDataTreeNotificationPublisherActor. Change-Id: I046e7e92aa96eec01d5a355c8431ef797c534ead Signed-off-by: Tom Pantelis --- ...taTreeNotificationPublisherActorProxy.java | 2 +- ...faultShardDataChangeListenerPublisher.java | 20 +----------- ...tShardDataTreeChangeListenerPublisher.java | 21 +------------ ...ardDataTreeNotificationPublisherActor.java | 31 +++++++++++++++++-- 4 files changed, 31 insertions(+), 43 deletions(-) diff --git a/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/AbstractShardDataTreeNotificationPublisherActorProxy.java b/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/AbstractShardDataTreeNotificationPublisherActorProxy.java index 93081eb735..61a22d4c2c 100644 --- a/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/AbstractShardDataTreeNotificationPublisherActorProxy.java +++ b/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/AbstractShardDataTreeNotificationPublisherActorProxy.java @@ -55,7 +55,7 @@ abstract class AbstractShardDataTreeNotificationPublisherActorProxy implements S String dispatcher = new Dispatchers(actorContext.system().dispatchers()).getDispatcherPath( Dispatchers.DispatcherType.Notification); - notifierActor = actorContext.actorOf(ShardDataTreeNotificationPublisherActor.props() + notifierActor = actorContext.actorOf(ShardDataTreeNotificationPublisherActor.props(actorName) .withDispatcher(dispatcher).withMailbox( org.opendaylight.controller.cluster.datastore.utils.ActorContext.BOUNDED_MAILBOX), actorName); } diff --git a/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/DefaultShardDataChangeListenerPublisher.java b/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/DefaultShardDataChangeListenerPublisher.java index cf98f38d07..bf52aa14c9 100644 --- a/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/DefaultShardDataChangeListenerPublisher.java +++ b/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/DefaultShardDataChangeListenerPublisher.java @@ -7,8 +7,6 @@ */ package org.opendaylight.controller.cluster.datastore; -import com.google.common.base.Stopwatch; -import java.util.concurrent.TimeUnit; import javax.annotation.concurrent.NotThreadSafe; import org.opendaylight.controller.md.sal.common.api.data.AsyncDataBroker.DataChangeScope; import org.opendaylight.controller.md.sal.common.api.data.AsyncDataChangeListener; @@ -35,7 +33,6 @@ final class DefaultShardDataChangeListenerPublisher implements ShardDataChangeLi private static final Logger LOG = LoggerFactory.getLogger(DefaultShardDataChangeListenerPublisher.class); private final ListenerTree dataChangeListenerTree = ListenerTree.create(); - private final Stopwatch timer = Stopwatch.createUnstarted(); @Override public void submitNotification(final DataChangeListenerRegistration listener, final DOMImmutableDataChangeEvent notification) { @@ -56,22 +53,7 @@ final class DefaultShardDataChangeListenerPublisher implements ShardDataChangeLi @Override public void publishChanges(DataTreeCandidate candidate, String logContext) { - timer.start(); - - try { - ResolveDataChangeEventsTask.create(candidate, dataChangeListenerTree).resolve(this); - } finally { - timer.stop(); - long elapsedTime = timer.elapsed(TimeUnit.MILLISECONDS); - if(elapsedTime >= PUBLISH_DELAY_THRESHOLD_IN_MS) { - LOG.warn("{}: Generation of DataChange events took longer than expected. Elapsed time: {}", - logContext, timer); - } else { - LOG.debug("{}: Elapsed time for generation of DataChange events: {}", logContext, timer); - } - - timer.reset(); - } + ResolveDataChangeEventsTask.create(candidate, dataChangeListenerTree).resolve(this); } @Override diff --git a/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/DefaultShardDataTreeChangeListenerPublisher.java b/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/DefaultShardDataTreeChangeListenerPublisher.java index 217ffd358c..b539443f6e 100644 --- a/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/DefaultShardDataTreeChangeListenerPublisher.java +++ b/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/DefaultShardDataTreeChangeListenerPublisher.java @@ -7,10 +7,8 @@ */ package org.opendaylight.controller.cluster.datastore; -import com.google.common.base.Stopwatch; import java.util.Collection; import java.util.Collections; -import java.util.concurrent.TimeUnit; import javax.annotation.concurrent.NotThreadSafe; import org.opendaylight.controller.md.sal.dom.spi.AbstractDOMDataTreeChangeListenerRegistration; import org.opendaylight.controller.sal.core.spi.data.AbstractDOMStoreTreeChangePublisher; @@ -32,26 +30,9 @@ final class DefaultShardDataTreeChangeListenerPublisher extends AbstractDOMStore implements ShardDataTreeChangeListenerPublisher { private static final Logger LOG = LoggerFactory.getLogger(DefaultShardDataTreeChangeListenerPublisher.class); - private final Stopwatch timer = Stopwatch.createUnstarted(); - @Override public void publishChanges(final DataTreeCandidate candidate, String logContext) { - timer.start(); - - try { - processCandidateTree(candidate); - } finally { - timer.stop(); - long elapsedTime = timer.elapsed(TimeUnit.MILLISECONDS); - if(elapsedTime >= PUBLISH_DELAY_THRESHOLD_IN_MS) { - LOG.warn("{}: Generation of DataTreeCandidateNode events took longer than expected. Elapsed time: {}", - logContext, timer); - } else { - LOG.debug("{}: Elapsed time for generation of DataTreeCandidateNode events: {}", logContext, timer); - } - - timer.reset(); - } + processCandidateTree(candidate); } @Override diff --git a/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/ShardDataTreeNotificationPublisherActor.java b/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/ShardDataTreeNotificationPublisherActor.java index e4e7eb33e9..8102356280 100644 --- a/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/ShardDataTreeNotificationPublisherActor.java +++ b/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/ShardDataTreeNotificationPublisherActor.java @@ -8,6 +8,8 @@ package org.opendaylight.controller.cluster.datastore; import akka.actor.Props; +import com.google.common.base.Stopwatch; +import java.util.concurrent.TimeUnit; import org.opendaylight.controller.cluster.common.actor.AbstractUntypedActor; import org.opendaylight.yangtools.yang.data.api.schema.tree.DataTreeCandidate; @@ -18,16 +20,39 @@ import org.opendaylight.yangtools.yang.data.api.schema.tree.DataTreeCandidate; * @author Thomas Pantelis */ public class ShardDataTreeNotificationPublisherActor extends AbstractUntypedActor { + private final Stopwatch timer = Stopwatch.createUnstarted(); + private final String name; + + private ShardDataTreeNotificationPublisherActor(String name) { + this.name = name; + } @Override protected void handleReceive(Object message) { if(message instanceof PublishNotifications) { - ((PublishNotifications)message).publish(); + PublishNotifications publisher = (PublishNotifications)message; + timer.start(); + + try { + publisher.publish(); + } finally { + long elapsedTime = timer.elapsed(TimeUnit.MILLISECONDS); + + if(elapsedTime >= ShardDataTreeNotificationPublisher.PUBLISH_DELAY_THRESHOLD_IN_MS) { + LOG.warn("{}: Generation of change events for {} took longer than expected. Elapsed time: {}", + publisher.logContext, name, timer); + } else { + LOG.debug("{}: Elapsed time for generation of change events for {}: {}", publisher.logContext, + name, timer); + } + + timer.reset(); + } } } - static Props props() { - return Props.create(ShardDataTreeNotificationPublisherActor.class); + static Props props(String notificationType) { + return Props.create(ShardDataTreeNotificationPublisherActor.class, notificationType); } static class PublishNotifications { -- 2.36.6