You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@commons.apache.org by gg...@apache.org on 2023/02/28 20:35:00 UTC
[commons-pool] branch master updated: [POOL-407] Provide some NPE-proofing for factories that misbehave by returning nulls instead of throwing exceptions
This is an automated email from the ASF dual-hosted git repository.
ggregory pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/commons-pool.git
The following commit(s) were added to refs/heads/master by this push:
new 38e78ee0 [POOL-407] Provide some NPE-proofing for factories that misbehave by returning nulls instead of throwing exceptions
38e78ee0 is described below
commit 38e78ee0a5cbf86efb19c1c3eabcef069f1f12e2
Author: Gary Gregory <ga...@gmail.com>
AuthorDate: Tue Feb 28 15:34:49 2023 -0500
[POOL-407] Provide some NPE-proofing for factories that misbehave by
returning nulls instead of throwing exceptions
---
pom.xml | 3 +-
.../org/apache/commons/pool2/PooledObject.java | 14 +++-
.../commons/pool2/impl/GenericKeyedObjectPool.java | 26 ++++---
.../commons/pool2/impl/GenericObjectPool.java | 22 +++---
...ctory.java => AbstractKeyedPool407Factory.java} | 22 ++----
...ectFactory.java => AbstractPool407Factory.java} | 32 ++++----
.../commons/pool2/pool407/AbstractPool407Test.java | 56 ++++++++++++++
.../apache/commons/pool2/pool407/KeyedPool407.java | 7 +-
.../commons/pool2/pool407/KeyedPool407Config.java | 8 +-
.../pool2/pool407/KeyedPool407NormalFactory.java | 14 ++--
.../pool407/KeyedPool407NullObjectFactory.java | 14 ++--
.../KeyedPool407NullPoolableObjectFactory.java | 14 ++--
.../commons/pool2/pool407/KeyedPool407Test.java | 86 ++++++++++++++--------
.../org/apache/commons/pool2/pool407/Pool407.java | 13 ++--
.../commons/pool2/pool407/Pool407Config.java | 11 ++-
...yedPool407Config.java => Pool407Constants.java} | 19 +++--
.../pool2/pool407/Pool407NormalFactory.java | 22 ++++--
.../pool2/pool407/Pool407NullObjectFactory.java | 20 +++--
.../pool407/Pool407NullPoolableObjectFactory.java | 21 ++++--
.../apache/commons/pool2/pool407/Pool407Test.java | 81 ++++++++++++--------
20 files changed, 326 insertions(+), 179 deletions(-)
diff --git a/pom.xml b/pom.xml
index 8a506c4f..e25ea42d 100644
--- a/pom.xml
+++ b/pom.xml
@@ -277,10 +277,9 @@
<forkedProcessTimeoutInSeconds>1800</forkedProcessTimeoutInSeconds>
<includes>
<include>**/Test*.java</include>
+ <include>**/*Test.java</include>
</includes>
<excludes>
- <!-- nested classes are not handled properly by Surefire -->
- <exclude>**/Test*$*.java</exclude>
<!-- Don't run this test by default - it uses lots of memory -->
<exclude>**/TestSoftRefOutOfMemory.java</exclude>
</excludes>
diff --git a/src/main/java/org/apache/commons/pool2/PooledObject.java b/src/main/java/org/apache/commons/pool2/PooledObject.java
index ac1d4b36..fffcbc5b 100644
--- a/src/main/java/org/apache/commons/pool2/PooledObject.java
+++ b/src/main/java/org/apache/commons/pool2/PooledObject.java
@@ -28,12 +28,22 @@ import java.util.Deque;
* Implementations of this class are required to be thread-safe.
* </p>
*
- * @param <T> the type of object in the pool
- *
+ * @param <T> the type of object in the pool.
* @since 2.0
*/
public interface PooledObject<T> extends Comparable<PooledObject<T>> {
+ /**
+ * Tests whether the given PooledObject is null <em>or</em> contains a null.
+ *
+ * @param pooledObject the PooledObject to test.
+ * @return whether the given PooledObject is null <em>or</em> contains a null.
+ * @since 2.12.0
+ */
+ static boolean isNull(PooledObject<?> pooledObject) {
+ return pooledObject == null || pooledObject.getObject() == null;
+ }
+
/**
* Allocates the object.
*
diff --git a/src/main/java/org/apache/commons/pool2/impl/GenericKeyedObjectPool.java b/src/main/java/org/apache/commons/pool2/impl/GenericKeyedObjectPool.java
index 3efff896..cddda909 100644
--- a/src/main/java/org/apache/commons/pool2/impl/GenericKeyedObjectPool.java
+++ b/src/main/java/org/apache/commons/pool2/impl/GenericKeyedObjectPool.java
@@ -301,7 +301,7 @@ public class GenericKeyedObjectPool<K, T, E extends Exception> extends BaseGener
* @throws E If the associated factory fails to passivate the object
*/
private void addIdleObject(final K key, final PooledObject<T> p) throws E {
- if (p != null) {
+ if (!PooledObject.isNull(p)) {
factory.passivateObject(key, p);
final LinkedBlockingDeque<PooledObject<T>> idleObjects = poolMap.get(key).getIdleObjects();
if (getLifo()) {
@@ -434,12 +434,12 @@ public class GenericKeyedObjectPool<K, T, E extends Exception> extends BaseGener
p = objectDeque.getIdleObjects().pollFirst();
if (p == null) {
p = create(key);
- if (p != null) {
+ if (!PooledObject.isNull(p)) {
create = true;
}
}
if (blockWhenExhausted) {
- if (p == null) {
+ if (PooledObject.isNull(p)) {
try {
p = borrowMaxWaitMillis < 0 ? objectDeque.getIdleObjects().takeFirst() :
objectDeque.getIdleObjects().pollFirst(borrowMaxWaitMillis, TimeUnit.MILLISECONDS);
@@ -447,18 +447,18 @@ public class GenericKeyedObjectPool<K, T, E extends Exception> extends BaseGener
throw cast(e);
}
}
- if (p == null) {
+ if (PooledObject.isNull(p)) {
throw new NoSuchElementException(appendStats(
"Timeout waiting for idle object, borrowMaxWaitMillis=" + borrowMaxWaitMillis));
}
- } else if (p == null) {
+ } else if (PooledObject.isNull(p)) {
throw new NoSuchElementException(appendStats("Pool exhausted"));
}
if (!p.allocate()) {
p = null;
}
- if (p != null) {
+ if (!PooledObject.isNull(p)) {
try {
factory.activateObject(key, p);
} catch (final Exception e) {
@@ -474,7 +474,7 @@ public class GenericKeyedObjectPool<K, T, E extends Exception> extends BaseGener
throw nsee;
}
}
- if (p != null && getTestOnBorrow()) {
+ if (!PooledObject.isNull(p) && getTestOnBorrow()) {
boolean validate = false;
Throwable validationThrowable = null;
try {
@@ -792,6 +792,11 @@ public class GenericKeyedObjectPool<K, T, E extends Exception> extends BaseGener
PooledObject<T> p = null;
try {
p = factory.makeObject(key);
+ if (PooledObject.isNull(p)) {
+ numTotal.decrementAndGet();
+ objectDeque.getCreateCount().decrementAndGet();
+ throw new NullPointerException(String.format("%s.makeObject() = null", factory.getClass().getSimpleName()));
+ }
if (getTestOnCreate() && !factory.validateObject(key, p)) {
numTotal.decrementAndGet();
objectDeque.getCreateCount().decrementAndGet();
@@ -1465,7 +1470,7 @@ public class GenericKeyedObjectPool<K, T, E extends Exception> extends BaseGener
final PooledObject<T> p = objectDeque.getAllObjects().get(new IdentityWrapper<>(obj));
- if (p == null) {
+ if (PooledObject.isNull(p)) {
throw new IllegalStateException("Returned object not currently part of this pool");
}
@@ -1568,10 +1573,7 @@ public class GenericKeyedObjectPool<K, T, E extends Exception> extends BaseGener
if (mostLoaded != null) {
register(loadedKey);
try {
- final PooledObject<T> p = create(loadedKey);
- if (p != null) {
- addIdleObject(loadedKey, p);
- }
+ addIdleObject(loadedKey, create(loadedKey));
} catch (final Exception e) {
swallowException(e);
} finally {
diff --git a/src/main/java/org/apache/commons/pool2/impl/GenericObjectPool.java b/src/main/java/org/apache/commons/pool2/impl/GenericObjectPool.java
index 5ce1ee57..f6349c16 100644
--- a/src/main/java/org/apache/commons/pool2/impl/GenericObjectPool.java
+++ b/src/main/java/org/apache/commons/pool2/impl/GenericObjectPool.java
@@ -185,7 +185,7 @@ public class GenericObjectPool<T, E extends Exception> extends BaseGenericObject
* @throws E If the factory fails to passivate the object
*/
private void addIdleObject(final PooledObject<T> p) throws E {
- if (p != null) {
+ if (!PooledObject.isNull(p)) {
factory.passivateObject(p);
if (getLifo()) {
idleObjects.addFirst(p);
@@ -292,12 +292,12 @@ public class GenericObjectPool<T, E extends Exception> extends BaseGenericObject
p = idleObjects.pollFirst();
if (p == null) {
p = create();
- if (p != null) {
+ if (!PooledObject.isNull(p)) {
create = true;
}
}
if (blockWhenExhausted) {
- if (p == null) {
+ if (PooledObject.isNull(p)) {
try {
p = borrowMaxWaitDuration.isNegative() ? idleObjects.takeFirst() : idleObjects.pollFirst(borrowMaxWaitDuration);
} catch (final InterruptedException e) {
@@ -305,18 +305,18 @@ public class GenericObjectPool<T, E extends Exception> extends BaseGenericObject
throw cast(e);
}
}
- if (p == null) {
+ if (PooledObject.isNull(p)) {
throw new NoSuchElementException(appendStats(
"Timeout waiting for idle object, borrowMaxWaitDuration=" + borrowMaxWaitDuration));
}
- } else if (p == null) {
+ } else if (PooledObject.isNull(p)) {
throw new NoSuchElementException(appendStats("Pool exhausted"));
}
if (!p.allocate()) {
p = null;
}
- if (p != null) {
+ if (!PooledObject.isNull(p)) {
try {
factory.activateObject(p);
} catch (final Exception e) {
@@ -333,7 +333,7 @@ public class GenericObjectPool<T, E extends Exception> extends BaseGenericObject
throw nsee;
}
}
- if (p != null && getTestOnBorrow()) {
+ if (!PooledObject.isNull(p) && getTestOnBorrow()) {
boolean validate = false;
Throwable validationThrowable = null;
try {
@@ -493,7 +493,7 @@ public class GenericObjectPool<T, E extends Exception> extends BaseGenericObject
* returns null.
* </p>
*
- * @return The new wrapped pooled object
+ * @return The new wrapped pooled object or null.
* @throws E if the object factory's {@code makeObject} fails
*/
private PooledObject<T> create() throws E {
@@ -558,6 +558,10 @@ public class GenericObjectPool<T, E extends Exception> extends BaseGenericObject
final PooledObject<T> p;
try {
p = factory.makeObject();
+ if (PooledObject.isNull(p)) {
+ createCount.decrementAndGet();
+ throw new NullPointerException(String.format("%s.makeObject() = null", factory.getClass().getSimpleName()));
+ }
if (getTestOnCreate() && !factory.validateObject(p)) {
createCount.decrementAndGet();
return null;
@@ -624,7 +628,7 @@ public class GenericObjectPool<T, E extends Exception> extends BaseGenericObject
while (idleObjects.size() < idleCount) {
final PooledObject<T> p = create();
- if (p == null) {
+ if (PooledObject.isNull(p)) {
// Can't create objects, no reason to think another call to
// create will work. Give up.
break;
diff --git a/src/test/java/org/apache/commons/pool2/pool407/KeyedPool407NullPoolableObjectFactory.java b/src/test/java/org/apache/commons/pool2/pool407/AbstractKeyedPool407Factory.java
similarity index 65%
copy from src/test/java/org/apache/commons/pool2/pool407/KeyedPool407NullPoolableObjectFactory.java
copy to src/test/java/org/apache/commons/pool2/pool407/AbstractKeyedPool407Factory.java
index 1896a18f..064862d1 100644
--- a/src/test/java/org/apache/commons/pool2/pool407/KeyedPool407NullPoolableObjectFactory.java
+++ b/src/test/java/org/apache/commons/pool2/pool407/AbstractKeyedPool407Factory.java
@@ -20,25 +20,17 @@ package org.apache.commons.pool2.pool407;
import org.apache.commons.pool2.BaseKeyedPooledObjectFactory;
import org.apache.commons.pool2.PooledObject;
-public final class KeyedPool407NullPoolableObjectFactory extends BaseKeyedPooledObjectFactory<String, KeyedPool407Fixture, RuntimeException> {
-
- @Override
- public KeyedPool407Fixture create(final String key) {
- return null;
- }
+/**
+ * Tests POOL-407.
+ */
+public abstract class AbstractKeyedPool407Factory extends BaseKeyedPooledObjectFactory<String, KeyedPool407Fixture, RuntimeException> {
- @Override
- public PooledObject<KeyedPool407Fixture> makeObject(final String key) throws RuntimeException {
- return null;
- }
+ abstract boolean isDefaultMakeObject();
@Override
public boolean validateObject(final String key, final PooledObject<KeyedPool407Fixture> p) {
- return p != null && p.getObject() != null;
+ // TODO Should this be enough even if wrap() does throw and returns a DefaultPooledObject wrapping a null?
+ return !PooledObject.isNull(p);
}
- @Override
- public PooledObject<KeyedPool407Fixture> wrap(final KeyedPool407Fixture value) {
- return null;
- }
}
diff --git a/src/test/java/org/apache/commons/pool2/pool407/Pool407NullObjectFactory.java b/src/test/java/org/apache/commons/pool2/pool407/AbstractPool407Factory.java
similarity index 57%
copy from src/test/java/org/apache/commons/pool2/pool407/Pool407NullObjectFactory.java
copy to src/test/java/org/apache/commons/pool2/pool407/AbstractPool407Factory.java
index fadcaa03..5638f2bd 100644
--- a/src/test/java/org/apache/commons/pool2/pool407/Pool407NullObjectFactory.java
+++ b/src/test/java/org/apache/commons/pool2/pool407/AbstractPool407Factory.java
@@ -19,27 +19,29 @@ package org.apache.commons.pool2.pool407;
import org.apache.commons.pool2.BasePooledObjectFactory;
import org.apache.commons.pool2.PooledObject;
-import org.apache.commons.pool2.impl.DefaultPooledObject;
-public final class Pool407NullObjectFactory extends BasePooledObjectFactory<Pool407Fixture, RuntimeException> {
+/**
+ * Tests POOL-407.
+ */
+public abstract class AbstractPool407Factory extends BasePooledObjectFactory<Pool407Fixture, RuntimeException> {
- @Override
- public Pool407Fixture create() {
- return null;
- }
+ /**
+ * Tests whether the subclass relies on the Pool's implementation of makeObject(). If the subclass returns false, then it implements makeObject(), in which
+ * case makeObject() returns a bad object like null or a null wrapper.
+ */
+ abstract boolean isDefaultMakeObject();
- @Override
- public PooledObject<Pool407Fixture> makeObject() throws RuntimeException {
- return new DefaultPooledObject<>(null);
- }
+ /**
+ * Tests whether this instance makes null or null wrappers.
+ *
+ * @return whether this instance makes null or null wrappers.
+ */
+ abstract boolean isNullFactory();
@Override
public boolean validateObject(final PooledObject<Pool407Fixture> p) {
- return p != null && p.getObject() != null;
+ // TODO Should this be enough even if wrap() does throw and returns a DefaultPooledObject wrapping a null?
+ return !PooledObject.isNull(p);
}
- @Override
- public PooledObject<Pool407Fixture> wrap(final Pool407Fixture value) {
- return new DefaultPooledObject<>(null);
- }
}
diff --git a/src/test/java/org/apache/commons/pool2/pool407/AbstractPool407Test.java b/src/test/java/org/apache/commons/pool2/pool407/AbstractPool407Test.java
new file mode 100644
index 00000000..60a252ef
--- /dev/null
+++ b/src/test/java/org/apache/commons/pool2/pool407/AbstractPool407Test.java
@@ -0,0 +1,56 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.commons.pool2.pool407;
+
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+import java.time.Duration;
+
+import org.apache.commons.pool2.PooledObject;
+
+/**
+ * Tests POOL-407.
+ */
+public class AbstractPool407Test {
+
+ protected <T> void assertShutdown(final boolean termination, final Duration poolConfigMaxWait, final T obj, final PooledObject<T> pooledObject) {
+ if (pooledObject != null) {
+ // The factory makes non-null objects and non-null PooledObjects,
+ // therefore the ExecutorService should terminate when requested, without delay.
+ assertTrue(termination);
+ } else {
+ // The factory makes null objects or null PooledObjects,
+ // therefore the ExecutorService should keep trying to create objects as configured in the pool's config object.
+ if (poolConfigMaxWait.equals(Pool407Constants.WAIT_FOREVER)) {
+ // If poolConfigMaxWait is maxed out, then the ExecutorService will not shutdown without delay.
+ if (obj == null) {
+ // create() returned null, so wrap() was not even called, and borrowObject() fails fast.
+ assertTrue(termination);
+ } else {
+ // The ExecutorService fails to terminate when requested because
+ assertFalse(true);
+ }
+ } else {
+ // If poolConfigMaxWait is short, then the ExecutorService should usually shutdown without delay.
+ assertTrue(termination);
+ }
+ }
+ }
+
+}
diff --git a/src/test/java/org/apache/commons/pool2/pool407/KeyedPool407.java b/src/test/java/org/apache/commons/pool2/pool407/KeyedPool407.java
index c843e43e..c7e22111 100644
--- a/src/test/java/org/apache/commons/pool2/pool407/KeyedPool407.java
+++ b/src/test/java/org/apache/commons/pool2/pool407/KeyedPool407.java
@@ -17,13 +17,16 @@
package org.apache.commons.pool2.pool407;
+import java.time.Duration;
+
import org.apache.commons.pool2.BaseKeyedPooledObjectFactory;
+import org.apache.commons.pool2.impl.BaseObjectPoolConfig;
import org.apache.commons.pool2.impl.GenericKeyedObjectPool;
public final class KeyedPool407 extends GenericKeyedObjectPool<String, KeyedPool407Fixture, RuntimeException> {
- public KeyedPool407(final BaseKeyedPooledObjectFactory<String, KeyedPool407Fixture, RuntimeException> factory) {
- super(factory, new KeyedPool407Config());
+ public KeyedPool407(final BaseKeyedPooledObjectFactory<String, KeyedPool407Fixture, RuntimeException> factory, final Duration maxWait) {
+ super(factory, new KeyedPool407Config(BaseObjectPoolConfig.DEFAULT_MAX_WAIT));
}
}
diff --git a/src/test/java/org/apache/commons/pool2/pool407/KeyedPool407Config.java b/src/test/java/org/apache/commons/pool2/pool407/KeyedPool407Config.java
index 903247bd..6feff5ff 100644
--- a/src/test/java/org/apache/commons/pool2/pool407/KeyedPool407Config.java
+++ b/src/test/java/org/apache/commons/pool2/pool407/KeyedPool407Config.java
@@ -23,9 +23,9 @@ import org.apache.commons.pool2.impl.GenericKeyedObjectPoolConfig;
public final class KeyedPool407Config extends GenericKeyedObjectPoolConfig<KeyedPool407Fixture> {
- public KeyedPool407Config() {
- setBlockWhenExhausted(true);
- setMaxTotalPerKey(1);
- setMaxWait(Duration.ofMillis(-1));
+ public KeyedPool407Config(final Duration poolConfigMaxWait) {
+ setBlockWhenExhausted(Pool407Constants.BLOCK_WHEN_EXHAUSTED);
+ setMaxTotalPerKey(Pool407Constants.MAX_TOTAL);
+ setMaxWait(poolConfigMaxWait);
}
}
diff --git a/src/test/java/org/apache/commons/pool2/pool407/KeyedPool407NormalFactory.java b/src/test/java/org/apache/commons/pool2/pool407/KeyedPool407NormalFactory.java
index 41bd65c4..8ff88bd8 100644
--- a/src/test/java/org/apache/commons/pool2/pool407/KeyedPool407NormalFactory.java
+++ b/src/test/java/org/apache/commons/pool2/pool407/KeyedPool407NormalFactory.java
@@ -17,11 +17,13 @@
package org.apache.commons.pool2.pool407;
-import org.apache.commons.pool2.BaseKeyedPooledObjectFactory;
import org.apache.commons.pool2.PooledObject;
import org.apache.commons.pool2.impl.DefaultPooledObject;
-public final class KeyedPool407NormalFactory extends BaseKeyedPooledObjectFactory<String, KeyedPool407Fixture, RuntimeException> {
+/**
+ * Tests POOL-407.
+ */
+public final class KeyedPool407NormalFactory extends AbstractKeyedPool407Factory {
private final KeyedPool407Fixture fixture;
@@ -38,15 +40,13 @@ public final class KeyedPool407NormalFactory extends BaseKeyedPooledObjectFactor
}
@Override
- public boolean validateObject(final String key, final PooledObject<KeyedPool407Fixture> p) {
- // TODO Should this be enough even if wrap() does throw and returns a DefaultPooledObject wrapping a null?
- return p.getObject() != null;
+ boolean isDefaultMakeObject() {
+ return true;
}
@Override
public PooledObject<KeyedPool407Fixture> wrap(final KeyedPool407Fixture value) {
- // Require a non-null value.
- // return new DefaultPooledObject<>(Objects.requireNonNull(value, "value"));
+ // value will never be null here.
return new DefaultPooledObject<>(value);
}
}
diff --git a/src/test/java/org/apache/commons/pool2/pool407/KeyedPool407NullObjectFactory.java b/src/test/java/org/apache/commons/pool2/pool407/KeyedPool407NullObjectFactory.java
index 2e9aa000..e3b9c206 100644
--- a/src/test/java/org/apache/commons/pool2/pool407/KeyedPool407NullObjectFactory.java
+++ b/src/test/java/org/apache/commons/pool2/pool407/KeyedPool407NullObjectFactory.java
@@ -17,11 +17,13 @@
package org.apache.commons.pool2.pool407;
-import org.apache.commons.pool2.BaseKeyedPooledObjectFactory;
import org.apache.commons.pool2.PooledObject;
import org.apache.commons.pool2.impl.DefaultPooledObject;
-public final class KeyedPool407NullObjectFactory extends BaseKeyedPooledObjectFactory<String, KeyedPool407Fixture, RuntimeException> {
+/**
+ * Tests POOL-407.
+ */
+public final class KeyedPool407NullObjectFactory extends AbstractKeyedPool407Factory {
@Override
public KeyedPool407Fixture create(final String key) {
@@ -29,13 +31,13 @@ public final class KeyedPool407NullObjectFactory extends BaseKeyedPooledObjectFa
}
@Override
- public PooledObject<KeyedPool407Fixture> makeObject(final String key) throws RuntimeException {
- return new DefaultPooledObject<>(null);
+ boolean isDefaultMakeObject() {
+ return false;
}
@Override
- public boolean validateObject(final String key, final PooledObject<KeyedPool407Fixture> p) {
- return p != null && p.getObject() != null;
+ public PooledObject<KeyedPool407Fixture> makeObject(final String key) throws RuntimeException {
+ return new DefaultPooledObject<>(null);
}
@Override
diff --git a/src/test/java/org/apache/commons/pool2/pool407/KeyedPool407NullPoolableObjectFactory.java b/src/test/java/org/apache/commons/pool2/pool407/KeyedPool407NullPoolableObjectFactory.java
index 1896a18f..c319fdcd 100644
--- a/src/test/java/org/apache/commons/pool2/pool407/KeyedPool407NullPoolableObjectFactory.java
+++ b/src/test/java/org/apache/commons/pool2/pool407/KeyedPool407NullPoolableObjectFactory.java
@@ -17,10 +17,12 @@
package org.apache.commons.pool2.pool407;
-import org.apache.commons.pool2.BaseKeyedPooledObjectFactory;
import org.apache.commons.pool2.PooledObject;
-public final class KeyedPool407NullPoolableObjectFactory extends BaseKeyedPooledObjectFactory<String, KeyedPool407Fixture, RuntimeException> {
+/**
+ * Tests POOL-407.
+ */
+public final class KeyedPool407NullPoolableObjectFactory extends AbstractKeyedPool407Factory {
@Override
public KeyedPool407Fixture create(final String key) {
@@ -28,13 +30,13 @@ public final class KeyedPool407NullPoolableObjectFactory extends BaseKeyedPooled
}
@Override
- public PooledObject<KeyedPool407Fixture> makeObject(final String key) throws RuntimeException {
- return null;
+ boolean isDefaultMakeObject() {
+ return false;
}
@Override
- public boolean validateObject(final String key, final PooledObject<KeyedPool407Fixture> p) {
- return p != null && p.getObject() != null;
+ public PooledObject<KeyedPool407Fixture> makeObject(final String key) throws RuntimeException {
+ return null;
}
@Override
diff --git a/src/test/java/org/apache/commons/pool2/pool407/KeyedPool407Test.java b/src/test/java/org/apache/commons/pool2/pool407/KeyedPool407Test.java
index 558ff732..356bc005 100644
--- a/src/test/java/org/apache/commons/pool2/pool407/KeyedPool407Test.java
+++ b/src/test/java/org/apache/commons/pool2/pool407/KeyedPool407Test.java
@@ -17,32 +17,35 @@
package org.apache.commons.pool2.pool407;
-import static org.junit.jupiter.api.Assertions.assertTrue;
-
+import java.time.Duration;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.Executors;
import java.util.concurrent.TimeUnit;
-import org.apache.commons.pool2.BaseKeyedPooledObjectFactory;
-import org.junit.jupiter.api.Disabled;
+import org.apache.commons.pool2.PooledObject;
import org.junit.jupiter.api.Test;
-public class KeyedPool407Test {
+/**
+ * Tests POOL-407.
+ */
+public class KeyedPool407Test extends AbstractPool407Test {
- private static class KeyedPool407Borrower implements Runnable {
+ /**
+ * Borrows from a pool and then immediately returns to that a pool.
+ */
+ private static class KeyedPool407RoundtripRunnable implements Runnable {
private final KeyedPool407 pool;
- public KeyedPool407Borrower(final KeyedPool407 pool) {
+ public KeyedPool407RoundtripRunnable(final KeyedPool407 pool) {
this.pool = pool;
}
@Override
public void run() {
try {
- final String key = "key";
- final KeyedPool407Fixture foo = pool.borrowObject(key);
- if (foo != null) {
- pool.returnObject(key, foo);
+ final KeyedPool407Fixture object = pool.borrowObject(KEY);
+ if (object != null) {
+ pool.returnObject(KEY, object);
}
} catch (final Exception e) {
throw new RuntimeException(e);
@@ -50,42 +53,61 @@ public class KeyedPool407Test {
}
}
- private static final int POOL_SIZE = 3;
+ private static final String KEY = "key";
+
+ protected void assertShutdown(final ExecutorService executor, final Duration poolConfigMaxWait, final AbstractKeyedPool407Factory factory) throws InterruptedException {
+ // Old note: This never finishes when the factory makes nulls because two threads are stuck forever
+ // If a factory always returns a null object or a null poolable object, then we will wait forever.
+ // This would also be true is object validation always fails.
+ executor.shutdown();
+ final boolean termination = executor.awaitTermination(Pool407Constants.AWAIT_TERMINATION_SECONDS, TimeUnit.SECONDS);
+ // Order matters: test create() before makeObject()
+ // Calling create() here in this test should not have side-effects
+ final KeyedPool407Fixture obj = factory.create(KEY);
+ // Calling makeObject() here in this test should not have side-effects
+ final PooledObject<KeyedPool407Fixture> pooledObject = obj != null ? factory.makeObject(KEY) : null;
+ assertShutdown(termination, poolConfigMaxWait, obj, pooledObject);
+ }
- private void test(final BaseKeyedPooledObjectFactory<String, KeyedPool407Fixture, RuntimeException> factory, final int poolSize)
- throws InterruptedException {
- final ExecutorService executor = Executors.newFixedThreadPool(poolSize);
- final KeyedPool407 pool = new KeyedPool407(factory);
- // start 'poolSize' threads that try to borrow a Pool407Fixture with the same key
- for (int i = 0; i < poolSize; i++) {
- executor.execute(new KeyedPool407Borrower(pool));
+ private void test(final AbstractKeyedPool407Factory factory, final int poolSize, final Duration poolConfigMaxWait) throws InterruptedException {
+ final ExecutorService executor = Executors.newFixedThreadPool(poolSize);
+ try (final KeyedPool407 pool = new KeyedPool407(factory, poolConfigMaxWait)) {
+ // Start 'poolSize' threads that try to borrow a Pool407Fixture with the same key
+ for (int i = 0; i < poolSize; i++) {
+ executor.execute(new KeyedPool407RoundtripRunnable(pool));
+ }
+ assertShutdown(executor, poolConfigMaxWait, factory);
}
+ }
- // this never finishes because two threads are stuck forever
- executor.shutdown();
- assertTrue(executor.awaitTermination(10, TimeUnit.SECONDS));
+ @Test
+ public void testNormalFactoryNonNullFixtureWaitMax() throws InterruptedException {
+ test(new KeyedPool407NormalFactory(new KeyedPool407Fixture()), Pool407Constants.POOL_SIZE, Pool407Constants.WAIT_FOREVER);
+ }
+
+ @Test
+ public void testNormalFactoryNullFixtureWaitMax() throws InterruptedException {
+ test(new KeyedPool407NormalFactory(null), Pool407Constants.POOL_SIZE, Pool407Constants.WAIT_FOREVER);
}
@Test
- public void testNormalFactoryNonNullFixture() throws InterruptedException {
- test(new KeyedPool407NormalFactory(new KeyedPool407Fixture()), POOL_SIZE);
+ public void testNullObjectFactoryWaitMax() throws InterruptedException {
+ test(new KeyedPool407NullObjectFactory(), Pool407Constants.POOL_SIZE, Pool407Constants.WAIT_FOREVER);
}
@Test
- public void testNormalFactoryNullFixture() throws InterruptedException {
- test(new KeyedPool407NormalFactory(null), POOL_SIZE);
+ public void testNullObjectFactoryWaitShort() throws InterruptedException {
+ test(new KeyedPool407NullObjectFactory(), Pool407Constants.POOL_SIZE, Pool407Constants.WAIT_SHORT);
}
@Test
- @Disabled("Either normal to fail or we should handle nulls internally better.")
- public void testNullObjectFactory() throws InterruptedException {
- test(new KeyedPool407NullObjectFactory(), POOL_SIZE);
+ public void testNullPoolableFactoryWaitMax() throws InterruptedException {
+ test(new KeyedPool407NullPoolableObjectFactory(), Pool407Constants.POOL_SIZE, Pool407Constants.WAIT_FOREVER);
}
@Test
- @Disabled("Either normal to fail or we should handle nulls internally better.")
- public void testNullPoolableFactory() throws InterruptedException {
- test(new KeyedPool407NullPoolableObjectFactory(), POOL_SIZE);
+ public void testNullPoolableFactoryWaitShort() throws InterruptedException {
+ test(new KeyedPool407NullPoolableObjectFactory(), Pool407Constants.POOL_SIZE, Pool407Constants.WAIT_SHORT);
}
}
diff --git a/src/test/java/org/apache/commons/pool2/pool407/Pool407.java b/src/test/java/org/apache/commons/pool2/pool407/Pool407.java
index d393dfdc..6880ec8b 100644
--- a/src/test/java/org/apache/commons/pool2/pool407/Pool407.java
+++ b/src/test/java/org/apache/commons/pool2/pool407/Pool407.java
@@ -17,16 +17,17 @@
package org.apache.commons.pool2.pool407;
+import java.time.Duration;
+
import org.apache.commons.pool2.BasePooledObjectFactory;
import org.apache.commons.pool2.impl.GenericObjectPool;
+/**
+ * Tests POOL-407.
+ */
public final class Pool407 extends GenericObjectPool<Pool407Fixture, RuntimeException> {
- public Pool407(final Pool407Fixture fixture) {
- super(new Pool407NormalFactory(fixture), new Pool407Config());
- }
-
- public Pool407(final BasePooledObjectFactory<Pool407Fixture, RuntimeException> factory) {
- super(factory, new Pool407Config());
+ public Pool407(final BasePooledObjectFactory<Pool407Fixture, RuntimeException> factory, final Duration poolConfigMaxWait) {
+ super(factory, new Pool407Config(poolConfigMaxWait));
}
}
diff --git a/src/test/java/org/apache/commons/pool2/pool407/Pool407Config.java b/src/test/java/org/apache/commons/pool2/pool407/Pool407Config.java
index 8cbbe2f7..ee75d183 100644
--- a/src/test/java/org/apache/commons/pool2/pool407/Pool407Config.java
+++ b/src/test/java/org/apache/commons/pool2/pool407/Pool407Config.java
@@ -21,11 +21,14 @@ import java.time.Duration;
import org.apache.commons.pool2.impl.GenericObjectPoolConfig;
+/**
+ * Tests POOL-407.
+ */
public final class Pool407Config extends GenericObjectPoolConfig<Pool407Fixture> {
- public Pool407Config() {
- setBlockWhenExhausted(true);
- setMaxTotal(1);
- setMaxWait(Duration.ofMillis(-1));
+ public Pool407Config(final Duration poolConfigMaxWait) {
+ setBlockWhenExhausted(Pool407Constants.BLOCK_WHEN_EXHAUSTED);
+ setMaxTotal(Pool407Constants.MAX_TOTAL);
+ setMaxWait(poolConfigMaxWait);
}
}
diff --git a/src/test/java/org/apache/commons/pool2/pool407/KeyedPool407Config.java b/src/test/java/org/apache/commons/pool2/pool407/Pool407Constants.java
similarity index 66%
copy from src/test/java/org/apache/commons/pool2/pool407/KeyedPool407Config.java
copy to src/test/java/org/apache/commons/pool2/pool407/Pool407Constants.java
index 903247bd..9e35124c 100644
--- a/src/test/java/org/apache/commons/pool2/pool407/KeyedPool407Config.java
+++ b/src/test/java/org/apache/commons/pool2/pool407/Pool407Constants.java
@@ -19,13 +19,18 @@ package org.apache.commons.pool2.pool407;
import java.time.Duration;
-import org.apache.commons.pool2.impl.GenericKeyedObjectPoolConfig;
+import org.apache.commons.pool2.impl.BaseObjectPoolConfig;
-public final class KeyedPool407Config extends GenericKeyedObjectPoolConfig<KeyedPool407Fixture> {
+/**
+ * Tests POOL-407.
+ */
+class Pool407Constants {
+
+ static final int AWAIT_TERMINATION_SECONDS = 10;
+ static final boolean BLOCK_WHEN_EXHAUSTED = true;
+ static final int MAX_TOTAL = 1;
+ static final int POOL_SIZE = 3;
+ static final Duration WAIT_FOREVER = BaseObjectPoolConfig.DEFAULT_MAX_WAIT;
+ static final Duration WAIT_SHORT = Duration.ofSeconds(1);
- public KeyedPool407Config() {
- setBlockWhenExhausted(true);
- setMaxTotalPerKey(1);
- setMaxWait(Duration.ofMillis(-1));
- }
}
diff --git a/src/test/java/org/apache/commons/pool2/pool407/Pool407NormalFactory.java b/src/test/java/org/apache/commons/pool2/pool407/Pool407NormalFactory.java
index 26b89a7c..8ade79a4 100644
--- a/src/test/java/org/apache/commons/pool2/pool407/Pool407NormalFactory.java
+++ b/src/test/java/org/apache/commons/pool2/pool407/Pool407NormalFactory.java
@@ -17,11 +17,13 @@
package org.apache.commons.pool2.pool407;
-import org.apache.commons.pool2.BasePooledObjectFactory;
import org.apache.commons.pool2.PooledObject;
import org.apache.commons.pool2.impl.DefaultPooledObject;
-public final class Pool407NormalFactory extends BasePooledObjectFactory<Pool407Fixture, RuntimeException> {
+/**
+ * Tests POOL-407.
+ */
+public final class Pool407NormalFactory extends AbstractPool407Factory {
private final Pool407Fixture fixture;
@@ -31,6 +33,9 @@ public final class Pool407NormalFactory extends BasePooledObjectFactory<Pool407F
@Override
public Pool407Fixture create() {
+ // When this returns null, we fail-fast internally and borrowsObject() throws an exception.
+ //
+ // Old note:
// This is key to the test, creation failed and returns null for instance see
// https://github.com/openhab/openhab-core/blob/main/bundles/org.openhab.core.io.transport.modbus/src/main/java/org/openhab/core/io/transport/modbus/internal/pooling/ModbusSlaveConnectionFactoryImpl.java#L163
// the test passes when this returns new Pool407Fixture();
@@ -38,15 +43,18 @@ public final class Pool407NormalFactory extends BasePooledObjectFactory<Pool407F
}
@Override
- public boolean validateObject(final PooledObject<Pool407Fixture> p) {
- // TODO Should this be enough even if wrap() does throw and returns a DefaultPooledObject wrapping a null?
- return p.getObject() != null;
+ boolean isDefaultMakeObject() {
+ return true;
+ }
+
+ @Override
+ boolean isNullFactory() {
+ return fixture == null;
}
@Override
public PooledObject<Pool407Fixture> wrap(final Pool407Fixture value) {
- // Require a non-null value.
- // return new DefaultPooledObject<>(Objects.requireNonNull(value, "value"));
+ // value will never be null here.
return new DefaultPooledObject<>(value);
}
}
diff --git a/src/test/java/org/apache/commons/pool2/pool407/Pool407NullObjectFactory.java b/src/test/java/org/apache/commons/pool2/pool407/Pool407NullObjectFactory.java
index fadcaa03..eecf8d26 100644
--- a/src/test/java/org/apache/commons/pool2/pool407/Pool407NullObjectFactory.java
+++ b/src/test/java/org/apache/commons/pool2/pool407/Pool407NullObjectFactory.java
@@ -17,25 +17,33 @@
package org.apache.commons.pool2.pool407;
-import org.apache.commons.pool2.BasePooledObjectFactory;
import org.apache.commons.pool2.PooledObject;
import org.apache.commons.pool2.impl.DefaultPooledObject;
-public final class Pool407NullObjectFactory extends BasePooledObjectFactory<Pool407Fixture, RuntimeException> {
+/**
+ * Tests POOL-407.
+ */
+public final class Pool407NullObjectFactory extends AbstractPool407Factory {
@Override
public Pool407Fixture create() {
+ // Never called because this class calls makeObject() and wrap().
return null;
}
@Override
- public PooledObject<Pool407Fixture> makeObject() throws RuntimeException {
- return new DefaultPooledObject<>(null);
+ boolean isDefaultMakeObject() {
+ return false;
+ }
+
+ @Override
+ boolean isNullFactory() {
+ return true;
}
@Override
- public boolean validateObject(final PooledObject<Pool407Fixture> p) {
- return p != null && p.getObject() != null;
+ public PooledObject<Pool407Fixture> makeObject() throws RuntimeException {
+ return new DefaultPooledObject<>(null);
}
@Override
diff --git a/src/test/java/org/apache/commons/pool2/pool407/Pool407NullPoolableObjectFactory.java b/src/test/java/org/apache/commons/pool2/pool407/Pool407NullPoolableObjectFactory.java
index b17d0e57..274057d2 100644
--- a/src/test/java/org/apache/commons/pool2/pool407/Pool407NullPoolableObjectFactory.java
+++ b/src/test/java/org/apache/commons/pool2/pool407/Pool407NullPoolableObjectFactory.java
@@ -17,24 +17,31 @@
package org.apache.commons.pool2.pool407;
-import org.apache.commons.pool2.BasePooledObjectFactory;
import org.apache.commons.pool2.PooledObject;
-public final class Pool407NullPoolableObjectFactory extends BasePooledObjectFactory<Pool407Fixture, RuntimeException> {
-
+/**
+ * Tests POOL-407.
+ */
+public final class Pool407NullPoolableObjectFactory extends AbstractPool407Factory {
+
@Override
public Pool407Fixture create() {
return null;
}
@Override
- public PooledObject<Pool407Fixture> makeObject() throws RuntimeException {
- return null;
+ boolean isDefaultMakeObject() {
+ return false;
}
@Override
- public boolean validateObject(final PooledObject<Pool407Fixture> p) {
- return p != null && p.getObject() != null;
+ boolean isNullFactory() {
+ return true;
+ }
+
+ @Override
+ public PooledObject<Pool407Fixture> makeObject() throws RuntimeException {
+ return null;
}
@Override
diff --git a/src/test/java/org/apache/commons/pool2/pool407/Pool407Test.java b/src/test/java/org/apache/commons/pool2/pool407/Pool407Test.java
index ccaa5569..09255d8a 100644
--- a/src/test/java/org/apache/commons/pool2/pool407/Pool407Test.java
+++ b/src/test/java/org/apache/commons/pool2/pool407/Pool407Test.java
@@ -17,31 +17,35 @@
package org.apache.commons.pool2.pool407;
-import static org.junit.jupiter.api.Assertions.assertTrue;
-
+import java.time.Duration;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.Executors;
import java.util.concurrent.TimeUnit;
-import org.apache.commons.pool2.BasePooledObjectFactory;
-import org.junit.jupiter.api.Disabled;
+import org.apache.commons.pool2.PooledObject;
import org.junit.jupiter.api.Test;
-public class Pool407Test {
+/**
+ * Tests POOL-407.
+ */
+public class Pool407Test extends AbstractPool407Test {
- private static class Pool407Borrower implements Runnable {
+ /**
+ * Borrows from a pool and then immediately returns to that a pool.
+ */
+ private static class Pool407RoundtripRunnable implements Runnable {
private final Pool407 pool;
- public Pool407Borrower(final Pool407 pool) {
+ public Pool407RoundtripRunnable(final Pool407 pool) {
this.pool = pool;
}
@Override
public void run() {
try {
- final Pool407Fixture foo = pool.borrowObject();
- if (foo != null) {
- pool.returnObject(foo);
+ final Pool407Fixture object = pool.borrowObject();
+ if (object != null) {
+ pool.returnObject(object);
}
} catch (final Exception e) {
throw new RuntimeException(e);
@@ -49,41 +53,58 @@ public class Pool407Test {
}
}
- private static final int POOL_SIZE = 3;
+ protected void assertShutdown(final ExecutorService executor, final Duration poolConfigMaxWait, final AbstractPool407Factory factory) throws InterruptedException {
+ // Old note: This never finishes when the factory makes nulls because two threads are stuck forever
+ // If a factory always returns a null object or a null poolable object, then we will wait forever.
+ // This would also be true is object validation always fails.
+ executor.shutdown();
+ final boolean termination = executor.awaitTermination(Pool407Constants.AWAIT_TERMINATION_SECONDS, TimeUnit.SECONDS);
+ // Order matters: test create() before makeObject()
+ // Calling create() here in this test should not have side-effects
+ final Pool407Fixture obj = factory.create();
+ // Calling makeObject() here in this test should not have side-effects
+ final PooledObject<Pool407Fixture> pooledObject = obj != null ? factory.makeObject() : null;
+ assertShutdown(termination, poolConfigMaxWait, obj, pooledObject);
+ }
- private void test(final BasePooledObjectFactory<Pool407Fixture, RuntimeException> factory, final int poolSize) throws InterruptedException {
+ private void test(final AbstractPool407Factory factory, final int poolSize, final Duration poolConfigMaxWait) throws InterruptedException {
final ExecutorService executor = Executors.newFixedThreadPool(poolSize);
- final Pool407 pool = new Pool407(factory);
-
- // start 'poolSize' threads that try to borrow a Pool407Fixture with the same key
- for (int i = 0; i < poolSize; i++) {
- executor.execute(new Pool407Borrower(pool));
+ try (final Pool407 pool = new Pool407(factory, poolConfigMaxWait)) {
+ // Start 'poolSize' threads that try to borrow a Pool407Fixture with the same key
+ for (int i = 0; i < poolSize; i++) {
+ executor.execute(new Pool407RoundtripRunnable(pool));
+ }
+ assertShutdown(executor, poolConfigMaxWait, factory);
}
+ }
- // this never finishes because two threads are stuck forever
- executor.shutdown();
- assertTrue(executor.awaitTermination(10, TimeUnit.SECONDS));
+ @Test
+ public void testNormalFactoryNonNullFixtureWaitMax() throws InterruptedException {
+ test(new Pool407NormalFactory(new Pool407Fixture()), Pool407Constants.POOL_SIZE, Pool407Constants.WAIT_FOREVER);
+ }
+
+ @Test
+ public void testNormalFactoryNullFixtureWaitMax() throws InterruptedException {
+ test(new Pool407NormalFactory(null), Pool407Constants.POOL_SIZE, Pool407Constants.WAIT_FOREVER);
}
@Test
- public void testNormalFactoryNonNullFixture() throws InterruptedException {
- test(new Pool407NormalFactory(new Pool407Fixture()), 3);
+ public void testNullObjectFactoryWaitMax() throws InterruptedException {
+ test(new Pool407NullObjectFactory(), Pool407Constants.POOL_SIZE, Pool407Constants.WAIT_FOREVER);
}
@Test
- public void testNormalFactoryNullFixture() throws InterruptedException {
- test(new Pool407NormalFactory(null), POOL_SIZE);
+ public void testNullObjectFactoryWaitShort() throws InterruptedException {
+ test(new Pool407NullObjectFactory(), Pool407Constants.POOL_SIZE, Pool407Constants.WAIT_SHORT);
}
@Test
- @Disabled("Either normal to fail or we should handle nulls internally better.")
- public void testNullObjectFactory() throws InterruptedException {
- test(new Pool407NullObjectFactory(), POOL_SIZE);
+ public void testNullPoolableFactoryWaitMax() throws InterruptedException {
+ test(new Pool407NullPoolableObjectFactory(), Pool407Constants.POOL_SIZE, Pool407Constants.WAIT_FOREVER);
}
@Test
- @Disabled("Either normal to fail or we should handle nulls internally better.")
- public void testNullPoolableFactory() throws InterruptedException {
- test(new Pool407NullPoolableObjectFactory(), POOL_SIZE);
+ public void testNullPoolableFactoryWaitShort() throws InterruptedException {
+ test(new Pool407NullPoolableObjectFactory(), Pool407Constants.POOL_SIZE, Pool407Constants.WAIT_SHORT);
}
}