cayenne-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From ntimof...@apache.org
Subject cayenne git commit: CAY-2380 ReferenceMap should not store or return null values + minor serialization cleanup
Date Mon, 13 Nov 2017 13:47:31 GMT
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 <stariy95@gmail.com>
Authored: Mon Nov 13 16:46:49 2017 +0300
Committer: Nikita Timofeev <stariy95@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


Mime
View raw message