tomcat-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From ma...@apache.org
Subject [tomcat] 03/04: Fix SpotBugs issues in JDBC pool tests
Date Wed, 20 Nov 2019 13:24:52 GMT
This is an automated email from the ASF dual-hosted git repository.

markt pushed a commit to branch 8.5.x
in repository https://gitbox.apache.org/repos/asf/tomcat.git

View the commit online:
https://github.com/apache/tomcat/commit/c4c14207035ac95c99f7c2da1dc326e3e989efa6

commit c4c14207035ac95c99f7c2da1dc326e3e989efa6
Author: Mark Thomas <markt@apache.org>
AuthorDate: Wed Nov 20 13:21:51 2019 +0000

    Fix SpotBugs issues in JDBC pool tests
---
 .../java/org/apache/tomcat/jdbc/bugs/Bug53367.java |  6 +--
 .../apache/tomcat/jdbc/test/ConnectCountTest.java  |  4 +-
 .../org/apache/tomcat/jdbc/test/FairnessTest.java  |  4 +-
 .../apache/tomcat/jdbc/test/JmxPasswordTest.java   |  2 +-
 .../apache/tomcat/jdbc/test/MultipleCloseTest.java |  4 +-
 .../apache/tomcat/jdbc/test/StarvationTest.java    |  2 +
 .../org/apache/tomcat/jdbc/test/TestException.java | 12 ++---
 .../tomcat/jdbc/test/TestStatementCache.java       |  2 +
 .../org/apache/tomcat/jdbc/test/TestTimeout.java   | 26 +++++++----
 .../jdbc/test/TestValidationQueryTimeout.java      | 16 +++----
 res/findbugs/filter-false-positives.xml            | 51 ++++++++++++++++++++++
 11 files changed, 99 insertions(+), 30 deletions(-)

diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/bugs/Bug53367.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/bugs/Bug53367.java
index 1e534e0..6c0984e 100644
--- a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/bugs/Bug53367.java
+++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/bugs/Bug53367.java
@@ -107,8 +107,8 @@ public class Bug53367 {
             Thread thread = new Thread(new Runnable() {
                 @Override
                 public void run() {
-                    try {
-                        ds.getConnection();
+                    // Expected to fail
+                    try (Connection c = ds.getConnection()) {
                     } catch (Exception e) {
                         System.err.println("Step 2:"+e.getMessage());
                     }
@@ -174,4 +174,4 @@ public class Bug53367 {
         Assert.assertEquals(0, pool.getActive());
         Assert.assertEquals(threadsCount, pool.getSize());
     }
-}
\ No newline at end of file
+}
diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/test/ConnectCountTest.java
b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/test/ConnectCountTest.java
index 5a0a71f..8e240f9 100644
--- a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/test/ConnectCountTest.java
+++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/test/ConnectCountTest.java
@@ -17,7 +17,9 @@
 package org.apache.tomcat.jdbc.test;
 
 import java.sql.Connection;
+import java.sql.SQLException;
 import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.ExecutionException;
 import java.util.concurrent.Future;
 import java.util.concurrent.TimeUnit;
 
@@ -259,7 +261,7 @@ public class ConnectCountTest extends DefaultTestCase {
                     totalruntime+=(System.nanoTime()-start);
                 }
 
-            } catch (Exception x) {
+            } catch (RuntimeException | SQLException | ExecutionException | InterruptedException
x) {
                 x.printStackTrace();
             } finally {
                 ConnectCountTest.this.latch.countDown();
diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/test/FairnessTest.java
b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/test/FairnessTest.java
index 1c282df..bb2cc8b 100644
--- a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/test/FairnessTest.java
+++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/test/FairnessTest.java
@@ -17,7 +17,9 @@
 package org.apache.tomcat.jdbc.test;
 
 import java.sql.Connection;
+import java.sql.SQLException;
 import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.ExecutionException;
 import java.util.concurrent.Future;
 import java.util.concurrent.TimeUnit;
 
@@ -241,7 +243,7 @@ public class FairnessTest extends DefaultTestCase {
                     totalruntime+=(System.nanoTime()-start);
                 }
 
-            } catch (Exception x) {
+            } catch (RuntimeException | SQLException | ExecutionException | InterruptedException
x) {
                 x.printStackTrace();
             } finally {
                 FairnessTest.this.latch.countDown();
diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/test/JmxPasswordTest.java
b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/test/JmxPasswordTest.java
index 2074447..6c05c00 100644
--- a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/test/JmxPasswordTest.java
+++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/test/JmxPasswordTest.java
@@ -36,7 +36,7 @@ import org.apache.tomcat.jdbc.test.driver.Driver;
 public class JmxPasswordTest extends DefaultTestCase{
     public static final String password = "password";
     public static final String username = "username";
-    public static ObjectName oname = null;
+    public ObjectName oname = null;
 
     @Before
     public void setUp() throws Exception {
diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/test/MultipleCloseTest.java
b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/test/MultipleCloseTest.java
index cd867d3..4180c51 100644
--- a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/test/MultipleCloseTest.java
+++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/test/MultipleCloseTest.java
@@ -61,9 +61,11 @@ public class MultipleCloseTest extends DefaultTestCase {
         Assert.assertTrue(con1.isClosed());
 
         // Open a new connection (This will re-use the previous pooled connection)
-        datasource.getConnection();
+        Connection con2 = datasource.getConnection();
 
         // A connection, once closed, should stay closed
         Assert.assertTrue(con1.isClosed());
+
+        con2.close();
     }
 }
diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/test/StarvationTest.java
b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/test/StarvationTest.java
index 0c1a667..1c551ed 100644
--- a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/test/StarvationTest.java
+++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/test/StarvationTest.java
@@ -82,6 +82,7 @@ public class StarvationTest extends DefaultTestCase {
         }finally {
             if (con2!=null) con2.close();
         }
+        con1.close();
     }
 
     @Test
@@ -104,5 +105,6 @@ public class StarvationTest extends DefaultTestCase {
         }finally {
             if (con2!=null) con2.close();
         }
+        con1.close();
     }
 }
diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/test/TestException.java
b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/test/TestException.java
index be06e38..fe4d20d 100644
--- a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/test/TestException.java
+++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/test/TestException.java
@@ -17,7 +17,9 @@
 package org.apache.tomcat.jdbc.test;
 
 import java.sql.Connection;
+import java.sql.Statement;
 
+import org.junit.Assert;
 import org.junit.Test;
 
 import org.apache.tomcat.jdbc.pool.ConnectionPool;
@@ -30,11 +32,11 @@ public class TestException extends DefaultTestCase {
     public void testException() throws Exception {
         datasource.getPoolProperties().setJdbcInterceptors(TestInterceptor.class.getName());
         Connection con = datasource.getConnection();
-        try {
-            con.createStatement();
-        }catch (Exception x) {
-            // Ignore
+        try (Statement s = con.createStatement()){
+        } catch (Exception x) {
+            Assert.fail();
         }
+        con.close();
     }
 
 
@@ -42,7 +44,7 @@ public class TestException extends DefaultTestCase {
 
         @Override
         public void reset(ConnectionPool parent, PooledConnection con) {
-            // TODO Auto-generated method stub
+            // NO-OP
         }
     }
 }
diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/test/TestStatementCache.java
b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/test/TestStatementCache.java
index 3a8778a..a80962c 100644
--- a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/test/TestStatementCache.java
+++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/test/TestStatementCache.java
@@ -88,6 +88,7 @@ public class TestStatementCache extends DefaultTestCase {
         ps3.close();
         Assert.assertTrue(ps3.isClosed());
         Assert.assertEquals(1,interceptor.getCacheSize().get());
+        con.close();
     }
 
     @Test
@@ -108,6 +109,7 @@ public class TestStatementCache extends DefaultTestCase {
         ps3.close();
         Assert.assertTrue(ps3.isClosed());
         Assert.assertEquals(0,interceptor.getCacheSize().get());
+        con.close();
     }
 
     @Test
diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/test/TestTimeout.java
b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/test/TestTimeout.java
index 9b9fbde..aec85b6 100644
--- a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/test/TestTimeout.java
+++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/test/TestTimeout.java
@@ -16,17 +16,18 @@
  */
 package org.apache.tomcat.jdbc.test;
 
-import java.util.concurrent.atomic.AtomicInteger;
+import java.sql.Connection;
+import java.util.HashSet;
+import java.util.Set;
 
 import org.junit.Assert;
 import org.junit.Test;
 
 public class TestTimeout extends DefaultTestCase {
 
-    AtomicInteger counter = new AtomicInteger(0);
-
     @Test
     public void testCheckoutTimeout() throws Exception {
+        Set<Connection> cons = new HashSet<>();
         try {
             this.datasource.getPoolProperties().setTestWhileIdle(true);
             this.datasource.getPoolProperties().setTestOnBorrow(false);
@@ -42,20 +43,24 @@ public class TestTimeout extends DefaultTestCase {
             System.out.println("About to test connection pool:"+datasource);
             for (int i = 0; i < 21; i++) {
                 long now = System.currentTimeMillis();
-                this.datasource.getConnection();
+                cons.add(this.datasource.getConnection());
                 long delta = System.currentTimeMillis()-now;
                 System.out.println("Got connection #"+i+" in "+delta+" ms.");
             }
-            Assert.assertTrue(false);
+            Assert.fail();
         } catch ( Exception x ) {
-            Assert.assertTrue(true);
+            // Expected on 21st checkout
         }finally {
             Thread.sleep(2000);
         }
+        for (Connection c : cons) {
+            c.close();
+        }
     }
 
     @Test
     public void testCheckoutTimeoutFair() throws Exception {
+        Set<Connection> cons = new HashSet<>();
         try {
             this.datasource.getPoolProperties().setFairQueue(true);
             this.datasource.getPoolProperties().setTestWhileIdle(true);
@@ -72,15 +77,18 @@ public class TestTimeout extends DefaultTestCase {
             System.out.println("About to test connection pool:"+datasource);
             for (int i = 0; i < 21; i++) {
                 long now = System.currentTimeMillis();
-                this.datasource.getConnection();
+                cons.add(this.datasource.getConnection());
                 long delta = System.currentTimeMillis()-now;
                 System.out.println("Got connection #"+i+" in "+delta+" ms.");
             }
-            Assert.assertTrue(false);
+            Assert.fail();
         } catch ( Exception x ) {
-            Assert.assertTrue(true);
+            // Expected on 21st checkout
         }finally {
             Thread.sleep(2000);
         }
+        for (Connection c : cons) {
+            c.close();
+        }
     }
 }
diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/test/TestValidationQueryTimeout.java
b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/test/TestValidationQueryTimeout.java
index 79e2fdc..054df27 100644
--- a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/test/TestValidationQueryTimeout.java
+++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/test/TestValidationQueryTimeout.java
@@ -35,7 +35,7 @@ import org.apache.tomcat.jdbc.pool.interceptor.QueryTimeoutInterceptor;
 
 public class TestValidationQueryTimeout extends DefaultTestCase {
 
-    private static int TIMEOUT = 10;
+    private static final int TIMEOUT = 10;
     private static boolean isTimeoutSet;
     private static final String longQuery = "select * from test as A, test as B, test as
C, test as D, test as E";
 
@@ -54,7 +54,6 @@ public class TestValidationQueryTimeout extends DefaultTestCase {
         this.datasource.setValidationQuery("SELECT 1");
         this.datasource.setValidationQueryTimeout(TIMEOUT);
 
-        TIMEOUT = 10;
         isTimeoutSet = false;
     }
 
@@ -67,8 +66,9 @@ public class TestValidationQueryTimeout extends DefaultTestCase {
     @Test
     public void testValidationQueryTimeoutEnabled() throws Exception {
         // because testOnBorrow is true, this triggers the validation query
-        this.datasource.getConnection();
+        Connection con = this.datasource.getConnection();
         Assert.assertTrue(isTimeoutSet);
+        con.close();
     }
 
     @Test
@@ -76,8 +76,9 @@ public class TestValidationQueryTimeout extends DefaultTestCase {
         this.datasource.setValidationQueryTimeout(-1);
 
         // because testOnBorrow is true, this triggers the validation query
-        this.datasource.getConnection();
+        Connection con = this.datasource.getConnection();
         Assert.assertFalse(isTimeoutSet);
+        con.close();
     }
 
     @Test
@@ -91,9 +92,6 @@ public class TestValidationQueryTimeout extends DefaultTestCase {
         Connection con = this.datasource.getConnection();
         Assert.assertTrue(isTimeoutSet);
 
-        // increase the expected timeout to 30, which is what we set for the interceptor
-        TIMEOUT = 30;
-
         // now create a statement, make sure the query timeout is set by the interceptor
         Statement st = con.createStatement();
         Assert.assertEquals(interceptorTimeout, st.getQueryTimeout());
@@ -107,10 +105,10 @@ public class TestValidationQueryTimeout extends DefaultTestCase {
         con.close();
 
         // pull another connection and check it
-        TIMEOUT = 10;
         isTimeoutSet = false;
-        this.datasource.getConnection();
+        Connection con2 = this.datasource.getConnection();
         Assert.assertTrue(isTimeoutSet);
+        con2.close();
     }
 
     // this test depends on the execution time of the validation query
diff --git a/res/findbugs/filter-false-positives.xml b/res/findbugs/filter-false-positives.xml
index 73443d6..06f104e 100644
--- a/res/findbugs/filter-false-positives.xml
+++ b/res/findbugs/filter-false-positives.xml
@@ -1898,6 +1898,15 @@
     <Bug code="ST" />
   </Match>
   <Match>
+    <!-- The name shadowing is deliberate -->
+    <Or>
+      <Class name="org.apache.tomcat.jdbc.test.driver.Connection" />
+      <Class name="org.apache.tomcat.jdbc.test.driver.Driver" />
+      <Class name="org.apache.tomcat.jdbc.test.driver.ResultSet" />
+    </Or>
+    <Bug pattern="NM_SAME_SIMPLE_NAME_AS_INTERFACE" />
+  </Match>
+  <Match>
     <!-- The call with the ignored return value is used to ensure the pool -->
     <!-- thinks the connection is being used.                              -->
     <Class name="org.apache.tomcat.jdbc.test.AbandonPercentageTest" />
@@ -1905,6 +1914,32 @@
     <Bug pattern="RV_RETURN_VALUE_IGNORED" />
   </Match>
   <Match>
+    <!-- A number of the tests incude performance tests -->
+    <Class name="org.apache.tomcat.jdbc.test.DefaultTestCase" />
+    <Method name="tearDown" />
+    <Bug pattern="DM_GC" />
+  </Match>
+  <Match>
+    <!-- Test does not explicitly close statement deliberately -->
+    <Class name="org.apache.tomcat.jdbc.test.StatementFinalizerTest" />
+    <Method name="testStatementFinalization" />
+    <Bug pattern="ODR_OPEN_DATABASE_RESOURCE"/>
+  </Match>
+  <Match>
+    <!-- Choice of name is deliberate -->
+    <Class name="org.apache.tomcat.jdbc.test.TestException" />
+    <Bug pattern="NM_CLASS_NOT_EXCEPTION" />
+  </Match>
+  <Match>
+    <!-- Testing auto-close so connections not explicitly closed -->
+    <Class name="org.apache.tomcat.jdbc.test.TestGCClose" />
+    <Or>
+      <Method name="testGCStop" />
+      <Method name="testClose" />
+    </Or>
+    <Bug pattern="ODR_OPEN_DATABASE_RESOURCE" />
+   </Match>
+  <Match>
     <!-- SQL is from config so is considered safe -->
     <Class name="org.apache.tomcat.jdbc.test.TestSlowQueryReport" />
     <Method name="testFastSql" />
@@ -1923,6 +1958,22 @@
     <Bug pattern="SQL_PREPARED_STATEMENT_GENERATED_FROM_NONCONSTANT_STRING" />
   </Match>
   <Match>
+    <!-- Tests throw exceptions so connections are never created -->
+    <Class name="org.apache.tomcat.jdbc.test.TestValidationQueryTimeout" />
+    <Or>
+      <Method name="testValidationQueryTimeoutOnConnection" />
+      <Method name="testValidationInvalidOnConnection" />
+      <Method name="testValidationQueryTimeoutOnBorrow" />
+    </Or>
+    <Bug pattern="ODR_OPEN_DATABASE_RESOURCE" />
+  </Match>
+  <Match>
+    <!-- Statics used to work around API limitations -->
+    <Class name="org.apache.tomcat.jdbc.test.TestValidationQueryTimeout" />
+    <Field name="isTimeoutSet" />
+    <Bug pattern="ST_WRITE_TO_STATIC_FROM_INSTANCE_METHOD" />
+  </Match>
+  <Match>
     <Class name="org.apache.tomcat.jdbc.test.TwoDataSources" />
     <Method name="testTwoDataSources" />
     <Or>


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org


Mime
View raw message