From d13f3b88549adc8ccdea1269da3053c79b577d6f Mon Sep 17 00:00:00 2001 From: Madan Jampani Date: Wed, 1 Jul 2015 17:37:50 -0700 Subject: [PATCH] ONOS-2280: Fix NPE in hosts EC map Change-Id: I4cb74d7c9526dc0e836e1e2790748324f60183f5 --- .../ecmap/EventuallyConsistentMapImpl.java | 64 +++++++++++-------- .../store/host/impl/ECHostStore.java | 15 ++--- .../EventuallyConsistentMapImplTest.java | 2 - 3 files changed, 44 insertions(+), 37 deletions(-) diff --git a/core/store/dist/src/main/java/org/onosproject/store/ecmap/EventuallyConsistentMapImpl.java b/core/store/dist/src/main/java/org/onosproject/store/ecmap/EventuallyConsistentMapImpl.java index 6d8daaa31b..c20016b7d1 100644 --- a/core/store/dist/src/main/java/org/onosproject/store/ecmap/EventuallyConsistentMapImpl.java +++ b/core/store/dist/src/main/java/org/onosproject/store/ecmap/EventuallyConsistentMapImpl.java @@ -47,6 +47,7 @@ import java.util.Collection; import java.util.Collections; import java.util.List; import java.util.Map; +import java.util.Objects; import java.util.Optional; import java.util.Set; import java.util.Timer; @@ -312,7 +313,9 @@ public class EventuallyConsistentMapImpl public V remove(K key) { checkState(!destroyed, destroyedMessage); checkNotNull(key, ERROR_NULL_KEY); - return removeInternal(key, Optional.empty()); + MapValue tombstone = new MapValue<>(null, timestampProvider.apply(key, null)); + MapValue previousValue = removeInternal(key, Optional.empty(), tombstone); + return previousValue != null ? previousValue.get() : null; } @Override @@ -320,34 +323,39 @@ public class EventuallyConsistentMapImpl checkState(!destroyed, destroyedMessage); checkNotNull(key, ERROR_NULL_KEY); checkNotNull(value, ERROR_NULL_VALUE); - removeInternal(key, Optional.of(value)); + MapValue tombstone = new MapValue<>(null, timestampProvider.apply(key, null)); + removeInternal(key, Optional.of(value), tombstone); } - private V removeInternal(K key, Optional value) { + private MapValue removeInternal(K key, Optional value, MapValue tombstone) { checkState(!destroyed, destroyedMessage); checkNotNull(key, ERROR_NULL_KEY); checkNotNull(value, ERROR_NULL_VALUE); - MapValue newValue = new MapValue<>(null, timestampProvider.apply(key, value.orElse(null))); + checkState(tombstone.isTombstone()); AtomicBoolean updated = new AtomicBoolean(false); - AtomicReference previousValue = new AtomicReference<>(); + AtomicReference> previousValue = new AtomicReference<>(); items.compute(key, (k, existing) -> { - if (existing != null && existing.isAlive()) { - updated.set(!value.isPresent() || value.get().equals(existing.get())); - previousValue.set(existing.get()); + boolean valueMatches = true; + if (value.isPresent() && existing != null && existing.isAlive()) { + valueMatches = Objects.equals(value.get(), existing.get()); } - updated.set(existing == null || newValue.isNewerThan(existing)); - return updated.get() ? newValue : existing; + updated.set(valueMatches && (existing == null || tombstone.isNewerThan(existing))); + if (updated.get()) { + previousValue.set(existing); + } + return updated.get() ? tombstone : existing; }); if (updated.get()) { - notifyPeers(new UpdateEntry<>(key, newValue), peerUpdateFunction.apply(key, previousValue.get())); - notifyListeners(new EventuallyConsistentMapEvent<>(REMOVE, key, previousValue.get())); - if (persistent) { - persistentStore.update(key, newValue); + notifyPeers(new UpdateEntry<>(key, tombstone), peerUpdateFunction.apply(key, null)); + if (previousValue.get() != null && previousValue.get().isAlive()) { + notifyListeners(new EventuallyConsistentMapEvent<>(REMOVE, key, previousValue.get().get())); + } + if (persistent) { + persistentStore.update(key, tombstone); } - return previousValue.get(); } - return null; + return previousValue.get(); } @Override @@ -540,12 +548,13 @@ public class EventuallyConsistentMapImpl if (remoteValueDigest == null || localValue.isNewerThan(remoteValueDigest.timestamp())) { // local value is more recent, push to sender queueUpdate(new UpdateEntry<>(key, localValue), ImmutableList.of(sender)); - } else { - if (remoteValueDigest.isTombstone() - && remoteValueDigest.timestamp().isNewerThan(localValue.timestamp())) { - if (updateInternal(key, new MapValue<>(null, remoteValueDigest.timestamp()))) { - externalEvents.add(new EventuallyConsistentMapEvent<>(REMOVE, key, null)); - } + } + if (remoteValueDigest != null && remoteValueDigest.isTombstone()) { + MapValue previousValue = removeInternal(key, + Optional.empty(), + new MapValue<>(null, remoteValueDigest.timestamp())); + if (previousValue != null && previousValue.isAlive()) { + externalEvents.add(new EventuallyConsistentMapEvent<>(REMOVE, key, previousValue.get())); } } }); @@ -559,10 +568,13 @@ public class EventuallyConsistentMapImpl updates.forEach(update -> { final K key = update.key(); final MapValue value = update.value(); - - if (updateInternal(key, value)) { - final EventuallyConsistentMapEvent.Type type = value.isTombstone() ? REMOVE : PUT; - notifyListeners(new EventuallyConsistentMapEvent<>(type, key, value.get())); + if (value.isTombstone()) { + MapValue previousValue = removeInternal(key, Optional.empty(), value); + if (previousValue != null && previousValue.get() != null) { + notifyListeners(new EventuallyConsistentMapEvent<>(REMOVE, key, previousValue.get())); + } + } else if (updateInternal(key, value)) { + notifyListeners(new EventuallyConsistentMapEvent<>(PUT, key, value.get())); } }); } diff --git a/core/store/dist/src/main/java/org/onosproject/store/host/impl/ECHostStore.java b/core/store/dist/src/main/java/org/onosproject/store/host/impl/ECHostStore.java index 637444c5eb..f4071ef0e3 100644 --- a/core/store/dist/src/main/java/org/onosproject/store/host/impl/ECHostStore.java +++ b/core/store/dist/src/main/java/org/onosproject/store/host/impl/ECHostStore.java @@ -1,5 +1,6 @@ package org.onosproject.store.host.impl; +import static com.google.common.base.Preconditions.checkNotNull; import static org.onosproject.net.DefaultAnnotations.merge; import static org.onosproject.net.host.HostEvent.Type.HOST_ADDED; import static org.onosproject.net.host.HostEvent.Type.HOST_REMOVED; @@ -11,7 +12,6 @@ import java.util.Collection; import java.util.Collections; import java.util.Objects; import java.util.Set; -import java.util.concurrent.ConcurrentHashMap; import java.util.function.Predicate; import java.util.stream.Collectors; @@ -50,13 +50,9 @@ import org.slf4j.Logger; import com.google.common.collect.HashMultimap; import com.google.common.collect.ImmutableSet; -import com.google.common.collect.Multimap; import com.google.common.collect.Multimaps; import com.google.common.collect.SetMultimap; import com.google.common.collect.Sets; -import static com.google.common.collect.Multimaps.newSetMultimap; -import static com.google.common.collect.Multimaps.synchronizedSetMultimap; -import static com.google.common.collect.Sets.newConcurrentHashSet; /** * Manages the inventory of hosts using a {@code EventuallyConsistentMap}. @@ -76,9 +72,10 @@ public class ECHostStore protected LogicalClockService clockService; // Hosts tracked by their location - private final Multimap locations - = synchronizedSetMultimap(newSetMultimap(new ConcurrentHashMap<>(), - () -> newConcurrentHashSet())); + private final SetMultimap locations = + Multimaps.synchronizedSetMultimap( + HashMultimap.create()); + private final SetMultimap portAddresses = Multimaps.synchronizedSetMultimap( HashMultimap.create()); @@ -252,7 +249,7 @@ public class ECHostStore @Override public void event(EventuallyConsistentMapEvent event) { - DefaultHost host = event.value(); + DefaultHost host = checkNotNull(event.value()); if (event.type() == PUT) { locations.put(host.location(), host); } else if (event.type() == REMOVE) { diff --git a/core/store/dist/src/test/java/org/onosproject/store/ecmap/EventuallyConsistentMapImplTest.java b/core/store/dist/src/test/java/org/onosproject/store/ecmap/EventuallyConsistentMapImplTest.java index 57943ad88f..9a656304e2 100644 --- a/core/store/dist/src/test/java/org/onosproject/store/ecmap/EventuallyConsistentMapImplTest.java +++ b/core/store/dist/src/test/java/org/onosproject/store/ecmap/EventuallyConsistentMapImplTest.java @@ -326,8 +326,6 @@ public class EventuallyConsistentMapImplTest { = getListener(); listener.event(new EventuallyConsistentMapEvent<>( EventuallyConsistentMapEvent.Type.REMOVE, KEY1, VALUE1)); - listener.event(new EventuallyConsistentMapEvent<>( - EventuallyConsistentMapEvent.Type.REMOVE, KEY1, null)); listener.event(new EventuallyConsistentMapEvent<>( EventuallyConsistentMapEvent.Type.PUT, KEY1, VALUE1)); listener.event(new EventuallyConsistentMapEvent<>(