This is an automated email from the ASF dual-hosted git repository. markt-asf pushed a commit to branch 10.1.x in repository https://gitbox.apache.org/repos/asf/tomcat.git
commit cdb326abc7a6feeb9cf7c50f2d099e4252162e3f Author: Mark Thomas <[email protected]> AuthorDate: Tue May 26 14:00:44 2026 +0100 Refactor DataSourceStore to reduce duplication around duplication Also addresses a regression the previous refactoring that meant an exception was thrown if the first attempt failed but the second succeeded. --- .../apache/catalina/session/DataSourceStore.java | 263 ++++++++------------- 1 file changed, 104 insertions(+), 159 deletions(-) diff --git a/java/org/apache/catalina/session/DataSourceStore.java b/java/org/apache/catalina/session/DataSourceStore.java index 24436af58b..2bcad9e9b4 100644 --- a/java/org/apache/catalina/session/DataSourceStore.java +++ b/java/org/apache/catalina/session/DataSourceStore.java @@ -353,101 +353,65 @@ public class DataSourceStore extends StoreBase { * * @return array containing the list of session IDs */ - private String[] keys(boolean expiredOnly) { - String[] keys = null; - int numberOfTries = 2; - while (numberOfTries > 0) { - - Connection _conn = getConnection(); - if (_conn == null) { - return new String[0]; - } - try { + private String[] keys(boolean expiredOnly) throws IOException { + String sqlTmp = "SELECT " + sessionIdCol + " FROM " + sessionTable + " WHERE " + sessionAppCol + " = ?"; + if (expiredOnly) { + sqlTmp += " AND (" + sessionLastAccessedCol + " + " + sessionMaxInactiveCol + " * 1000 < ?)"; + } + final String keysSql = sqlTmp; - String keysSql = - "SELECT " + sessionIdCol + " FROM " + sessionTable + " WHERE " + sessionAppCol + " = ?"; + String[] keys = withRetry((ConnectionOperation<String[],IOException>) conn -> { + try (PreparedStatement preparedKeysSql = conn.prepareStatement(keysSql)) { + preparedKeysSql.setString(1, getName()); if (expiredOnly) { - keysSql += " AND (" + sessionLastAccessedCol + " + " + sessionMaxInactiveCol + " * 1000 < ?)"; + preparedKeysSql.setLong(2, System.currentTimeMillis()); } - try (PreparedStatement preparedKeysSql = _conn.prepareStatement(keysSql)) { - preparedKeysSql.setString(1, getName()); - if (expiredOnly) { - preparedKeysSql.setLong(2, System.currentTimeMillis()); - } - try (ResultSet rst = preparedKeysSql.executeQuery()) { - List<String> tmpkeys = new ArrayList<>(); - if (rst != null) { - while (rst.next()) { - tmpkeys.add(rst.getString(1)); - } + try (ResultSet rst = preparedKeysSql.executeQuery()) { + List<String> tmpkeys = new ArrayList<>(); + if (rst != null) { + while (rst.next()) { + tmpkeys.add(rst.getString(1)); } - keys = tmpkeys.toArray(new String[0]); - // Break out after the finally block - numberOfTries = 0; } + return tmpkeys.toArray(new String[0]); } - } catch (SQLException e) { - manager.getContext().getLogger().error(sm.getString("dataSourceStore.SQLException"), e); - keys = new String[0]; - // Close the connection so that it gets reopened next time - } finally { - release(_conn); } - numberOfTries--; - } - return keys; + }); + + return keys == null ? new String[0] : keys; } @Override public int getSize() throws IOException { - int size = 0; String sizeSql = "SELECT COUNT(" + sessionIdCol + ") FROM " + sessionTable + " WHERE " + sessionAppCol + " = ?"; - int numberOfTries = 2; - while (numberOfTries > 0) { - Connection _conn = getConnection(); - - if (_conn == null) { - return size; - } - - try (PreparedStatement preparedSizeSql = _conn.prepareStatement(sizeSql)) { + Integer size = withRetry((ConnectionOperation<Integer,IOException>) conn -> { + try (PreparedStatement preparedSizeSql = conn.prepareStatement(sizeSql)) { preparedSizeSql.setString(1, getName()); try (ResultSet rst = preparedSizeSql.executeQuery()) { if (rst.next()) { - size = rst.getInt(1); + return Integer.valueOf(rst.getInt(1)); + } else { + return Integer.valueOf(0); } - // Break out after the finally block - numberOfTries = 0; } - } catch (SQLException e) { - manager.getContext().getLogger().error(sm.getString("dataSourceStore.SQLException"), e); - } finally { - release(_conn); } - numberOfTries--; - } - return size; + }); + + return size == null ? 0 : size.intValue(); } @Override public Session load(String id) throws ClassNotFoundException, IOException { - StandardSession _session = null; org.apache.catalina.Context context = getManager().getContext(); Log contextLog = context.getLogger(); - - int numberOfTries = 2; String loadSql = "SELECT " + sessionIdCol + ", " + sessionDataCol + " FROM " + sessionTable + " WHERE " + sessionIdCol + " = ? AND " + sessionAppCol + " = ?"; - while (numberOfTries > 0) { - Connection _conn = getConnection(); - if (_conn == null) { - return null; - } + Session session = withRetry((ConnectionOperation<StandardSession,ClassNotFoundException>) conn -> { ClassLoader oldThreadContextCL = context.bind(Globals.IS_SECURITY_ENABLED, null); - try (PreparedStatement preparedLoadSql = _conn.prepareStatement(loadSql)) { + try (PreparedStatement preparedLoadSql = conn.prepareStatement(loadSql)) { preparedLoadSql.setString(1, id); preparedLoadSql.setString(2, getName()); try (ResultSet rst = preparedLoadSql.executeQuery()) { @@ -457,58 +421,29 @@ public class DataSourceStore extends StoreBase { contextLog.trace(sm.getString("dataSourceStore.loading", id, sessionTable)); } - _session = (StandardSession) manager.createEmptySession(); + StandardSession _session = (StandardSession) manager.createEmptySession(); _session.readObjectData(ois); _session.setManager(manager); + return _session; } } else if (context.getLogger().isDebugEnabled()) { contextLog.debug(sm.getString("dataSourceStore.noObject", id)); } - // Break out after the finally block - numberOfTries = 0; + return null; } - } catch (SQLException e) { - contextLog.error(sm.getString("dataSourceStore.SQLException"), e); } finally { context.unbind(Globals.IS_SECURITY_ENABLED, oldThreadContextCL); - release(_conn); } - numberOfTries--; - } - return _session; + }); + return session; } @Override public void remove(String id) throws IOException { - - SQLException sqlException = null; - - int numberOfTries = 2; - while (numberOfTries > 0) { - Connection _conn = getConnection(); - - if (_conn == null) { - return; - } - - try { - remove(id, _conn); - // Break out after the finally block - numberOfTries = 0; - } catch (SQLException e) { - // Keep the first exception just in case - if (sqlException == null) { - sqlException = e; - } - } finally { - release(_conn); - } - numberOfTries--; - } - - if (sqlException != null) { - throw new IOException(sm.getString("dataSourceStore.SQLException"), sqlException); - } + withRetry(conn -> { + remove(id, conn); + return null; + }); if (manager.getContext().getLogger().isTraceEnabled()) { manager.getContext().getLogger().trace(sm.getString("dataSourceStore.removing", id, sessionTable)); @@ -538,25 +473,13 @@ public class DataSourceStore extends StoreBase { public void clear() throws IOException { String clearSql = "DELETE FROM " + sessionTable + " WHERE " + sessionAppCol + " = ?"; - int numberOfTries = 2; - while (numberOfTries > 0) { - Connection _conn = getConnection(); - if (_conn == null) { - return; - } - - try (PreparedStatement preparedClearSql = _conn.prepareStatement(clearSql)) { + withRetry(conn -> { + try (PreparedStatement preparedClearSql = conn.prepareStatement(clearSql)) { preparedClearSql.setString(1, getName()); preparedClearSql.execute(); - // Break out after the finally block - numberOfTries = 0; - } catch (SQLException e) { - manager.getContext().getLogger().error(sm.getString("dataSourceStore.SQLException"), e); - } finally { - release(_conn); } - numberOfTries--; - } + return null; + }); } @Override @@ -564,7 +487,6 @@ public class DataSourceStore extends StoreBase { String saveSql = "INSERT INTO " + sessionTable + " (" + sessionIdCol + ", " + sessionAppCol + ", " + sessionDataCol + ", " + sessionValidCol + ", " + sessionMaxInactiveCol + ", " + sessionLastAccessedCol + ") VALUES (?, ?, ?, ?, ?, ?)"; - SQLException sqlException = null; synchronized (session) { @@ -575,46 +497,24 @@ public class DataSourceStore extends StoreBase { } byte[] obs = bos.toByteArray(); - int numberOfTries = 2; - while (numberOfTries > 0) { - Connection _conn = getConnection(); - if (_conn == null) { - return; - } - - try { - // Remove session if it exists and insert again. - remove(session.getIdInternal(), _conn); - - int size = obs.length; - try (ByteArrayInputStream bis = new ByteArrayInputStream(obs, 0, size); - InputStream in = new BufferedInputStream(bis, size); - PreparedStatement preparedSaveSql = _conn.prepareStatement(saveSql)) { - preparedSaveSql.setString(1, session.getIdInternal()); - preparedSaveSql.setString(2, getName()); - preparedSaveSql.setBinaryStream(3, in, size); - preparedSaveSql.setString(4, session.isValid() ? "1" : "0"); - preparedSaveSql.setInt(5, session.getMaxInactiveInterval()); - preparedSaveSql.setLong(6, session.getLastAccessedTime()); - preparedSaveSql.execute(); - // Break out after the finally block - numberOfTries = 0; - sqlException = null; - } - } catch (SQLException e) { - // Keep the first exception just in case - if (sqlException == null) { - sqlException = e; - } - } finally { - release(_conn); + withRetry(conn -> { + // Remove session if it exists and insert again. + remove(session.getIdInternal(), conn); + + int size = obs.length; + try (ByteArrayInputStream bis = new ByteArrayInputStream(obs, 0, size); + InputStream in = new BufferedInputStream(bis, size); + PreparedStatement preparedSaveSql = conn.prepareStatement(saveSql)) { + preparedSaveSql.setString(1, session.getIdInternal()); + preparedSaveSql.setString(2, getName()); + preparedSaveSql.setBinaryStream(3, in, size); + preparedSaveSql.setString(4, session.isValid() ? "1" : "0"); + preparedSaveSql.setInt(5, session.getMaxInactiveInterval()); + preparedSaveSql.setLong(6, session.getLastAccessedTime()); + preparedSaveSql.execute(); } - numberOfTries--; - } - } - - if (sqlException != null) { - throw new IOException(sm.getString("dataSourceStore.SQLException"), sqlException); + return null; + }); } if (manager.getContext().getLogger().isTraceEnabled()) { @@ -744,4 +644,49 @@ public class DataSourceStore extends StoreBase { } } + + private <T, E extends Exception> T withRetry(ConnectionOperation<T,E> operation) throws IOException, E { + SQLException sqlException = null; + + int numberOfTries = 2; + while (numberOfTries > 0) { + /* + * TODO: To further improve consistency, consider refactoring getConnection so an IOException is thrown here + * with a nested SQLException if a connection cannot be obtained. This would also allow some of the null + * handling on return to be removed. + */ + Connection _conn = getConnection(); + if (_conn == null) { + return null; + } + + try { + return operation.execute(_conn); + } catch (SQLException e) { + // Retain the first exception to use as the cause if all retries fail + if (sqlException == null) { + sqlException = e; + } + } finally { + release(_conn); + } + numberOfTries--; + } + + throw new IOException(sm.getString("dataSourceStore.SQLException"), sqlException); + } + + + /** + * Functional interface for store operation. Used with {@link DataSourceStore#withRetry(ConnectionOperation)} to + * reduce code duplication. + * + * @param <T> The return type for the operation + * @param <E> The additional exception type thrown by this operation. If no additional exception type is thrown then + * specify IOException + */ + @FunctionalInterface + private interface ConnectionOperation<T, E extends Exception> { + T execute(Connection connection) throws IOException, SQLException, E; + } } --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
