Merge pull request #750 from theigl/746-pool-max-capacity

#746 Ensure pool maximum capacity is honored

Author: magro

eclipse/code-format.xml

@@ -163,7 +163,7 @@
 <setting id="org.eclipse.jdt.core.compiler.codegen.inlineJsrBytecode" value="enabled"/>
 <setting id="org.eclipse.jdt.core.formatter.insert_space_before_closing_paren_in_switch" value="do not insert"/>
 <setting id="org.eclipse.jdt.core.formatter.alignment_for_method_declaration" value="0"/>
-<setting id="org.eclipse.jdt.core.formatter.join_wrapped_lines" value="true"/>
+<setting id="org.eclipse.jdt.core.formatter.join_wrapped_lines" value="false"/>
 <setting id="org.eclipse.jdt.core.formatter.insert_space_between_empty_parens_in_constructor_declaration" value="do not insert"/>
 <setting id="org.eclipse.jdt.core.formatter.wrap_before_conditional_operator" value="true"/>
 <setting id="org.eclipse.jdt.core.formatter.indent_switchstatements_compare_to_cases" value="true"/>

src/com/esotericsoftware/kryo/util/Pool.java

@@ -45,27 +45,34 @@ public Pool (boolean threadSafe, boolean softReferences) {
 	public Pool (boolean threadSafe, boolean softReferences, final int maximumCapacity) {
 		Queue<T> queue;
 		if (threadSafe)
-			queue = new LinkedBlockingQueue(maximumCapacity);
+			queue = new LinkedBlockingQueue<T>(maximumCapacity) {
+				@Override
+				public boolean add (T o) {
+					if (size() >= maximumCapacity) return false;
+					super.add(o);
+					return true;
+				}
+			};
 		else if (softReferences) {
-			queue = new LinkedList() { // More efficient clean() than ArrayDeque.
+			queue = new LinkedList<T>() { // More efficient clean() than ArrayDeque.
 				@Override
-				public boolean add (Object object) {
+				public boolean add (T object) {
 					if (size() >= maximumCapacity) return false;
 					super.add(object);
 					return true;
 				}
 			};
 		} else {
-			queue = new ArrayDeque() {
+			queue = new ArrayDeque<T>() {
 				@Override
-				public boolean add (Object object) {
+				public boolean offer (T object) {
 					if (size() >= maximumCapacity) return false;
-					super.add(object);
+					super.offer(object);
 					return true;
 				}
 			};
 		}
-		freeObjects = softReferences ? new SoftReferenceQueue(queue) : queue;
+		freeObjects = softReferences ? new SoftReferenceQueue<>(((Queue<SoftReference<T>>)queue)) : queue;
 	}
 
 	protected abstract T create ();
@@ -86,7 +93,7 @@ public void free (T object) {
 		if (object == null) throw new IllegalArgumentException("object cannot be null.");
 		reset(object);
 		if (!freeObjects.offer(object) && freeObjects instanceof SoftReferenceQueue) {
-			((SoftReferenceQueue)freeObjects).cleanOne();
+			((SoftReferenceQueue<T>)freeObjects).cleanOne();
 			freeObjects.offer(object);
 		}
 		peak = Math.max(peak, freeObjects.size());
@@ -108,7 +115,7 @@ public void clear () {
 	 * capacity. It is not necessary to call {@link #clean()} before calling {@link #free(Object)}, which will try to remove an
 	 * empty reference if the maximum capacity has been reached. */
 	public void clean () {
-		if (freeObjects instanceof SoftReferenceQueue) ((SoftReferenceQueue)freeObjects).clean();
+		if (freeObjects instanceof SoftReferenceQueue) ((SoftReferenceQueue<T>)freeObjects).clean();
 	}
 
 	/** The number of objects available to be obtained.
@@ -140,16 +147,16 @@ public static interface Poolable {
 	/** Wraps queue values with {@link SoftReference} for {@link Pool}.
 	 * @author Martin Grotzke */
 	static class SoftReferenceQueue<T> implements Queue<T> {
-		private Queue delegate;
+		private final Queue<SoftReference<T>> delegate;
 
-		public SoftReferenceQueue (Queue delegate) {
+		public SoftReferenceQueue (Queue<SoftReference<T>> delegate) {
 			this.delegate = delegate;
 		}
 
 		@Override
 		public T poll () {
 			while (true) {
-				SoftReference<T> reference = (SoftReference<T>)delegate.poll();
+				SoftReference<T> reference = delegate.poll();
 				if (reference == null) return null;
 				T object = reference.get();
 				if (object != null) return object;
@@ -158,7 +165,7 @@ public T poll () {
 
 		@Override
 		public boolean offer (T e) {
-			return delegate.add(new SoftReference(e));
+			return delegate.add(new SoftReference<>(e));
 		}
 
 		@Override
@@ -172,17 +179,16 @@ public void clear () {
 		}
 
 		void cleanOne () {
-			for (Iterator iter = delegate.iterator(); iter.hasNext();) {
-				if (((SoftReference)iter.next()).get() == null) {
+			for (Iterator<SoftReference<T>> iter = delegate.iterator(); iter.hasNext();) {
+				if (iter.next().get() == null) {
 					iter.remove();
 					break;
 				}
 			}
 		}
 
 		void clean () {
-			for (Iterator iter = delegate.iterator(); iter.hasNext();)
-				if (((SoftReference)iter.next()).get() == null) iter.remove();
+			delegate.removeIf(o -> o.get() == null);
 		}
 
 		@Override

test/com/esotericsoftware/kryo/util/PoolTest.java

@@ -26,6 +26,9 @@
 
 import java.util.Arrays;
 import java.util.Collection;
+import java.util.List;
+import java.util.stream.Collectors;
+import java.util.stream.IntStream;
 
 import org.junit.Before;
 import org.junit.Test;
@@ -35,19 +38,16 @@
 
 @RunWith(Parameterized.class)
 public class PoolTest extends KryoTestCase {
+
+	private static final int MAXIMUM_CAPACITY = 4;
+
 	@Parameters
 	public static Collection<Object[]> data () {
-		return Arrays.asList(new Object[][] {{new Pool<Kryo>(true, false, 16) {
-			@Override
-			protected Kryo create () {
-				return new Kryo();
-			}
-		}}, {new Pool<Kryo>(true, true, 16) {
-			@Override
-			protected Kryo create () {
-				return new Kryo();
-			}
-		}}});
+		return Arrays.asList(new Object[][] {
+			{new TestPool(false, false, MAXIMUM_CAPACITY)},
+			{new TestPool(true, false, MAXIMUM_CAPACITY)},
+			{new TestPool(false, true, MAXIMUM_CAPACITY)},
+			{new TestPool(true, true, MAXIMUM_CAPACITY)}});
 	}
 
 	private final Pool<Kryo> pool;
@@ -63,20 +63,31 @@ public void beforeMethod () {
 	}
 
 	@Test
-	public void getShouldReturnAvailableInstance () {
+	public void obtainShouldReturnAvailableInstance () {
 		Kryo kryo = pool.obtain();
 		pool.free(kryo);
 		assertSame(kryo, pool.obtain());
 	}
 
 	@Test
-	public void releaseShouldAddKryoToPool () {
+	public void freeShouldAddKryoToPool () {
 		assertEquals(0, pool.getFree());
 		Kryo kryo = pool.obtain();
 		pool.free(kryo);
 		assertEquals(1, pool.getFree());
 	}
 
+	@Test
+	public void freeShouldNotAddMoreThanMaximumCapacityToKryoPool () {
+		final List<Kryo> kryos = IntStream.rangeClosed(0, MAXIMUM_CAPACITY + 1)
+			.mapToObj(i -> pool.obtain())
+			.collect(Collectors.toList());
+		for (Kryo kryo : kryos) {
+			pool.free(kryo);
+		}
+		assertEquals(MAXIMUM_CAPACITY, pool.getFree());
+	}
+
 	@Test
 	public void testSize () {
 		assertEquals(0, pool.getFree());
@@ -89,4 +100,16 @@ public void testSize () {
 		pool.free(kryo2);
 		assertEquals(2, pool.getFree());
 	}
+
+	private static class TestPool extends Pool<Kryo> {
+
+		public TestPool (boolean threadSafe, boolean softReferences, int maximumCapacity) {
+			super(threadSafe, softReferences, maximumCapacity);
+		}
+
+		@Override
+		protected Kryo create () {
+			return new Kryo();
+		}
+	}
 }