From 72c9ceb25ec66caf854b0439162b88a49d5896f1 Mon Sep 17 00:00:00 2001 From: sainadh777 <31940823+sainadh777@users.noreply.github.com> Date: Tue, 18 Aug 2026 20:55:26 -0700 Subject: [PATCH] Defer abandoned connection trace formatting --- TODO.md | 10 -- .../tomcat/jdbc/pool/ConnectionPool.java | 15 ++- .../tomcat/jdbc/pool/PooledConnection.java | 22 +++- .../tomcat/jdbc/test/AbandonedTraceTest.java | 114 ++++++++++++++++++ 4 files changed, 146 insertions(+), 15 deletions(-) create mode 100644 modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/test/AbandonedTraceTest.java diff --git a/TODO.md b/TODO.md index c289a1048bc6..badddc3e31d5 100644 --- a/TODO.md +++ b/TODO.md @@ -443,16 +443,6 @@ These are bugs, correctness issues, or missing functionality that may affect pro --- -### 39. Connection Pool Thread Dump Optimization (1 item) - -| # | File:Line | Description | Fix Idea | Effort | Difficulty | -|---|-----------|-------------|----------|--------|------------| -| 39.1 | `ConnectionPool.java:1273` | Stores full stack trace string; could store `StackTraceElement[]` directly | Change `getThreadDump()` to return `StackTraceElement[]`. Store array and format on-demand for display. Saves memory. | 0.5-1 day | Low | - -**Total estimated effort: 0.5-1 day, Low difficulty** - ---- - ### 40. JDBC Pool JNDI Lookup (1 item) | # | File:Line | Description | Fix Idea | Effort | Difficulty | diff --git a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/ConnectionPool.java b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/ConnectionPool.java index 8da0df00ae5e..1355b2718777 100644 --- a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/ConnectionPool.java +++ b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/ConnectionPool.java @@ -795,7 +795,7 @@ protected PooledConnection createConnection(long now, PooledConnection notUsed, //no need to lock a new one, its not contented con.setTimestamp(now); if (getPoolProperties().isLogAbandoned()) { - con.setStackTrace(getThreadDump()); + con.setStackTraceElements(getThreadDumpElements()); } if (!busy.offer(con)) { log.debug("Connection doesn't fit into busy array, connection will not be traceable."); @@ -863,7 +863,7 @@ protected PooledConnection borrowConnection(long now, PooledConnection con, Stri con.setTimestamp(now); if (getPoolProperties().isLogAbandoned()) { //set the stack trace for this pool - con.setStackTrace(getThreadDump()); + con.setStackTraceElements(getThreadDumpElements()); } if (!busy.offer(con)) { log.debug("Connection doesn't fit into busy array, connection will not be traceable."); @@ -887,7 +887,7 @@ protected PooledConnection borrowConnection(long now, PooledConnection con, Stri con.setTimestamp(now); if (getPoolProperties().isLogAbandoned()) { //set the stack trace for this pool - con.setStackTrace(getThreadDump()); + con.setStackTraceElements(getThreadDumpElements()); } if (!busy.offer(con)) { log.debug("Connection doesn't fit into busy array, connection will not be traceable."); @@ -1270,7 +1270,6 @@ public void testAllIdle(boolean checkMaxAgeOnly) { /** * Creates a stack trace representing the existing thread's current state. * @return a string object representing the current state. - * TODO investigate if we simply should store {@link java.lang.Thread#getStackTrace()} elements */ protected static String getThreadDump() { Exception x = new Exception(); @@ -1278,6 +1277,14 @@ protected static String getThreadDump() { return getStackTrace(x); } + /** + * Creates a stack trace representing the existing thread's current state without formatting it. + * @return the stack trace elements representing the current state + */ + protected static StackTraceElement[] getThreadDumpElements() { + return new Exception().getStackTrace(); + } + /** * Convert an exception into a String * @param x - the throwable diff --git a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/PooledConnection.java b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/PooledConnection.java index 4967d6c696d7..d766ce4c2de7 100644 --- a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/PooledConnection.java +++ b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/PooledConnection.java @@ -83,9 +83,13 @@ public class PooledConnection implements PooledConnectionMBean { */ protected volatile javax.sql.XAConnection xaConnection; /** - * When we track abandon traces, this string holds the thread dump + * When we track abandon traces, this string holds the formatted thread dump */ private String abandonTrace = null; + /** + * When we track abandon traces, this array holds the thread dump until it needs to be formatted + */ + private StackTraceElement[] abandonTraceElements = null; /** * Timestamp the connection was last 'touched' by the pool */ @@ -680,6 +684,16 @@ public boolean release() { public void setStackTrace(String trace) { abandonTrace = trace; + abandonTraceElements = null; + } + + /** + * Sets the stack trace elements from when this connection was borrowed. + * @param trace the stack trace elements for this connection + */ + void setStackTraceElements(StackTraceElement[] trace) { + abandonTrace = null; + abandonTraceElements = trace; } /** @@ -687,6 +701,12 @@ public void setStackTrace(String trace) { * @return the stack trace or null of no trace was set */ public String getStackTrace() { + if (abandonTrace == null && abandonTraceElements != null) { + Exception x = new Exception(); + x.setStackTrace(abandonTraceElements); + abandonTrace = ConnectionPool.getStackTrace(x); + abandonTraceElements = null; + } return abandonTrace; } diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/test/AbandonedTraceTest.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/test/AbandonedTraceTest.java new file mode 100644 index 000000000000..5e5a86823cca --- /dev/null +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/test/AbandonedTraceTest.java @@ -0,0 +1,114 @@ +/* + * 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.test; + +import java.sql.SQLException; + +import org.junit.Assert; +import org.junit.Test; + +import org.apache.tomcat.jdbc.pool.ConnectionPool; +import org.apache.tomcat.jdbc.pool.PoolProperties; +import org.apache.tomcat.jdbc.pool.PooledConnection; + +public class AbandonedTraceTest { + + @Test + public void testTraceFormattingIsLazy() throws Exception { + PoolProperties properties = new PoolProperties(); + properties.setInitialSize(0); + properties.setLogAbandoned(true); + properties.setTimeBetweenEvictionRunsMillis(-1); + + TesterConnectionPool pool = new TesterConnectionPool(properties); + try { + TesterPooledConnection connection = new TesterPooledConnection(properties, pool); + + Assert.assertSame(connection, pool.borrow(connection)); + Assert.assertFalse(connection.isStringTraceSet()); + + String trace = connection.getStackTrace(); + Assert.assertTrue(trace, trace.contains("AbandonedTraceTest.testTraceFormattingIsLazy")); + } finally { + pool.closePool(); + } + } + + private static class TesterConnectionPool extends ConnectionPool { + + TesterConnectionPool(PoolProperties properties) throws SQLException { + super(properties); + } + + PooledConnection borrow(PooledConnection connection) throws SQLException { + return borrowConnection(System.currentTimeMillis(), connection, null, null); + } + + void closePool() { + close(true); + } + } + + private static class TesterPooledConnection extends PooledConnection { + + private boolean stringTraceSet; + + TesterPooledConnection(PoolProperties properties, ConnectionPool parent) { + super(properties, parent); + } + + @Override + public boolean isDiscarded() { + return false; + } + + @Override + public boolean isInitialized() { + return true; + } + + @Override + public boolean isMaxAgeExpired() { + return false; + } + + @Override + public boolean isReleased() { + return false; + } + + @Override + public void setStackTrace(String trace) { + stringTraceSet = true; + super.setStackTrace(trace); + } + + @Override + public boolean shouldForceReconnect(String username, String password) { + return false; + } + + @Override + public boolean validate(int validateAction) { + return true; + } + + boolean isStringTraceSet() { + return stringTraceSet; + } + } +}