From 527dd18200f490be189fe6c0ec43fad8198da127 Mon Sep 17 00:00:00 2001 From: Tomas Cere Date: Fri, 11 Dec 2015 17:44:20 +0100 Subject: [PATCH] Use lock in topology node writer Prevents race conditions from akka callbacks. Prevent NPE's when deleting node that's not connected. Change-Id: I0e46083fe98c88cae7dc836c98f9eb4e22026579 Signed-off-by: Tomas Cere --- .../topology/util/NodeRoleChangeStrategy.java | 12 ++- .../topology/impl/TopologyNodeWriter.java | 87 +++++++++++-------- 2 files changed, 61 insertions(+), 38 deletions(-) diff --git a/opendaylight/netconf/abstract-topology/src/main/java/org/opendaylight/netconf/topology/util/NodeRoleChangeStrategy.java b/opendaylight/netconf/abstract-topology/src/main/java/org/opendaylight/netconf/topology/util/NodeRoleChangeStrategy.java index 3cafd35b55..4783404b54 100644 --- a/opendaylight/netconf/abstract-topology/src/main/java/org/opendaylight/netconf/topology/util/NodeRoleChangeStrategy.java +++ b/opendaylight/netconf/abstract-topology/src/main/java/org/opendaylight/netconf/topology/util/NodeRoleChangeStrategy.java @@ -61,10 +61,14 @@ public class NodeRoleChangeStrategy implements RoleChangeStrategy, EntityOwnersh @Override public void unregisterRoleCandidate() { LOG.debug("Unregistering role candidate"); - candidateRegistration.close(); - candidateRegistration = null; - ownershipListenerRegistration.close(); - ownershipListenerRegistration = null; + if (candidateRegistration != null) { + candidateRegistration.close(); + candidateRegistration = null; + } + if (ownershipListenerRegistration != null) { + ownershipListenerRegistration.close(); + ownershipListenerRegistration = null; + } } @Override diff --git a/opendaylight/netconf/netconf-topology/src/main/java/org/opendaylight/netconf/topology/impl/TopologyNodeWriter.java b/opendaylight/netconf/netconf-topology/src/main/java/org/opendaylight/netconf/topology/impl/TopologyNodeWriter.java index 563ce977b2..54ef71f2b6 100644 --- a/opendaylight/netconf/netconf-topology/src/main/java/org/opendaylight/netconf/topology/impl/TopologyNodeWriter.java +++ b/opendaylight/netconf/netconf-topology/src/main/java/org/opendaylight/netconf/topology/impl/TopologyNodeWriter.java @@ -12,6 +12,7 @@ import com.google.common.base.Preconditions; import com.google.common.util.concurrent.CheckedFuture; import com.google.common.util.concurrent.FutureCallback; import com.google.common.util.concurrent.Futures; +import java.util.concurrent.locks.ReentrantLock; import javax.annotation.Nonnull; import org.opendaylight.controller.md.sal.binding.api.BindingTransactionChain; import org.opendaylight.controller.md.sal.binding.api.DataBroker; @@ -48,6 +49,8 @@ public class TopologyNodeWriter implements NodeWriter{ private final InstanceIdentifier networkTopologyPath; private final KeyedInstanceIdentifier topologyListPath; + private final ReentrantLock lock = new ReentrantLock(true); + public TopologyNodeWriter(final String topologyId, final DataBroker dataBroker) { this.topologyId = topologyId; this.txChain = Preconditions.checkNotNull(dataBroker).createTransactionChain(new TransactionChainListener() { @@ -70,49 +73,65 @@ public class TopologyNodeWriter implements NodeWriter{ @Override public void init(@Nonnull NodeId id, @Nonnull Node operationalDataNode) { - final WriteTransaction writeTx = txChain.newWriteOnlyTransaction(); - - createNetworkTopologyIfNotPresent(writeTx); - final InstanceIdentifier path = createBindingPathForTopology(new NodeKey(id), topologyId); - - LOG.trace("{}: Init device state transaction {} putting if absent operational data started. Putting data on path {}", - id.getValue(), writeTx.getIdentifier(), path); - writeTx.put(LogicalDatastoreType.OPERATIONAL, path, operationalDataNode); - LOG.trace("{}: Init device state transaction {} putting operational data ended.", - id.getValue(), writeTx.getIdentifier()); - - commitTransaction(writeTx, "init", id); + lock.lock(); + try { + final WriteTransaction writeTx = txChain.newWriteOnlyTransaction(); + + createNetworkTopologyIfNotPresent(writeTx); + final InstanceIdentifier path = createBindingPathForTopology(new NodeKey(id), topologyId); + + LOG.trace("{}: Init device state transaction {} putting if absent operational data started. Putting data on path {}", + id.getValue(), writeTx.getIdentifier(), path); + writeTx.put(LogicalDatastoreType.OPERATIONAL, path, operationalDataNode); + LOG.trace("{}: Init device state transaction {} putting operational data ended.", + id.getValue(), writeTx.getIdentifier()); + + commitTransaction(writeTx, "init", id); + } finally { + lock.unlock(); + } } @Override public void update(@Nonnull NodeId id, @Nonnull Node operationalDataNode) { - final WriteTransaction writeTx = txChain.newWriteOnlyTransaction(); + lock.lock(); + try { + final WriteTransaction writeTx = txChain.newWriteOnlyTransaction(); + + final InstanceIdentifier path = createBindingPathForTopology(new NodeKey(id), topologyId); + LOG.trace("{}: Update device state transaction {} merging operational data started. Putting data on path {}", + id, writeTx.getIdentifier(), operationalDataNode); + writeTx.put(LogicalDatastoreType.OPERATIONAL, path, operationalDataNode); + LOG.trace("{}: Update device state transaction {} merging operational data ended.", + id, writeTx.getIdentifier()); + + commitTransaction(writeTx, "update", id); + } finally { + lock.unlock(); + } - final InstanceIdentifier path = createBindingPathForTopology(new NodeKey(id), topologyId); - LOG.trace("{}: Update device state transaction {} merging operational data started. Putting data on path {}", - id, writeTx.getIdentifier(), operationalDataNode); - writeTx.put(LogicalDatastoreType.OPERATIONAL, path, operationalDataNode); - LOG.trace("{}: Update device state transaction {} merging operational data ended.", - id, writeTx.getIdentifier()); - - commitTransaction(writeTx, "update", id); } @Override public void delete(@Nonnull NodeId id) { - final WriteTransaction writeTx = txChain.newWriteOnlyTransaction(); - - final InstanceIdentifier path = createBindingPathForTopology(new NodeKey(id), topologyId); - - LOG.trace( - "{}: Close device state transaction {} removing all data started. Path: {}", - id, writeTx.getIdentifier(), path); - writeTx.delete(LogicalDatastoreType.OPERATIONAL, path); - LOG.trace( - "{}: Close device state transaction {} removing all data ended.", - id, writeTx.getIdentifier()); - - commitTransaction(writeTx, "close", id); + lock.lock(); + try { + final WriteTransaction writeTx = txChain.newWriteOnlyTransaction(); + + final InstanceIdentifier path = createBindingPathForTopology(new NodeKey(id), topologyId); + + LOG.trace( + "{}: Close device state transaction {} removing all data started. Path: {}", + id, writeTx.getIdentifier(), path); + writeTx.delete(LogicalDatastoreType.OPERATIONAL, path); + LOG.trace( + "{}: Close device state transaction {} removing all data ended.", + id, writeTx.getIdentifier()); + + commitTransaction(writeTx, "close", id); + } finally { + lock.unlock(); + } } private void commitTransaction(final WriteTransaction transaction, final String txType, final NodeId id) { -- 2.36.6