Defer abandoned connection trace formatting

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 |

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,14 +1270,21 @@ 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();
         x.fillInStackTrace();
         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

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,13 +684,29 @@ 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;
     }
 
     /**
      * Returns the stack trace from when this connection was borrowed. Can return null if no stack trace was set.
      * @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;
     }
 

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;
+        }
+    }
+}