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 b7b4322 [POOL-326] Threading issue, NullPointerException and
IllegalStateException in GenericKeyedObjectPool.
b7b4322 is described below
commit b7b4322aada5d6d5521d47b5fed769b40753d68e
Author: Gary Gregory <garydgreg...@gmail.com>
AuthorDate: Fri Feb 8 12:48:53 2019 -0500
[POOL-326] Threading issue, NullPointerException and
IllegalStateException in GenericKeyedObjectPool.
---
src/changes/changes.xml | 3 +
.../commons/pool2/impl/GenericKeyedObjectPool.java | 30 ++--
.../apache/commons/pool2/ObjectPoolIssue326.java | 183 +++++++++++++++++++++
3 files changed, 202 insertions(+), 14 deletions(-)
diff --git a/src/changes/changes.xml b/src/changes/changes.xml
index c189121..7026378 100644
--- a/src/changes/changes.xml
+++ b/src/changes/changes.xml
@@ -68,6 +68,9 @@ The <action> type attribute can be add,update,fix,remove.
<action dev="ggregory" issue="POOL-360" type="update">
Update optional library cglib from 3.2.9 to 3.2.10.
</action>
+ <action dev="ggregory" issue="POOL-326" type="fix" due-to="Chris Allison,
Phil Steitz">
+ Threading issue, NullPointerException and IllegalStateException in
GenericKeyedObjectPool.
+ </action>
</release>
<release version="2.6.0" date="2018-07-06" description="This is a
maintenance release.">
<action dev="ggregory" issue="POOL-336" type="update" due-to="Wolfgang
Glas">
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 72aab16..ab5950e 100644
--- a/src/main/java/org/apache/commons/pool2/impl/GenericKeyedObjectPool.java
+++ b/src/main/java/org/apache/commons/pool2/impl/GenericKeyedObjectPool.java
@@ -1144,26 +1144,28 @@ public class GenericKeyedObjectPool<K, T> extends
BaseGenericObjectPool<T>
* @param k The key to de-register
*/
private void deregister(final K k) {
+ Lock lock = keyLock.readLock();
ObjectDeque<T> objectDeque;
-
- objectDeque = poolMap.get(k);
- final long numInterested =
objectDeque.getNumInterested().decrementAndGet();
- if (numInterested == 0 && objectDeque.getCreateCount().get() == 0) {
- // Potential to remove key
- final Lock writeLock = keyLock.writeLock();
- writeLock.lock();
- try {
- if (objectDeque.getCreateCount().get() == 0 &&
- objectDeque.getNumInterested().get() == 0) {
+ try {
+ lock.lock();
+ objectDeque = poolMap.get(k);
+ final long numInterested =
objectDeque.getNumInterested().decrementAndGet();
+ if (numInterested == 0 && objectDeque.getCreateCount().get() == 0)
{
+ // Potential to remove key
+ // Upgrade to write lock
+ lock.unlock();
+ lock = keyLock.writeLock();
+ lock.lock();
+ if (objectDeque.getCreateCount().get() == 0 &&
objectDeque.getNumInterested().get() == 0) {
// NOTE: Keys must always be removed from both poolMap and
- // poolKeyList at the same time while protected by
- // keyLock.writeLock()
+ // poolKeyList at the same time while protected by
+ // keyLock.writeLock()
poolMap.remove(k);
poolKeyList.remove(k);
}
- } finally {
- writeLock.unlock();
}
+ } finally {
+ lock.unlock();
}
}
diff --git a/src/test/java/org/apache/commons/pool2/ObjectPoolIssue326.java
b/src/test/java/org/apache/commons/pool2/ObjectPoolIssue326.java
new file mode 100644
index 0000000..d1fbb49
--- /dev/null
+++ b/src/test/java/org/apache/commons/pool2/ObjectPoolIssue326.java
@@ -0,0 +1,183 @@
+/*
+ * 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;
+
+import java.util.ArrayList;
+import java.util.List;
+import java.util.NoSuchElementException;
+import java.util.concurrent.Callable;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.Future;
+import org.apache.commons.pool2.BaseKeyedPooledObjectFactory;
+import org.apache.commons.pool2.PooledObject;
+import org.apache.commons.pool2.impl.BaseObjectPoolConfig;
+import org.apache.commons.pool2.impl.DefaultPooledObject;
+import org.apache.commons.pool2.impl.GenericKeyedObjectPool;
+import org.apache.commons.pool2.impl.GenericKeyedObjectPoolConfig;
+
+/**
+ * On my box with 4 cores this test fails at between 5s and 900s with an
average
+ * of 240s (data from 10 runs of test).
+ *
+ * It is hard to turn this in a unit test because it can affect the build
+ * negatively since you need to run it for a while.
+ */
+public final class ObjectPoolIssue326 {
+ public static void main(String[] args) {
+ try {
+ new ObjectPoolIssue326().run();
+ } catch (Exception e) {
+ e.printStackTrace();
+ }
+ }
+
+ private void run() throws Exception {
+ GenericKeyedObjectPoolConfig poolConfig = new
GenericKeyedObjectPoolConfig();
+ poolConfig.setMaxTotal(10);
+ poolConfig.setMaxTotalPerKey(5);
+ poolConfig.setMinIdlePerKey(-1);
+ poolConfig.setMaxIdlePerKey(-1);
+ poolConfig.setLifo(true);
+ poolConfig.setFairness(true);
+ poolConfig.setMaxWaitMillis(30 * 1000);
+ poolConfig.setMinEvictableIdleTimeMillis(-1);
+ poolConfig.setSoftMinEvictableIdleTimeMillis(-1);
+ poolConfig.setNumTestsPerEvictionRun(1);
+ poolConfig.setTestOnCreate(false);
+ poolConfig.setTestOnBorrow(false);
+ poolConfig.setTestOnReturn(false);
+ poolConfig.setTestWhileIdle(false);
+ poolConfig.setTimeBetweenEvictionRunsMillis(5 * 1000);
+
poolConfig.setEvictionPolicyClassName(BaseObjectPoolConfig.DEFAULT_EVICTION_POLICY_CLASS_NAME);
+ poolConfig.setBlockWhenExhausted(false);
+ poolConfig.setJmxEnabled(false);
+ poolConfig.setJmxNameBase(null);
+ poolConfig.setJmxNamePrefix(null);
+
+ GenericKeyedObjectPool<Integer, Object> pool = new
GenericKeyedObjectPool<>(new ObjectFactory(), poolConfig);
+
+ // number of threads to reproduce is finicky. this count seems to be
best for my
+ // 4 core box.
+ // too many doesn't reproduce it ever, too few doesn't either.
+ ExecutorService service =
Executors.newFixedThreadPool(Runtime.getRuntime().availableProcessors() * 2);
+ long startTime = System.currentTimeMillis();
+ long testIter = 0;
+ try {
+ while (true) {
+ testIter++;
+ if (testIter % 1000 == 0) {
+ System.out.println(testIter);
+ }
+ List<Task> tasks = createTasks(pool);
+ List<Future<Object>> futures = service.invokeAll(tasks);
+ for (Future<Object> future : futures) {
+ future.get();
+ }
+ }
+ } finally {
+ System.out.println("Time: " + (System.currentTimeMillis() -
startTime) / 1000.0);
+ service.shutdown();
+ }
+ }
+
+ private List<Task> createTasks(GenericKeyedObjectPool<Integer, Object>
pool) {
+ List<Task> tasks = new ArrayList<>();
+ for (int i = 0; i < 250; i++) {
+ tasks.add(new Task(pool, i));
+ }
+ return tasks;
+ }
+
+ private class ObjectFactory extends BaseKeyedPooledObjectFactory<Integer,
Object> {
+ @Override
+ public Object create(Integer s) throws Exception {
+ return new TestObject();
+ }
+
+ @Override
+ public PooledObject<Object> wrap(Object o) {
+ return new DefaultPooledObject<>(o);
+ }
+ }
+
+ private class TestObject {
+ }
+
+ private class Task implements Callable<Object> {
+ private final GenericKeyedObjectPool<Integer, Object> m_pool;
+ private final int m_key;
+
+ Task(GenericKeyedObjectPool<Integer, Object> pool, int count) {
+ m_pool = pool;
+ m_key = count % 20;
+ }
+
+ @Override
+ public Object call() throws Exception {
+ try {
+ Object value;
+ value = m_pool.borrowObject(m_key);
+ // don't make this too long or it won't reproduce, and don't
make it zero or it
+ // won't reproduce
+ // constant low value also doesn't reproduce
+ busyWait(System.currentTimeMillis() % 4);
+ m_pool.returnObject(m_key, value);
+ return "success";
+ } catch (NoSuchElementException e) {
+ // ignore, we've exhausted the pool
+ // not sure whether what we do here matters for reproducing
+ busyWait(System.currentTimeMillis() % 20);
+ return "exhausted";
+ }
+ }
+
+ private void busyWait(long timeMillis) {
+ // busy waiting intentionally as a simple thread.sleep fails to
reproduce
+ long endTime = System.currentTimeMillis() + timeMillis;
+ while (System.currentTimeMillis() < endTime)
+ ;
+ }
+ }
+}
+
+/*
+ *
+ * Example stack trace: java.util.concurrent.ExecutionException:
+ * java.lang.NullPointerException at
+ * java.util.concurrent.FutureTask.report(FutureTask.java:122) at
+ * java.util.concurrent.FutureTask.get(FutureTask.java:192) at
+ * threading_pool.ObjectPoolIssue.run(ObjectPoolIssue.java:63) at
+ * threading_pool.ObjectPoolIssue.main(ObjectPoolIssue.java:23) at
+ * sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at
+ *
sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
+ * at
+ *
sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.
+ * java:43) at java.lang.reflect.Method.invoke(Method.java:498) at
+ * com.intellij.rt.execution.application.AppMain.main(AppMain.java:147) Caused
+ * by: java.lang.NullPointerException at
+ * org.apache.commons.pool2.impl.GenericKeyedObjectPool.returnObject(
+ * GenericKeyedObjectPool.java:474) at
+ * threading_pool.ObjectPoolIssue$Task.call(ObjectPoolIssue.java:112) at
+ * java.util.concurrent.FutureTask.run(FutureTask.java:266) at
+ * java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:
+ * 1142) at
+ * java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:
+ * 617) at java.lang.Thread.run(Thread.java:745)
+ *
+ */
