You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cayenne.apache.org by nt...@apache.org on 2017/11/13 13:47:31 UTC
cayenne git commit: CAY-2380 ReferenceMap should not store or return
null values + minor serialization cleanup
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 <st...@gmail.com>
Authored: Mon Nov 13 16:46:49 2017 +0300
Committer: Nikita Timofeev <st...@gmail.com>
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