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-dbcp.git
The following commit(s) were added to refs/heads/master by this push:
new e627d55f PooledConnectionImpl.destroyObject(PStmtKey, PooledObject)
can throw NullPointerException
e627d55f is described below
commit e627d55fe8a33b3ca482aac1da1b5555761c6146
Author: Gary Gregory <[email protected]>
AuthorDate: Thu Feb 29 11:39:40 2024 -0500
PooledConnectionImpl.destroyObject(PStmtKey, PooledObject) can throw
NullPointerException
- PoolingConnection.destroyObject(PStmtKey, PooledObject) can throw
NullPointerException #312
- See also PR #312
---
src/changes/changes.xml | 2 +
.../commons/dbcp2/PoolablePreparedStatement.java | 9 ++
.../apache/commons/dbcp2/PoolingConnection.java | 15 ++-
.../commons/dbcp2/cpdsadapter/ConnectionImpl.java | 17 +++-
.../dbcp2/cpdsadapter/PooledConnectionImpl.java | 16 +++-
.../java/org/apache/commons/dbcp2/TestUtils.java | 4 +
.../dbcp2/cpdsadapter/TestDriverAdapterCPDS.java | 102 ++++++++++++++++-----
7 files changed, 133 insertions(+), 32 deletions(-)
diff --git a/src/changes/changes.xml b/src/changes/changes.xml
index cd2b18a6..0394f577 100644
--- a/src/changes/changes.xml
+++ b/src/changes/changes.xml
@@ -66,6 +66,8 @@ The <action> type attribute can be add,update,fix,remove.
<!-- FIX -->
<action type="fix" issue="DBCP-590" dev="psteitz" due-to=" Réda Housni
Alaoui">BasicDataSource#setAbandonedUsageTracking has no effect.</action>
<action type="fix" issue="DBCP-596" dev="ggregory" due-to="Aapo
Haapanen, Gary Gregory">PoolingConnection.toString() causes
StackOverflowError.</action>
+ <action type="fix" dev="ggregory" due-to="Gary Gregory, Rémy
Maucherat">PooledConnectionImpl.destroyObject(PStmtKey, PooledObject) can throw
NullPointerException #312.</action>
+ <action type="fix" dev="ggregory" due-to="Gary Gregory, Rémy
Maucherat">PoolingConnection.destroyObject(PStmtKey, PooledObject) can throw
NullPointerException #312.</action>
<!-- ADD -->
<action type="add" dev="ggregory" due-to="Gary Gregory">Add property
project.build.outputTimestamp for build reproducibility.</action>
<action type="add" dev="ggregory" due-to="Heewon Lee">Add null guards in
DelegatingDatabaseMetaData constructor #352.</action>
diff --git
a/src/main/java/org/apache/commons/dbcp2/PoolablePreparedStatement.java
b/src/main/java/org/apache/commons/dbcp2/PoolablePreparedStatement.java
index 31556c48..f1d736f0 100644
--- a/src/main/java/org/apache/commons/dbcp2/PoolablePreparedStatement.java
+++ b/src/main/java/org/apache/commons/dbcp2/PoolablePreparedStatement.java
@@ -113,6 +113,15 @@ public class PoolablePreparedStatement<K> extends
DelegatingPreparedStatement {
}
}
+ /**
+ * Package-protected for tests.
+ *
+ * @return The key.
+ */
+ K getKey() {
+ return key;
+ }
+
@Override
public void passivate() throws SQLException {
// DBCP-372. clearBatch with throw an exception if called when the
diff --git a/src/main/java/org/apache/commons/dbcp2/PoolingConnection.java
b/src/main/java/org/apache/commons/dbcp2/PoolingConnection.java
index 2d62609d..be2d4dc4 100644
--- a/src/main/java/org/apache/commons/dbcp2/PoolingConnection.java
+++ b/src/main/java/org/apache/commons/dbcp2/PoolingConnection.java
@@ -303,9 +303,18 @@ public class PoolingConnection extends
DelegatingConnection<Connection>
* the wrapped pooled statement to be destroyed.
*/
@Override
- public void destroyObject(final PStmtKey key, final
PooledObject<DelegatingPreparedStatement> pooledObject)
- throws SQLException {
- pooledObject.getObject().getInnermostDelegate().close();
+ public void destroyObject(final PStmtKey key, final
PooledObject<DelegatingPreparedStatement> pooledObject) throws SQLException {
+ if (pooledObject != null) {
+ @SuppressWarnings("resource")
+ final DelegatingPreparedStatement object =
pooledObject.getObject();
+ if (object != null) {
+ @SuppressWarnings("resource")
+ final Statement innermostDelegate =
object.getInnermostDelegate();
+ if (innermostDelegate != null) {
+ innermostDelegate.close();
+ }
+ }
+ }
}
private String getCatalogOrNull() {
diff --git
a/src/main/java/org/apache/commons/dbcp2/cpdsadapter/ConnectionImpl.java
b/src/main/java/org/apache/commons/dbcp2/cpdsadapter/ConnectionImpl.java
index 1593e5ce..bbb8ecd6 100644
--- a/src/main/java/org/apache/commons/dbcp2/cpdsadapter/ConnectionImpl.java
+++ b/src/main/java/org/apache/commons/dbcp2/cpdsadapter/ConnectionImpl.java
@@ -111,6 +111,15 @@ final class ConnectionImpl extends
DelegatingConnection<Connection> {
return null;
}
+ /**
+ * Package-private for tests.
+ *
+ * @return the PooledConnectionImpl.
+ */
+ PooledConnectionImpl getPooledConnectionImpl() {
+ return pooledConnection;
+ }
+
/**
* If false, getDelegate() and getInnermostDelegate() will return null.
*
@@ -245,6 +254,10 @@ final class ConnectionImpl extends
DelegatingConnection<Connection> {
}
}
+ //
+ // Methods for accessing the delegate connection
+ //
+
/**
* If pooling of {@code PreparedStatement}s is turned on in the {@link
DriverAdapterCPDS}, a pooled object may
* be returned, otherwise delegate to the wrapped JDBC 1.x {@link
java.sql.Connection}.
@@ -265,10 +278,6 @@ final class ConnectionImpl extends
DelegatingConnection<Connection> {
}
}
- //
- // Methods for accessing the delegate connection
- //
-
@Override
public PreparedStatement prepareStatement(final String sql, final int
resultSetType, final int resultSetConcurrency,
final int resultSetHoldability) throws SQLException {
diff --git
a/src/main/java/org/apache/commons/dbcp2/cpdsadapter/PooledConnectionImpl.java
b/src/main/java/org/apache/commons/dbcp2/cpdsadapter/PooledConnectionImpl.java
index 748166c6..f81ec81c 100644
---
a/src/main/java/org/apache/commons/dbcp2/cpdsadapter/PooledConnectionImpl.java
+++
b/src/main/java/org/apache/commons/dbcp2/cpdsadapter/PooledConnectionImpl.java
@@ -20,6 +20,7 @@ import java.sql.CallableStatement;
import java.sql.Connection;
import java.sql.PreparedStatement;
import java.sql.SQLException;
+import java.sql.Statement;
import java.util.ArrayList;
import java.util.Collections;
import java.util.List;
@@ -331,9 +332,18 @@ final class PooledConnectionImpl
* the wrapped {@link PreparedStatement} to be destroyed.
*/
@Override
- public void destroyObject(final PStmtKey key, final
PooledObject<DelegatingPreparedStatement> pooledObject)
- throws SQLException {
- pooledObject.getObject().getInnermostDelegate().close();
+ public void destroyObject(final PStmtKey key, final
PooledObject<DelegatingPreparedStatement> pooledObject) throws SQLException {
+ if (pooledObject != null) {
+ @SuppressWarnings("resource")
+ final DelegatingPreparedStatement object =
pooledObject.getObject();
+ if (object != null) {
+ @SuppressWarnings("resource")
+ final Statement innermostDelegate =
object.getInnermostDelegate();
+ if (innermostDelegate != null) {
+ innermostDelegate.close();
+ }
+ }
+ }
}
/**
diff --git a/src/test/java/org/apache/commons/dbcp2/TestUtils.java
b/src/test/java/org/apache/commons/dbcp2/TestUtils.java
index c2cb80c3..490ee6d6 100644
--- a/src/test/java/org/apache/commons/dbcp2/TestUtils.java
+++ b/src/test/java/org/apache/commons/dbcp2/TestUtils.java
@@ -21,6 +21,10 @@ import org.junit.jupiter.api.Test;
public class TestUtils {
+ public static PStmtKey getPStmtKey(final
PoolablePreparedStatement<PStmtKey> poolablePreparedStatement) {
+ return poolablePreparedStatement.getKey();
+ }
+
@Test
public void testClassLoads() {
Utils.closeQuietly((AutoCloseable) null);
diff --git
a/src/test/java/org/apache/commons/dbcp2/cpdsadapter/TestDriverAdapterCPDS.java
b/src/test/java/org/apache/commons/dbcp2/cpdsadapter/TestDriverAdapterCPDS.java
index 27911c9d..6fc6042f 100644
---
a/src/test/java/org/apache/commons/dbcp2/cpdsadapter/TestDriverAdapterCPDS.java
+++
b/src/test/java/org/apache/commons/dbcp2/cpdsadapter/TestDriverAdapterCPDS.java
@@ -31,6 +31,7 @@ import java.sql.PreparedStatement;
import java.sql.ResultSet;
import java.sql.SQLException;
import java.sql.SQLFeatureNotSupportedException;
+import java.sql.Statement;
import java.time.Duration;
import java.util.Properties;
@@ -40,7 +41,13 @@ import javax.naming.StringRefAddr;
import javax.sql.DataSource;
import org.apache.commons.dbcp2.Constants;
+import org.apache.commons.dbcp2.DelegatingPreparedStatement;
+import org.apache.commons.dbcp2.DelegatingStatement;
+import org.apache.commons.dbcp2.PStmtKey;
+import org.apache.commons.dbcp2.PoolablePreparedStatement;
+import org.apache.commons.dbcp2.TestUtils;
import org.apache.commons.dbcp2.datasources.SharedPoolDataSource;
+import org.apache.commons.pool2.impl.DefaultPooledObject;
import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
@@ -52,12 +59,12 @@ public class TestDriverAdapterCPDS {
private static final class ThreadDbcp367 extends Thread {
- private final DataSource ds;
+ private final DataSource dataSource;
private volatile boolean failed;
- public ThreadDbcp367(final DataSource ds) {
- this.ds = ds;
+ public ThreadDbcp367(final DataSource dataSource) {
+ this.dataSource = dataSource;
}
public boolean isFailed() {
@@ -66,11 +73,11 @@ public class TestDriverAdapterCPDS {
@Override
public void run() {
- Connection c = null;
+ Connection conn = null;
try {
- for (int j=0; j < 5000; j++) {
- c = ds.getConnection();
- c.close();
+ for (int j = 0; j < 5000; j++) {
+ conn = dataSource.getConnection();
+ conn.close();
}
} catch (final SQLException sqle) {
failed = true;
@@ -79,6 +86,19 @@ public class TestDriverAdapterCPDS {
}
}
+ @SuppressWarnings("resource")
+ private static void checkAfterClose(final Connection element, final
PStmtKey pStmtKey) throws SQLException {
+ final ConnectionImpl connectionImpl = (ConnectionImpl) element;
+ assertNull(connectionImpl.getInnermostDelegate());
+ assertNotNull(connectionImpl.getInnermostDelegateInternal());
+ final PooledConnectionImpl pooledConnectionImpl =
connectionImpl.getPooledConnectionImpl();
+ assertNotNull(pooledConnectionImpl);
+ // Simulate released resources, should not throw NPEs
+ pooledConnectionImpl.destroyObject(pStmtKey, null);
+ pooledConnectionImpl.destroyObject(pStmtKey, new
DefaultPooledObject<>(null));
+ pooledConnectionImpl.destroyObject(pStmtKey, new
DefaultPooledObject<>(new DelegatingPreparedStatement(null, null)));
+ }
+
private DriverAdapterCPDS pcds;
@BeforeEach
@@ -88,11 +108,40 @@ public class TestDriverAdapterCPDS {
pcds.setUrl("jdbc:apache:commons:testdriver");
pcds.setUser("foo");
pcds.setPassword("bar");
- pcds.setPoolPreparedStatements(false);
+ pcds.setPoolPreparedStatements(true);
+ }
+
+ @Test
+ public void testClose()
+ throws Exception {
+ final Connection[] c = new Connection[10];
+ for (int i = 0; i < c.length; i++) {
+ c[i] = pcds.getPooledConnection().getConnection();
+ }
+
+ // close one of the connections
+ c[0].close();
+ assertTrue(c[0].isClosed());
+ // get a new connection
+ c[0] = pcds.getPooledConnection().getConnection();
+
+ for (final Connection element : c) {
+ element.close();
+ checkAfterClose(element, null);
+ }
+
+ // open all the connections
+ for (int i = 0; i < c.length; i++) {
+ c[i] = pcds.getPooledConnection().getConnection();
+ }
+ for (final Connection element : c) {
+ element.close();
+ checkAfterClose(element, null);
+ }
}
@Test
- public void testClosingWithUserName()
+ public void testCloseWithUserName()
throws Exception {
final Connection[] c = new Connection[10];
for (int i = 0; i < c.length; i++) {
@@ -107,6 +156,7 @@ public class TestDriverAdapterCPDS {
for (final Connection element : c) {
element.close();
+ checkAfterClose(element, null);
}
// open all the connections
@@ -115,6 +165,7 @@ public class TestDriverAdapterCPDS {
}
for (final Connection element : c) {
element.close();
+ checkAfterClose(element, null);
}
}
@@ -366,37 +417,44 @@ public class TestDriverAdapterCPDS {
assertNotNull(conn);
try (final PreparedStatement stmt = conn.prepareStatement("select
* from dual")) {
assertNotNull(stmt);
- try (final ResultSet rset = stmt.executeQuery()) {
- assertNotNull(rset);
- assertTrue(rset.next());
+ try (final ResultSet resultSet = stmt.executeQuery()) {
+ assertNotNull(resultSet);
+ assertTrue(resultSet.next());
}
}
}
}
+ @SuppressWarnings("resource")
@Test
public void testSimpleWithUsername() throws Exception {
+ final Connection connCheck;
+ PStmtKey pStmtKey;
try (final Connection conn = pcds.getPooledConnection("u1",
"p1").getConnection()) {
assertNotNull(conn);
+ connCheck = conn;
try (final PreparedStatement stmt = conn.prepareStatement("select
* from dual")) {
assertNotNull(stmt);
- try (final ResultSet rset = stmt.executeQuery()) {
- assertNotNull(rset);
- assertTrue(rset.next());
+ final DelegatingStatement delegatingStatement =
(DelegatingStatement) stmt;
+ final Statement delegateStatement =
delegatingStatement.getDelegate();
+ pStmtKey = TestUtils.getPStmtKey((PoolablePreparedStatement)
delegateStatement);
+ assertNotNull(pStmtKey);
+ try (final ResultSet resultSet = stmt.executeQuery()) {
+ assertNotNull(resultSet);
+ assertTrue(resultSet.next());
}
}
}
+ checkAfterClose(connCheck, pStmtKey);
}
@Test
- public void testToStringWithoutConnectionProperties() throws
ClassNotFoundException
- {
+ public void testToStringWithoutConnectionProperties() throws
ClassNotFoundException {
final DriverAdapterCPDS cleanCpds = new DriverAdapterCPDS();
- cleanCpds.setDriver( "org.apache.commons.dbcp2.TesterDriver" );
- cleanCpds.setUrl( "jdbc:apache:commons:testdriver" );
- cleanCpds.setUser( "foo" );
- cleanCpds.setPassword( "bar" );
-
+ cleanCpds.setDriver("org.apache.commons.dbcp2.TesterDriver");
+ cleanCpds.setUrl("jdbc:apache:commons:testdriver");
+ cleanCpds.setUser("foo");
+ cleanCpds.setPassword("bar");
cleanCpds.toString();
}
}
