Author: fhanik Date: Fri Aug 8 14:02:54 2014 New Revision: 1616760 URL: http://svn.apache.org/r1616760 Log: Fix https://issues.apache.org/bugzilla/show_bug.cgi?id=54227 Proper fix for max age evaluation at time of borrow. It should be done during borrow, not during setup. Setup does not perform proper initialization
Rename checkUser, poor name, hard to understand what the return value is Fix unit test around pool thread renaming Added: tomcat/trunk/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/ShouldForceReconnectTest.java (with props) Modified: tomcat/trunk/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/ConnectionPool.java tomcat/trunk/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/PooledConnection.java tomcat/trunk/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/test/PoolCleanerTest.java Modified: tomcat/trunk/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/ConnectionPool.java URL: http://svn.apache.org/viewvc/tomcat/trunk/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/ConnectionPool.java?rev=1616760&r1=1616759&r2=1616760&view=diff ============================================================================== --- tomcat/trunk/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/ConnectionPool.java (original) +++ tomcat/trunk/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/ConnectionPool.java Fri Aug 8 14:02:54 2014 @@ -279,10 +279,6 @@ public class ConnectionPool { * @throws SQLException if an interceptor can't be configured, if the proxy can't be instantiated */ protected Connection setupConnection(PooledConnection con) throws SQLException { - //check if it's been sitting in the pool too long - if (con.isMaxAgeExpired()) { - con.reconnect(); - } //fetch previously cached interceptor proxy - one per connection JdbcInterceptor handler = con.getHandler(); if (handler==null) { @@ -750,30 +746,20 @@ public class ConnectionPool { boolean setToNull = false; try { con.lock(); - boolean usercheck = con.checkUser(username, password); - if (con.isReleased()) { return null; } + //evaluate username/password change as well as max age functionality + boolean forceReconnect = con.shouldForceReconnect(username, password) || con.isMaxAgeExpired(); + if (!con.isDiscarded() && !con.isInitialized()) { - //attempt to connect - try { - con.connect(); - } catch (Exception x) { - release(con); - setToNull = true; - if (x instanceof SQLException) { - throw (SQLException)x; - } else { - SQLException ex = new SQLException(x.getMessage()); - ex.initCause(x); - throw ex; - } - } + //here it states that the connection not discarded, but the connection is null + //don't attempt a connect here. It will be done during the reconnect. + forceReconnect = true; } - if (usercheck) { + if (!forceReconnect) { if ((!con.isDiscarded()) && con.validate(PooledConnection.VALIDATE_BORROW)) { //set the timestamp con.setTimestamp(now); Modified: tomcat/trunk/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/PooledConnection.java URL: http://svn.apache.org/viewvc/tomcat/trunk/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/PooledConnection.java?rev=1616760&r1=1616759&r2=1616760&view=diff ============================================================================== --- tomcat/trunk/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/PooledConnection.java (original) +++ tomcat/trunk/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/PooledConnection.java Fri Aug 8 14:02:54 2014 @@ -134,8 +134,24 @@ public class PooledConnection { return connectionVersion; } + /** + * @deprecated use {@link #shouldForceReconnect(String, String)} + * method kept since it was public, to avoid changing interface. name was pooo + */ public boolean checkUser(String username, String password) { - if (!getPoolProperties().isAlternateUsernameAllowed()) return true; + return !shouldForceReconnect(username, password); + } + /** + * Returns true if we must force reconnect based on credentials passed in. + * Returns false if {@link PoolConfiguration#isAlternateUsernameAllowed()} method returns false. + * Returns false if the username/password has not changed since this connection was connected + * @param username the username you wish to connect with, pass in null to accept the default username from {@link PoolConfiguration#getUsername()} + * @param password the password you wish to connect with, pass in null to accept the default username from {@link org.apache.tomcat.jdbc.pool.PoolConfiguration#getPassword()} + * @return true is the pool must reconnect + */ + public boolean shouldForceReconnect(String username, String password) { + + if (!getPoolProperties().isAlternateUsernameAllowed()) return false; if (username==null) username = poolProperties.getUsername(); if (password==null) password = poolProperties.getPassword(); @@ -143,15 +159,15 @@ public class PooledConnection { String storedUsr = (String)getAttributes().get(PROP_USER); String storedPwd = (String)getAttributes().get(PROP_PASSWORD); - boolean result = (username==null && storedUsr==null); - result = (result || (username!=null && username.equals(storedUsr))); + boolean noChangeInCredentials = (username==null && storedUsr==null); + noChangeInCredentials = (noChangeInCredentials || (username!=null && username.equals(storedUsr))); - result = result && ((password==null && storedPwd==null) || (password!=null && password.equals(storedPwd))); + noChangeInCredentials = noChangeInCredentials && ((password==null && storedPwd==null) || (password!=null && password.equals(storedPwd))); if (username==null) getAttributes().remove(PROP_USER); else getAttributes().put(PROP_USER, username); if (password==null) getAttributes().remove(PROP_PASSWORD); else getAttributes().put(PROP_PASSWORD, password); - return result; + return !noChangeInCredentials; } /** Added: tomcat/trunk/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/ShouldForceReconnectTest.java URL: http://svn.apache.org/viewvc/tomcat/trunk/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/ShouldForceReconnectTest.java?rev=1616760&view=auto ============================================================================== --- tomcat/trunk/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/ShouldForceReconnectTest.java (added) +++ tomcat/trunk/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/ShouldForceReconnectTest.java Fri Aug 8 14:02:54 2014 @@ -0,0 +1,142 @@ +/* + * 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.tomcat.jdbc.pool; + +import org.junit.After; +import org.junit.Before; +import org.junit.Test; + +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; + +public class ShouldForceReconnectTest { + + private ConnectionPool pool; + private PoolProperties properties; + + private static final String DEFAULT_USER = "username_def"; + private static final String DEFAULT_PASSWD = "password_def"; + private static final String ALT_USER = "username_alt"; + private static final String ALT_PASSWD = "password_alt"; + + @Before + public void setUp() throws Exception { + properties = new PoolProperties(); + properties.setUsername(DEFAULT_USER); + properties.setPassword(DEFAULT_PASSWD); + properties.setAlternateUsernameAllowed(true); + properties.setInitialSize(0); + properties.setRemoveAbandoned(false); + properties.setTimeBetweenEvictionRunsMillis(-1); + pool = new ConnectionPool(properties); + } + + @After + public void tearDown() throws Exception { + + + + + } + + @Test + public void testShouldForceReconnect() throws Exception { + PooledConnection con = new PooledConnection(properties, pool); + + //connection previously connect with default + configureDefault(con); + assertFalse(con.shouldForceReconnect(null, null)); + + configureDefault(con); + assertFalse(con.shouldForceReconnect(DEFAULT_USER, DEFAULT_PASSWD)); + + configureDefault(con); + assertFalse(con.shouldForceReconnect(null,DEFAULT_PASSWD)); + + configureDefault(con); + assertFalse(con.shouldForceReconnect(DEFAULT_USER, null)); + + configureDefault(con); + assertTrue(con.shouldForceReconnect(ALT_USER,ALT_PASSWD)); + + configureDefault(con); + assertTrue(con.shouldForceReconnect(null,ALT_PASSWD)); + + configureDefault(con); + assertTrue(con.shouldForceReconnect(ALT_USER,null)); + + //connection previously connect with alternate + configureAlt(con); + assertFalse(con.shouldForceReconnect(ALT_USER, ALT_PASSWD)); + + configureAlt(con); + assertTrue(con.shouldForceReconnect(null, null)); + + configureAlt(con); + assertTrue(con.shouldForceReconnect(DEFAULT_USER, DEFAULT_PASSWD)); + + configureAlt(con); + assertTrue(con.shouldForceReconnect(null, DEFAULT_PASSWD)); + + configureAlt(con); + assertTrue(con.shouldForceReconnect(DEFAULT_USER, null)); + + configureAlt(con); + assertTrue(con.shouldForceReconnect(null,ALT_PASSWD)); + + configureAlt(con); + assertTrue(con.shouldForceReconnect(ALT_USER,null)); + + //test changes in username password + configureDefault(con); + assertFalse(con.shouldForceReconnect(null, null)); + assertTrue(con.shouldForceReconnect(ALT_USER, ALT_PASSWD)); + assertFalse(con.shouldForceReconnect(ALT_USER, ALT_PASSWD)); + assertTrue(con.shouldForceReconnect(null, null)); + + configureDefault(con); + assertFalse(con.shouldForceReconnect(DEFAULT_USER, DEFAULT_PASSWD)); + assertTrue(con.shouldForceReconnect(ALT_USER, ALT_PASSWD)); + assertFalse(con.shouldForceReconnect(ALT_USER, ALT_PASSWD)); + assertTrue(con.shouldForceReconnect(DEFAULT_USER, DEFAULT_PASSWD)); + + configureAlt(con); + assertFalse(con.shouldForceReconnect(ALT_USER, ALT_PASSWD)); + assertFalse(con.shouldForceReconnect(ALT_USER, ALT_PASSWD)); + assertTrue(con.shouldForceReconnect(DEFAULT_USER, DEFAULT_PASSWD)); + assertFalse(con.shouldForceReconnect(null, null)); + assertTrue(con.shouldForceReconnect(ALT_USER, ALT_PASSWD)); + + + configureAlt(con); + assertTrue(con.shouldForceReconnect(DEFAULT_USER, DEFAULT_PASSWD)); + assertTrue(con.shouldForceReconnect(ALT_USER, ALT_PASSWD)); + assertFalse(con.shouldForceReconnect(ALT_USER, ALT_PASSWD)); + assertTrue(con.shouldForceReconnect(DEFAULT_USER, DEFAULT_PASSWD)); + + } + + private void configureAlt(PooledConnection con) { + con.getAttributes().put(PooledConnection.PROP_USER, ALT_USER); + con.getAttributes().put(PooledConnection.PROP_PASSWORD, ALT_PASSWD); + } + + private void configureDefault(PooledConnection con) { + con.getAttributes().put(PooledConnection.PROP_USER, DEFAULT_USER); + con.getAttributes().put(PooledConnection.PROP_PASSWORD, DEFAULT_PASSWD); + } +} \ No newline at end of file Propchange: tomcat/trunk/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/ShouldForceReconnectTest.java ------------------------------------------------------------------------------ svn:eol-style = native Modified: tomcat/trunk/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/test/PoolCleanerTest.java URL: http://svn.apache.org/viewvc/tomcat/trunk/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/test/PoolCleanerTest.java?rev=1616760&r1=1616759&r2=1616760&view=diff ============================================================================== --- tomcat/trunk/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/test/PoolCleanerTest.java (original) +++ tomcat/trunk/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/test/PoolCleanerTest.java Fri Aug 8 14:02:54 2014 @@ -30,7 +30,7 @@ public class PoolCleanerTest extends Def Map<Thread, StackTraceElement[]> threadmap = Thread.getAllStackTraces(); int result = 0; for (Thread t : threadmap.keySet()) { - if (t.getName().startsWith("PoolCleaner[")) result++; + if (t.getName().startsWith("Tomcat JDBC Pool Cleaner[")) result++; } return result; } --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org