Mailing-List: contact commits-help@tomee.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@tomee.apache.org
Content-Type: text/plain; charset="us-ascii"
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
From: jgallimore@apache.org
To: commits@tomee.apache.org
Date: Thu, 11 Aug 2016 00:29:41 -0000
Message-Id: <fc0ef2a6a1fd4bbeba4252ecb2111b9c@git.apache.org>
In-Reply-To: <4d58d56c076749239737aa9454427995@git.apache.org>
References: <4d58d56c076749239737aa9454427995@git.apache.org>
Subject: [6/8] tomee git commit: TOMEE-1643 ensure to close the XAConnection
 to not leak connection or break the pool
archived-at: Thu, 11 Aug 2016 00:29:38 -0000

TOMEE-1643 ensure to close the XAConnection to not leak connection or break the pool


Project: http://git-wip-us.apache.org/repos/asf/tomee/repo
Commit: http://git-wip-us.apache.org/repos/asf/tomee/commit/5cf2bc8f
Tree: http://git-wip-us.apache.org/repos/asf/tomee/tree/5cf2bc8f
Diff: http://git-wip-us.apache.org/repos/asf/tomee/diff/5cf2bc8f

Branch: refs/heads/tomee-1.7.x
Commit: 5cf2bc8fb7ed6464d870b1f9af258ab9885b5a92
Parents: 9e19d4a
Author: Romain Manni-Bucau <rmannibu@gmail.com>
Authored: Thu Oct 22 12:29:59 2015 +0200
Committer: Jonathan Gallimore <jon@jrg.me.uk>
Committed: Thu Aug 11 00:20:52 2016 +0100

----------------------------------------------------------------------
 .../jdbc/managed/local/ManagedConnection.java   | 83 ++++++++------------
 .../tomee/jdbc/TomcatXADataSourceTest.java      | 59 ++++++++------
 2 files changed, 69 insertions(+), 73 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/tomee/blob/5cf2bc8f/container/openejb-core/src/main/java/org/apache/openejb/resource/jdbc/managed/local/ManagedConnection.java
----------------------------------------------------------------------
diff --git a/container/openejb-core/src/main/java/org/apache/openejb/resource/jdbc/managed/local/ManagedConnection.java b/container/openejb-core/src/main/java/org/apache/openejb/resource/jdbc/managed/local/ManagedConnection.java
index bb24f0f..5b03dd5 100644
--- a/container/openejb-core/src/main/java/org/apache/openejb/resource/jdbc/managed/local/ManagedConnection.java
+++ b/container/openejb-core/src/main/java/org/apache/openejb/resource/jdbc/managed/local/ManagedConnection.java
@@ -20,14 +20,6 @@ package org.apache.openejb.resource.jdbc.managed.local;
 import org.apache.openejb.util.LogCategory;
 import org.apache.openejb.util.Logger;
 
-import java.lang.reflect.InvocationHandler;
-import java.lang.reflect.InvocationTargetException;
-import java.lang.reflect.Method;
-import java.lang.reflect.Proxy;
-import java.sql.Connection;
-import java.sql.SQLException;
-import java.sql.Wrapper;
-
 import javax.sql.CommonDataSource;
 import javax.sql.DataSource;
 import javax.sql.XAConnection;
@@ -40,6 +32,12 @@ import javax.transaction.Transaction;
 import javax.transaction.TransactionManager;
 import javax.transaction.TransactionSynchronizationRegistry;
 import javax.transaction.xa.XAResource;
+import java.lang.reflect.InvocationHandler;
+import java.lang.reflect.InvocationTargetException;
+import java.lang.reflect.Method;
+import java.sql.Connection;
+import java.sql.SQLException;
+import java.sql.Wrapper;
 
 public class ManagedConnection implements InvocationHandler {
     private final TransactionManager transactionManager;
@@ -98,6 +96,14 @@ public class ManagedConnection implements InvocationHandler {
 
             // shouldn't be used without a transaction but if so just delegate to the actual connection
             if (transaction == null) {
+                if ("close".equals(mtdName)) {
+                    if (delegate == null) { // no need to get a connection
+                        return null;
+                    }
+
+                    closeConnection(xaConnection, delegate);
+                    return null;
+                }
                 if (delegate == null) {
                     newConnection();
                 }
@@ -129,7 +135,7 @@ public class ManagedConnection implements InvocationHandler {
                         throw new SQLException("Unable to enlist connection the transaction", e);
                     }
 
-                    transaction.registerSynchronization(new ClosingSynchronization(delegate));
+                    transaction.registerSynchronization(new ClosingSynchronization(xaConnection, delegate));
 
                     try {
                         setAutoCommit(false);
@@ -165,7 +171,7 @@ public class ManagedConnection implements InvocationHandler {
                 (key.user == null ? XADataSource.class.cast(key.ds).getXAConnection() : XADataSource.class.cast(key.ds).getXAConnection(key.user, key.pwd));
         if (XAConnection.class.isInstance(connection)) {
             xaConnection = XAConnection.class.cast(connection);
-            delegate = wrapDelegate(xaConnection, xaConnection.getConnection());
+            delegate = xaConnection.getConnection();
             xaResource = xaConnection.getXAResource();
         } else {
             delegate = Connection.class.cast(connection);
@@ -174,13 +180,6 @@ public class ManagedConnection implements InvocationHandler {
         return connection;
     }
 
-    private Connection wrapDelegate(final XAConnection xaConnection, final Connection connection) {
-        return (Connection) Proxy.newProxyInstance(
-                Thread.currentThread().getContextClassLoader(),
-                new Class<?>[] { Connection.class },
-                new XAConnectionWrapper(xaConnection, connection));
-    }
-
     protected void setAutoCommit(final boolean value) throws SQLException {
         if (delegate == null) {
             newConnection();
@@ -228,21 +227,13 @@ public class ManagedConnection implements InvocationHandler {
         return new SQLException("can't call " + mtdName + " when the connection is JtaManaged");
     }
 
-    private static void close(final Connection connection) {
-        try {
-            if (!connection.isClosed()) {
-                connection.close();
-            }
-        } catch (final SQLException e) {
-            // no-op
-        }
-    }
-
     private static class ClosingSynchronization implements Synchronization {
+        private final XAConnection xaConnection;
         private final Connection connection;
 
-        public ClosingSynchronization(final Connection connection) {
-            this.connection = connection;
+        public ClosingSynchronization(final XAConnection xaConnection, final Connection delegate) {
+            this.xaConnection = xaConnection;
+            this.connection = delegate;
         }
 
         @Override
@@ -252,7 +243,19 @@ public class ManagedConnection implements InvocationHandler {
 
         @Override
         public void afterCompletion(final int status) {
-            close(connection);
+            closeConnection(xaConnection, connection);
+        }
+    }
+
+    private static void closeConnection(final XAConnection xaConnection, final Connection connection) {
+        try {
+            if (xaConnection != null) { // handles the underlying connection
+                xaConnection.close();
+            } else if (connection != null && !connection.isClosed()) {
+                connection.close();
+            }
+        } catch (final SQLException e) {
+            // no-op
         }
     }
 
@@ -293,24 +296,4 @@ public class ManagedConnection implements InvocationHandler {
             return hash;
         }
     }
-
-    private class XAConnectionWrapper implements InvocationHandler {
-        private final XAConnection xaConnection;
-        private final Connection delegate;
-
-        public XAConnectionWrapper(final XAConnection xaConnection, final Connection delegate) {
-            this.xaConnection = xaConnection;
-            this.delegate = delegate;
-        }
-
-        @Override
-        public Object invoke(final Object proxy, final Method method, final Object[] args) throws Throwable {
-            if ("close".equals(method.getName()) && (args == null || args.length == 0)) {
-                xaConnection.close();
-                return null;
-            } else {
-                return method.invoke(delegate, args);
-            }
-        }
-    }
 }

http://git-wip-us.apache.org/repos/asf/tomee/blob/5cf2bc8f/tomee/tomee-jdbc/src/test/java/org/apache/tomee/jdbc/TomcatXADataSourceTest.java
----------------------------------------------------------------------
diff --git a/tomee/tomee-jdbc/src/test/java/org/apache/tomee/jdbc/TomcatXADataSourceTest.java b/tomee/tomee-jdbc/src/test/java/org/apache/tomee/jdbc/TomcatXADataSourceTest.java
index 68892e5..5863384 100644
--- a/tomee/tomee-jdbc/src/test/java/org/apache/tomee/jdbc/TomcatXADataSourceTest.java
+++ b/tomee/tomee-jdbc/src/test/java/org/apache/tomee/jdbc/TomcatXADataSourceTest.java
@@ -18,6 +18,7 @@ package org.apache.tomee.jdbc;
 
 import org.apache.openejb.jee.EjbJar;
 import org.apache.openejb.junit.ApplicationComposer;
+import org.apache.openejb.resource.jdbc.managed.local.ManagedDataSource;
 import org.apache.openejb.testing.Configuration;
 import org.apache.openejb.testing.Module;
 import org.apache.openejb.testng.PropertiesBuilder;
@@ -27,16 +28,11 @@ import org.junit.Test;
 import org.junit.runner.RunWith;
 
 import javax.annotation.Resource;
-import javax.management.AttributeNotFoundException;
-import javax.management.InstanceNotFoundException;
-import javax.management.MBeanException;
-import javax.management.MBeanServer;
-import javax.management.MalformedObjectNameException;
-import javax.management.ObjectName;
-import javax.management.ReflectionException;
 import javax.sql.DataSource;
-import java.lang.management.ManagementFactory;
 import java.sql.Connection;
+import java.sql.SQLException;
+import java.util.ArrayList;
+import java.util.Collection;
 import java.util.Properties;
 
 import static org.hamcrest.CoreMatchers.instanceOf;
@@ -46,8 +42,6 @@ import static org.junit.Assert.assertThat;
 
 @RunWith(ApplicationComposer.class)
 public class TomcatXADataSourceTest {
-    private static final MBeanServer server = ManagementFactory.getPlatformMBeanServer();
-
     @Resource(name = "xadb")
     private DataSource ds;
 
@@ -65,36 +59,55 @@ public class TomcatXADataSourceTest {
             .p("txMgr.txRecovery", "true")
             .p("txMgr.logFileDir", "target/test/xa/howl")
 
-                // real XA datasources
+            // real XA datasources
             .p("xa", "new://Resource?class-name=" + JDBCXADataSource.class.getName())
             .p("xa.url", "jdbc:hsqldb:mem:tomcat-xa")
             .p("xa.user", "sa")
             .p("xa.password", "")
             .p("xa.SkipImplicitAttributes", "true")
+            .p("xa.SkipPropertiesFallback", "true") // otherwise goes to connection properties
 
             .p("xadb", "new://Resource?type=DataSource")
             .p("xadb.xaDataSource", "xa")
             .p("xadb.JtaManaged", "true")
+            .p("xadb.MaxIdle", "25")
+            .p("xadb.MaxActive", "25")
+            .p("xadb.InitialSize", "3")
 
             .build();
     }
 
     @Test
-    public void check() throws Exception {
+    public void check() throws SQLException {
         assertNotNull(ds);
-        final Connection c = ds.getConnection();
-        assertNotNull(c);
-        assertThat(c.getMetaData().getConnection(), instanceOf(JDBCXAConnectionWrapper.class));
-        c.close();
+        final TomEEDataSourceCreator.TomEEDataSource tds = TomEEDataSourceCreator.TomEEDataSource.class.cast(ManagedDataSource.class.cast(ds).getDelegate());
 
-        assertEquals(0, getActiveConnections("xadb"));
-    }
+        assertEquals(3, tds.getIdle()); // InitSize
+
+        try (final Connection c = ds.getConnection()) {
+            assertNotNull(c);
+
+            final Connection connection = c.getMetaData().getConnection(); // just to do something and force the connection init
+            assertThat(connection, instanceOf(JDBCXAConnectionWrapper.class));
+        } // here we close the connection so we are back in the initial state
 
+        assertEquals(0, tds.getActive());
+        assertEquals(3, tds.getIdle());
 
-    private int getActiveConnections(final String dataSourceName)
-            throws MalformedObjectNameException, MBeanException, AttributeNotFoundException, InstanceNotFoundException, ReflectionException {
-        final ObjectName objectName = new ObjectName("openejb.management:ObjectType=datasources,DataSource=" + dataSourceName);
-        final Object activeConnectionsAttribute = server.getAttribute(objectName, "Active");
-        return (int) (Integer) activeConnectionsAttribute;
+        for (int it = 0; it < 5; it++) { // ensures it always works and not only the first time
+            final Collection<Connection> connections = new ArrayList<>(25);
+            for (int i = 0; i < 25; i++) {
+                final Connection connection = ds.getConnection();
+                connections.add(connection);
+                connection.getMetaData(); // trigger connection retrieving otherwise nothing is done (pool is not used)
+            }
+            assertEquals(25, tds.getActive());
+            assertEquals(0, tds.getIdle());
+            for (final Connection toClose : connections) {
+                toClose.close();
+            }
+            assertEquals(0, tds.getActive());
+            assertEquals(25, tds.getIdle());
+        }
     }
 }