You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomcat.apache.org by ma...@apache.org on 2019/11/20 13:24:52 UTC
[tomcat] 03/04: Fix SpotBugs issues in JDBC pool tests
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 <ma...@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