Repository: cayenne Updated Branches: refs/heads/master ea44dcad1 -> d0f462f54
CAY-2380 ReferenceMap should not store or return null values + minor serialization cleanup Project: http://git-wip-us.apache.org/repos/asf/cayenne/repo Commit: http://git-wip-us.apache.org/repos/asf/cayenne/commit/d0f462f5 Tree: http://git-wip-us.apache.org/repos/asf/cayenne/tree/d0f462f5 Diff: http://git-wip-us.apache.org/repos/asf/cayenne/diff/d0f462f5 Branch: refs/heads/master Commit: d0f462f54e387e54fb06a541fc71a2571366e146 Parents: ea44dca Author: Nikita Timofeev <[email protected]> Authored: Mon Nov 13 16:46:49 2017 +0300 Committer: Nikita Timofeev <[email protected]> Committed: Mon Nov 13 16:46:49 2017 +0300 ---------------------------------------------------------------------- .../org/apache/cayenne/util/ReferenceMap.java | 93 ++++++++++++++------ .../org/apache/cayenne/util/SoftValueMap.java | 17 +--- .../org/apache/cayenne/util/WeakValueMap.java | 14 +-- .../apache/cayenne/util/WeakValueMapTest.java | 54 ++++++++---- 4 files changed, 108 insertions(+), 70 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/cayenne/blob/d0f462f5/cayenne-server/src/main/java/org/apache/cayenne/util/ReferenceMap.java ---------------------------------------------------------------------- diff --git a/cayenne-server/src/main/java/org/apache/cayenne/util/ReferenceMap.java b/cayenne-server/src/main/java/org/apache/cayenne/util/ReferenceMap.java index b311309..99a91a4 100644 --- a/cayenne-server/src/main/java/org/apache/cayenne/util/ReferenceMap.java +++ b/cayenne-server/src/main/java/org/apache/cayenne/util/ReferenceMap.java @@ -33,6 +33,7 @@ import java.util.HashMap; import java.util.HashSet; import java.util.Iterator; import java.util.Map; +import java.util.NoSuchElementException; import java.util.Set; /** @@ -44,8 +45,7 @@ import java.util.Set; * <p> * This map doesn't guarantee that value will be there even right after put(), as GC can remove it at any time. * <p> - * This implementation supports proper serialization, concrete classes should use {@link #writeObjectInternal(ObjectOutputStream)} - * and {@link #readObjectInternal(ObjectInputStream)} methods to support it too. + * This implementation supports proper serialization. * <p> * * @param <K> key type @@ -63,8 +63,6 @@ abstract class ReferenceMap<K, V, R extends Reference<V>> extends AbstractMap<K, * Implementation notes: * - internally data stored in HashMap thus this class and all implementations are not thread safe; * - to track references that were cleared ReferenceQueue is used; - * - this map stores not only direct key => ref map but also a reverse ref => key to be able - * effectively clear data that was removed by GC; * - this map is abstract, all that required for the concrete implementation is * to define newReference(Object) method; * - all accessors/modifiers should call checkReferenceQueue() to clear all stale data @@ -75,7 +73,7 @@ abstract class ReferenceMap<K, V, R extends Reference<V>> extends AbstractMap<K, /** * This is a main data storage used for most operations */ - protected transient Map<K, R> map; + protected transient HashMap<K, R> map; protected transient ReferenceQueue<V> referenceQueue; @@ -121,12 +119,14 @@ abstract class ReferenceMap<K, V, R extends Reference<V>> extends AbstractMap<K, public boolean containsValue(Object value) { checkReferenceQueue(); for(R ref : map.values()) { - if(ref != null) { - V v = ref.get(); - if(v != null) { - if(v.equals(value)) { - return true; - } + if(ref == null) { + // should not happen, we can't have nulls in internal map + throw new IllegalStateException(); + } + V v = ref.get(); + if(v != null) { + if(v.equals(value)) { + return true; } } } @@ -145,6 +145,9 @@ abstract class ReferenceMap<K, V, R extends Reference<V>> extends AbstractMap<K, @Override public V put(K key, V value) { + if(value == null) { + throw new NullPointerException("ReferenceMap can't contain null values"); + } checkReferenceQueue(); R refValue = newReference(value); R oldValue = map.put(key, refValue); @@ -168,6 +171,9 @@ abstract class ReferenceMap<K, V, R extends Reference<V>> extends AbstractMap<K, public void putAll(Map<? extends K, ? extends V> m) { checkReferenceQueue(); for(Map.Entry<? extends K, ? extends V> entry : m.entrySet()) { + if(entry.getValue() == null) { + throw new NullPointerException("ReferenceMap can't contain null values"); + } R value = newReference(entry.getValue()); map.put(entry.getKey(), value); } @@ -182,6 +188,7 @@ abstract class ReferenceMap<K, V, R extends Reference<V>> extends AbstractMap<K, @Override public Set<K> keySet() { checkReferenceQueue(); + // should this check for cleared references? it can be invalid later anyway... return map.keySet(); } @@ -193,7 +200,11 @@ abstract class ReferenceMap<K, V, R extends Reference<V>> extends AbstractMap<K, Collection<V> values = new ArrayList<>(referenceValues.size()); for(R v : referenceValues) { if(v != null) { - values.add(v.get()); + V value = v.get(); + // check for null in case GC cleared some values after last queue check + if(value != null) { + values.add(value); + } } } return values; @@ -254,25 +265,20 @@ abstract class ReferenceMap<K, V, R extends Reference<V>> extends AbstractMap<K, abstract R newReference(V value); private void writeObject(ObjectOutputStream out) throws IOException { - writeObjectInternal(out); - } - - private void readObject(ObjectInputStream in) throws IOException, ClassNotFoundException { - readObjectInternal(in); - } - - protected void writeObjectInternal(ObjectOutputStream out) throws IOException { checkReferenceQueue(); - Map<K, V> replacementMap = new HashMap<>(); + Map<K, V> replacementMap = new HashMap<>(map.size()); for(Entry<K, R> entry : map.entrySet()) { if(entry.getValue() != null) { - replacementMap.put(entry.getKey(), entry.getValue().get()); + V value = entry.getValue().get(); + if(value != null) { + replacementMap.put(entry.getKey(), value); + } } } out.writeObject(replacementMap); } - protected void readObjectInternal(ObjectInputStream in) throws IOException, ClassNotFoundException { + private void readObject(ObjectInputStream in) throws IOException, ClassNotFoundException { @SuppressWarnings("unchecked") Map<K, V> replacement = (Map<K, V>) in.readObject(); map = new HashMap<>(replacement.size()); @@ -297,24 +303,53 @@ abstract class ReferenceMap<K, V, R extends Reference<V>> extends AbstractMap<K, } /** - * Iterator used by entrySet. Wrapper around {@link #map} iterator + * Iterator used by entrySet. Wrapper around {@link #map} iterator. + * It fetch ahead to be sure we have valid value, or otherwise we can return cleared reference. */ class ReferenceEntryIterator implements Iterator<Entry<K, V>> { Iterator<Entry<K, R>> internalIterator; + Entry<K, V> next; + ReferenceEntryIterator() { internalIterator = map.entrySet().iterator(); + tryAdvance(); } @Override public boolean hasNext() { - return internalIterator.hasNext(); + return next != null; } @Override public Entry<K, V> next() { - return new ReferenceEntry(internalIterator.next()); + if(!hasNext()) { + throw new NoSuchElementException(); + } + Entry<K, V> result = next; + tryAdvance(); + return result; + } + + /** + * Moves ahead internalIterator and tries to find first and store nonnull reference + */ + private void tryAdvance() { + next = null; + + while(internalIterator.hasNext()) { + Entry<K, R> nextRefEntry = internalIterator.next(); + if(nextRefEntry.getValue() == null) { + // should not happen, we can't have nulls in internal map + throw new IllegalStateException(); + } + V value = nextRefEntry.getValue().get(); + if(value != null) { + next = new ReferenceEntry(nextRefEntry, value); + break; + } + } } } @@ -327,8 +362,8 @@ abstract class ReferenceMap<K, V, R extends Reference<V>> extends AbstractMap<K, Entry<K, R> refEntry; - public ReferenceEntry(Entry<K, R> refEntry) { - super(refEntry.getKey(), refEntry.getValue() != null ? refEntry.getValue().get() : null); + public ReferenceEntry(Entry<K, R> refEntry, V value) { + super(refEntry.getKey(), value); this.refEntry = refEntry; } @@ -337,7 +372,7 @@ abstract class ReferenceMap<K, V, R extends Reference<V>> extends AbstractMap<K, R newRef = newReference(value); R oldRef = refEntry.setValue(newRef); if(oldRef != null) { - return oldRef.get(); + return getValue(); } return null; } http://git-wip-us.apache.org/repos/asf/cayenne/blob/d0f462f5/cayenne-server/src/main/java/org/apache/cayenne/util/SoftValueMap.java ---------------------------------------------------------------------- diff --git a/cayenne-server/src/main/java/org/apache/cayenne/util/SoftValueMap.java b/cayenne-server/src/main/java/org/apache/cayenne/util/SoftValueMap.java index 6955a56..cbfb642 100644 --- a/cayenne-server/src/main/java/org/apache/cayenne/util/SoftValueMap.java +++ b/cayenne-server/src/main/java/org/apache/cayenne/util/SoftValueMap.java @@ -19,16 +19,14 @@ package org.apache.cayenne.util; -import java.io.IOException; -import java.io.ObjectInputStream; -import java.io.ObjectOutputStream; import java.io.Serializable; import java.lang.ref.SoftReference; -import java.lang.ref.WeakReference; import java.util.Map; /** - * Map that stores values wrapped into {@link WeakReference} + * Map that stores values wrapped into {@link SoftReference} + * + * @see WeakValueMap * * @since 4.1 */ @@ -52,13 +50,4 @@ public class SoftValueMap<K, V> extends ReferenceMap<K, V, SoftReference<V>> imp SoftReference<V> newReference(V value) { return new SoftReference<>(value, referenceQueue); } - - private void writeObject(ObjectOutputStream out) throws IOException { - writeObjectInternal(out); - } - - @SuppressWarnings("unchecked") - private void readObject(ObjectInputStream in) throws IOException, ClassNotFoundException { - readObjectInternal(in); - } } http://git-wip-us.apache.org/repos/asf/cayenne/blob/d0f462f5/cayenne-server/src/main/java/org/apache/cayenne/util/WeakValueMap.java ---------------------------------------------------------------------- diff --git a/cayenne-server/src/main/java/org/apache/cayenne/util/WeakValueMap.java b/cayenne-server/src/main/java/org/apache/cayenne/util/WeakValueMap.java index 76a1610..ca0c58b 100644 --- a/cayenne-server/src/main/java/org/apache/cayenne/util/WeakValueMap.java +++ b/cayenne-server/src/main/java/org/apache/cayenne/util/WeakValueMap.java @@ -19,9 +19,6 @@ package org.apache.cayenne.util; -import java.io.IOException; -import java.io.ObjectInputStream; -import java.io.ObjectOutputStream; import java.io.Serializable; import java.lang.ref.WeakReference; import java.util.Map; @@ -29,6 +26,8 @@ import java.util.Map; /** * Map that stores values wrapped into {@link WeakReference} * + * @see SoftValueMap + * * @since 4.1 */ public class WeakValueMap<K, V> extends ReferenceMap<K, V, WeakReference<V>> implements Serializable { @@ -51,13 +50,4 @@ public class WeakValueMap<K, V> extends ReferenceMap<K, V, WeakReference<V>> imp WeakReference<V> newReference(V value) { return new WeakReference<>(value, referenceQueue); } - - private void writeObject(ObjectOutputStream out) throws IOException { - writeObjectInternal(out); - } - - @SuppressWarnings("unchecked") - private void readObject(ObjectInputStream in) throws IOException, ClassNotFoundException { - readObjectInternal(in); - } } http://git-wip-us.apache.org/repos/asf/cayenne/blob/d0f462f5/cayenne-server/src/test/java/org/apache/cayenne/util/WeakValueMapTest.java ---------------------------------------------------------------------- diff --git a/cayenne-server/src/test/java/org/apache/cayenne/util/WeakValueMapTest.java b/cayenne-server/src/test/java/org/apache/cayenne/util/WeakValueMapTest.java index e5a29f6..ace6579 100644 --- a/cayenne-server/src/test/java/org/apache/cayenne/util/WeakValueMapTest.java +++ b/cayenne-server/src/test/java/org/apache/cayenne/util/WeakValueMapTest.java @@ -19,6 +19,7 @@ package org.apache.cayenne.util; +import java.io.Serializable; import java.util.ConcurrentModificationException; import java.util.HashMap; import java.util.Iterator; @@ -73,7 +74,7 @@ public class WeakValueMapTest { Map<String, Integer> data = new HashMap<>(); data.put("key_1", 123); data.put("key_2", 42); - data.put("key_3", null); + data.put("key_3", 543); Map<String, Integer> map = new WeakValueMap<>(data); @@ -84,7 +85,7 @@ public class WeakValueMapTest { assertFalse(map.containsValue(321)); assertTrue(map.containsValue(42)); assertNull(map.get("nonexistent_key2")); - assertNull(map.get("key_3")); + assertEquals(Integer.valueOf(543), map.get("key_3")); assertEquals(new Integer(123), map.getOrDefault("key_1", 42)); assertEquals(data.size(), map.values().size()); @@ -101,7 +102,7 @@ public class WeakValueMapTest { Map<String, Integer> data = new HashMap<>(); data.put("key_1", 123); data.put("key_2", 42); - data.put("key_3", null); + data.put("key_3", 543); Map<String, Integer> map = new WeakValueMap<>(data); @@ -123,7 +124,7 @@ public class WeakValueMapTest { Map<String, Integer> map = new WeakValueMap<>(); map.put("key_1", 123); map.put("key_2", 42); - map.put("key_3", null); + map.put("key_3", 543); assertEquals(3, map.size()); int counter = 0; @@ -141,18 +142,20 @@ public class WeakValueMapTest { @Test public void testSerializationSupport() throws Exception { - WeakValueMap<String, Integer> map = new WeakValueMap<>(); + WeakValueMap<String, Object> map = new WeakValueMap<>(); map.put("key_1", 123); map.put("key_2", 42); - map.put("key_3", null); - assertEquals(3, map.size()); + map.put("key_3", 543); + map.put("key_4", new TestSerializable()); + assertEquals(4, map.size()); - WeakValueMap<String, Integer> clone = Util.cloneViaSerialization(map); + WeakValueMap<String, Object> clone = Util.cloneViaSerialization(map); - assertEquals(3, clone.size()); - assertEquals(new Integer(42), clone.get("key_2")); + assertEquals(4, clone.size()); + assertEquals(42, clone.get("key_2")); assertTrue(clone.containsKey("key_3")); assertTrue(clone.containsValue(123)); + assertTrue(clone.containsKey("key_4")); } @Test @@ -160,13 +163,13 @@ public class WeakValueMapTest { Map<String, Integer> map1 = new WeakValueMap<>(); map1.put("key_1", 123); map1.put("key_2", 42); - map1.put("key_3", null); + map1.put("key_3", 543); assertEquals(3, map1.size()); Map<String, Integer> map2 = new HashMap<>(); map2.put("key_1", 123); map2.put("key_2", 42); - map2.put("key_3", null); + map2.put("key_3", 543); assertEquals(map1, map2); assertEquals(map1.hashCode(), map2.hashCode()); @@ -177,8 +180,9 @@ public class WeakValueMapTest { Map<String, Integer> map = new WeakValueMap<>(3); map.put("key_1", 123); map.put("key_2", 42); - map.put("key_3", null); - assertEquals(3, map.size()); + map.put("key_3", 543); + map.put("key_4", 321); + assertEquals(4, map.size()); for(Map.Entry<String, Integer> entry : map.entrySet()) { if("key_2".equals(entry.getKey())) { @@ -192,7 +196,7 @@ public class WeakValueMapTest { Map<String, Integer> map = new WeakValueMap<>(3); map.put("key_1", 123); map.put("key_2", 42); - map.put("key_3", null); + map.put("key_3", 543); assertEquals(3, map.size()); Iterator<Map.Entry<String, Integer>> iterator = map.entrySet().iterator(); @@ -200,4 +204,24 @@ public class WeakValueMapTest { iterator.remove(); } } + + @Test(expected = NullPointerException.class) + public void testPutNullValue() { + Map<Object, Object> map = new WeakValueMap<>(); + map.put("1", null); + } + + @Test(expected = NullPointerException.class) + public void testPutAllNullValue() { + + Map<Object, Object> values = new HashMap<>(); + values.put("123", null); + + Map<Object, Object> map = new WeakValueMap<>(); + map.putAll(values); + } + + static class TestSerializable implements Serializable { + private static final long serialVersionUID = -8726479278547192134L; + } } \ No newline at end of file
