From 736cf260c953e881289070f5db38d787d3265449 Mon Sep 17 00:00:00 2001 From: Mohammed Bekraoui Date: Tue, 21 Nov 2023 12:26:16 +0100 Subject: [PATCH 1/2] DBCP-592: Support request boundaries --- .../commons/dbcp2/DelegatingConnection.java | 47 +++++ .../commons/dbcp2/TestRequestBoundaries.java | 187 ++++++++++++++++++ .../commons/dbcp2/TesterConnection.java | 11 ++ 3 files changed, 245 insertions(+) create mode 100644 src/test/java/org/apache/commons/dbcp2/TestRequestBoundaries.java diff --git a/src/main/java/org/apache/commons/dbcp2/DelegatingConnection.java b/src/main/java/org/apache/commons/dbcp2/DelegatingConnection.java index 9231246a4d..4f875fdea7 100644 --- a/src/main/java/org/apache/commons/dbcp2/DelegatingConnection.java +++ b/src/main/java/org/apache/commons/dbcp2/DelegatingConnection.java @@ -16,6 +16,8 @@ */ package org.apache.commons.dbcp2; +import java.lang.reflect.InvocationTargetException; +import java.lang.reflect.Method; import java.sql.Array; import java.sql.Blob; import java.sql.CallableStatement; @@ -42,6 +44,8 @@ import java.util.concurrent.Executor; import org.apache.commons.dbcp2.managed.ManagedConnection; +import org.apache.commons.logging.Log; +import org.apache.commons.logging.LogFactory; /** * A base delegating implementation of {@link Connection}. @@ -76,6 +80,20 @@ public class DelegatingConnection extends AbandonedTrace i private String cachedCatalog; private String cachedSchema; private Duration defaultQueryTimeoutDuration; + /** + * Logger + */ + private static final Log log = LogFactory.getLog(DelegatingConnection.class); + /** + * Request boundaries + */ + private static final Method beginRequest; + private static final Method endRequest; + + static { + beginRequest = getConnectionMethod("beginRequest"); + endRequest = getConnectionMethod("endRequest"); + } /** * Creates a wrapper for the Connection which traces this Connection in the AbandonedObjectPool. @@ -86,6 +104,21 @@ public DelegatingConnection(final C connection) { this.connection = connection; } + /** + * Uses reflection to get the method form the Connection interface + * @param methodName name of the method to get + * @return the method if it exists, otherwise null + */ + private static Method getConnectionMethod(String methodName) { + Method method = null; + try { + method = Connection.class.getMethod(methodName); + } catch (NoSuchMethodException ex) { + // Ignore exception and set both methods to null + } + return method; + } + @Override public void abort(final Executor executor) throws SQLException { try { @@ -98,6 +131,13 @@ public void abort(final Executor executor) throws SQLException { protected void activate() { closed = false; setLastUsed(); + if (beginRequest != null && connection != null) { + try { + beginRequest.invoke(connection); + } catch (InvocationTargetException | IllegalAccessException ex) { + log.warn("Error calling beginRequest on connection", ex); + } + } if (connection instanceof DelegatingConnection) { ((DelegatingConnection) connection).activate(); } @@ -662,6 +702,13 @@ protected void passivate() throws SQLException { throw new SQLExceptionList(thrownList); } } + if (endRequest != null && connection != null) { + try { + endRequest.invoke(connection); + } catch (InvocationTargetException | IllegalAccessException ex) { + log.warn("Error calling endRequest on connection", ex); + } + } setLastUsed(Instant.EPOCH); } diff --git a/src/test/java/org/apache/commons/dbcp2/TestRequestBoundaries.java b/src/test/java/org/apache/commons/dbcp2/TestRequestBoundaries.java new file mode 100644 index 0000000000..42082a015b --- /dev/null +++ b/src/test/java/org/apache/commons/dbcp2/TestRequestBoundaries.java @@ -0,0 +1,187 @@ +/* + * 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.dbcp2; + +import org.junit.jupiter.api.Test; +import org.mockito.stubbing.OngoingStubbing; + +import java.sql.Connection; +import java.sql.Driver; +import java.sql.SQLException; +import java.util.ArrayList; +import java.util.List; +import java.util.Properties; + +import static org.junit.jupiter.api.Assumptions.assumeTrue; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.isNull; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; +import static org.mockito.Mockito.reset; + +public class TestRequestBoundaries { + Driver driver = mock(TesterDriver.class); + + @Test + public void testBeginRequestOneConnection() throws SQLException { + // Verify JDK version + assumeTrue(Double.valueOf(System.getProperty("java.class.version")) >= 53); + + // Setup + BasicDataSource dataSource = getDataSource(driver); + TesterConnection connection = setupPhysicalConnections(1).get(0); + + // Get connection + dataSource.getConnection(); + + // Verify number of calls + assertCallCount(connection, 1, 0); + } + + @Test + public void testEndRequestOneConnection() throws SQLException { + // Verify JDK version + assumeTrue(Double.valueOf(System.getProperty("java.class.version")) >= 53); + + // Setup + BasicDataSource dataSource = getDataSource(driver); + TesterConnection connection = setupPhysicalConnections(1).get(0); + + // Get then close connection + dataSource.getConnection().close(); + + // Verify number of calls + assertCallCount(connection, 1, 1); + } + + @Test + public void testBeginRequestTwoVirtualConnections() throws SQLException { + // Verify JDK version + assumeTrue(Double.valueOf(System.getProperty("java.class.version")) >= 53); + + // Setup + BasicDataSource dataSource = getDataSource(driver); + TesterConnection connection = setupPhysicalConnections(1).get(0); + + // Get connection close it then get another connection + dataSource.getConnection().close(); + dataSource.getConnection(); + + // Verify number calls + assertCallCount(connection, 2, 1); + } + + @Test + public void testEndRequestTwoVirtualConnections() throws SQLException { + // Verify JDK version + assumeTrue(Double.valueOf(System.getProperty("java.class.version")) >= 53); + + // Setup + BasicDataSource dataSource = getDataSource(driver); + TesterConnection connection = setupPhysicalConnections(1).get(0); + + // Get a connection and close then get another connection and close it + dataSource.getConnection().close(); + dataSource.getConnection().close(); + + // Verify number of calls + assertCallCount(connection, 2, 2); + } + + @Test + public void testRequestBoundariesTwoPhysicalConnections() throws SQLException { + // Verify JDK version + assumeTrue(Double.valueOf(System.getProperty("java.class.version")) >= 53); + + // Setup + BasicDataSource dataSource = getDataSource(driver); + List connections = setupPhysicalConnections(2); + + // Get a connection, then get another connection, then close the first connection + Connection fetchedConnection = dataSource.getConnection(); + dataSource.getConnection(); + fetchedConnection.close(); + + // Verify number of calls + assertCallCount(connections.get(0), 1, 1); + assertCallCount(connections.get(1), 1, 0); + } + + @Test + public void testConnectionWithoutRequestBoundaries() throws SQLException { + // Verify JDK version + assumeTrue(Double.valueOf(System.getProperty("java.class.version")) < 53); + + // Setup + BasicDataSource dataSource = getDataSource(driver); + TesterConnection connection = setupPhysicalConnections(1).get(0); + + // Get connection + dataSource.getConnection().close(); + + assertCallCount(connection, 0, 0); + } + + public BasicDataSource getDataSource(Driver driver) throws SQLException { + reset(driver); + + BasicDataSource dataSource = BasicDataSourceFactory.createDataSource(new Properties()); + dataSource.setDriver(driver); + + // Before testing the call count of beginRequest and endRequest method we'll make sure that the + // connectionFactory has been validated which involves creating a physical connection and destroying it. If we + // don't, it's going to be done automatically when the first connection is requested. This is going to mess the + // call count. + validateConnectionFactory(dataSource, driver); + + return dataSource; + } + + public void validateConnectionFactory(BasicDataSource dataSource, Driver driver) throws SQLException { + when(driver.connect(isNull(), any(Properties.class))).thenReturn(getTesterConnection()); + dataSource.getLogWriter(); + } + + public TesterConnection getTesterConnection() throws SQLException { + TesterConnection connection = new TesterConnection(null, null); + + return connection; + } + + public List setupPhysicalConnections(int numOfConnections) throws SQLException { + List listOfConnections = new ArrayList<>(); + + for (int i = 0; i < numOfConnections; i++) { + listOfConnections.add(getTesterConnection()); + } + + OngoingStubbing ongoingStubbing = when(driver.connect(isNull(), any(Properties.class))); + + for (Connection connection : listOfConnections) { + ongoingStubbing = ongoingStubbing.thenReturn(connection); + } + return listOfConnections; + } + + public void assertCallCount(TesterConnection connection, int expectedBeginRequestCalls, int expectedEndRequestCalls) + throws SQLException { + assertEquals(expectedBeginRequestCalls, connection.beginRequestCount.get()); + assertEquals(expectedEndRequestCalls, connection.endRequestCount.get()); + } +} diff --git a/src/test/java/org/apache/commons/dbcp2/TesterConnection.java b/src/test/java/org/apache/commons/dbcp2/TesterConnection.java index 0712276668..4ca17eecf2 100644 --- a/src/test/java/org/apache/commons/dbcp2/TesterConnection.java +++ b/src/test/java/org/apache/commons/dbcp2/TesterConnection.java @@ -34,6 +34,7 @@ import java.util.Map; import java.util.Properties; import java.util.concurrent.Executor; +import java.util.concurrent.atomic.AtomicInteger; /** * A dummy {@link Connection}, for testing purposes. @@ -53,6 +54,8 @@ public class TesterConnection extends AbandonedTrace implements Connection { protected final String userName; protected Exception failure; protected boolean sqlExceptionOnClose; + public AtomicInteger beginRequestCount = new AtomicInteger(0); + public AtomicInteger endRequestCount = new AtomicInteger(0); TesterConnection(final String userName, @SuppressWarnings("unused") final String password) { @@ -433,4 +436,12 @@ public void setWarnings(final SQLWarning warning) { public T unwrap(final Class iface) throws SQLException { throw new SQLException("Not implemented."); } + + public void beginRequest() { + beginRequestCount.incrementAndGet(); + } + + public void endRequest() { + endRequestCount.incrementAndGet(); + } } From 9913c942859b35eec566a0bb0894d32fec229ead Mon Sep 17 00:00:00 2001 From: Mohammed Bekraoui Date: Wed, 6 Dec 2023 10:16:20 +0100 Subject: [PATCH 2/2] Call beginRequest on innerMostDelegate --- .../commons/dbcp2/DelegatingConnection.java | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/src/main/java/org/apache/commons/dbcp2/DelegatingConnection.java b/src/main/java/org/apache/commons/dbcp2/DelegatingConnection.java index 4f875fdea7..27f3823c2d 100644 --- a/src/main/java/org/apache/commons/dbcp2/DelegatingConnection.java +++ b/src/main/java/org/apache/commons/dbcp2/DelegatingConnection.java @@ -131,15 +131,17 @@ public void abort(final Executor executor) throws SQLException { protected void activate() { closed = false; setLastUsed(); - if (beginRequest != null && connection != null) { - try { - beginRequest.invoke(connection); - } catch (InvocationTargetException | IllegalAccessException ex) { - log.warn("Error calling beginRequest on connection", ex); - } - } + if (connection instanceof DelegatingConnection) { ((DelegatingConnection) connection).activate(); + } else { + if (beginRequest != null && connection != null) { + try { + beginRequest.invoke(connection); + } catch (InvocationTargetException | IllegalAccessException ex) { + log.warn("Error calling beginRequest on connection", ex); + } + } } }