This is an automated email from the ASF dual-hosted git repository. rmaucher pushed a commit to branch 10.1.x in repository https://gitbox.apache.org/repos/asf/tomcat.git
commit 8d185b24277b0873be50feb4b39ea17e73e41cb6 Author: opencode <[email protected]> AuthorDate: Tue Oct 27 09:16:37 2026 +0100 Release the per-connection lock by lock state, not by configuration PooledConnection.lock() and unlock() each evaluated useLock || isPoolSweeperEnabled() at call time. The PoolConfiguration is shared and mutated in place by runtime JMX setters (setUseLock, and the setters that flip any of the pool-sweeper predicates), so the flag could differ between a lock() and its matching unlock(). The pool pairs them as lock(); try { ... } finally { unlock(); } at nine call sites in ConnectionPool, so the divergence broke the acquire/release pairing in both directions: acquired at lock() time but disabled at unlock() time left the connection write-locked forever by a departed thread (the next lock() on that connection then blocked indefinitely), and not acquired at lock() time but enabled at unlock() time threw IllegalMonitorStateException from the caller's finally block, replacing the borrow result and stranding the connection in busy. unlock() now releases based on the lock state instead of the configuration: it unlocks iff the current thread holds the write lock, releasing exactly one hold per call so reentrant nesting still unwinds correctly. The state-based predicate is a constant-time AQS owner read, no more expensive than the configuration check it replaces, and it also no-ops safely when lock() itself failed before acquiring. Regression tests in TestPooledConnectionLockRelease flip useLock between direct lock()/unlock() calls on a pooled connection behind a live borrow and prove, via a fresh holder thread with two latches, that a switched-off pair still released the lock (pre-fix: leaked) and a switched-on pair no longer throws (pre-fix: IllegalMonitorStateException). --- .../apache/tomcat/jdbc/pool/PooledConnection.java | 16 ++- .../jdbc/pool/TestPooledConnectionLockRelease.java | 126 +++++++++++++++++++++ 2 files changed, 137 insertions(+), 5 deletions(-) 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 0a8bf428ef..5f98000966 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 @@ -850,7 +850,7 @@ public class PooledConnection implements PooledConnectionMBean { /** * Locks the connection only if either {@link PoolConfiguration#isPoolSweeperEnabled()} or - * {@link PoolConfiguration#getUseLock()} return true. The per connection lock ensures thread safety is + * {@link PoolConfiguration#getUseLock()} return true. The per connection lock ensures thread safety if * multiple threads are performing operations on the connection. * Otherwise this is a noop for performance */ @@ -862,12 +862,18 @@ public class PooledConnection implements PooledConnectionMBean { } /** - * Unlocks the connection only if the sweeper is enabled - * Otherwise this is a noop for performance + * Unlocks the connection if, and only if, this thread currently holds + * the lock. The release decision is based on the lock state rather than + * on the configuration that {@link #lock()} consulted, because that + * configuration is mutable (it can be changed at runtime through JMX) + * and may differ between the two calls: deciding by configuration would + * either leave the lock write-locked forever (enabled at lock() time, + * disabled at unlock() time) or throw IllegalMonitorStateException + * (disabled at lock() time, enabled at unlock() time). */ public void unlock() { - if (poolProperties.getUseLock() || this.poolProperties.isPoolSweeperEnabled()) { - //optimized, only use a lock when there is concurrency + if (lock.isWriteLockedByCurrentThread()) { + //release only what this thread actually acquired lock.writeLock().unlock(); } } diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestPooledConnectionLockRelease.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestPooledConnectionLockRelease.java new file mode 100644 index 0000000000..fc21ec3661 --- /dev/null +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestPooledConnectionLockRelease.java @@ -0,0 +1,126 @@ +/* + * 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 java.sql.Connection; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.TimeUnit; + +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Test; + +import org.apache.tomcat.jdbc.JdbcPoolBaseTest; + +/** + * The per-connection lock is only taken when the configuration says so, but + * the configuration is mutable at runtime (JMX setters change useLock and + * the pool sweeper predicates on the shared PoolProperties). A + * {@code lock()}/{@code unlock()} pair must therefore release based on the + * lock state, not on the configuration at release time: otherwise a flip + * between the calls either leaves the connection write-locked forever or + * throws IllegalMonitorStateException at the caller. + */ +class TestPooledConnectionLockRelease extends JdbcPoolBaseTest { + + /* Switches the per-connection locking fully off, both the useLock + * flag and every predicate of the pool-sweeper computation. */ + private static void lockingDisabled(PoolConfiguration props) { + props.setUseLock(false); + props.setRemoveAbandoned(false); + props.setSuspectTimeout(0); + props.setTestWhileIdle(false); + props.setMinEvictableIdleTimeMillis(0); + props.setMaxAge(0); + Assertions.assertFalse(props.isPoolSweeperEnabled()); + } + + /* The pool-internal PooledConnection behind a borrowed connection. */ + private static PooledConnection delegateOf(Connection pooled) { + ProxyConnection proxy = interceptorOf(pooled, ProxyConnection.class); + Assertions.assertNotNull(proxy); + PooledConnection delegate = proxy.getDelegateConnection(); + Assertions.assertNotNull(delegate); + return delegate; + } + + /* + * Tries to lock the connection from a fresh daemon thread while locking + * is configured on, and reports whether the lock was taken and + * released. A leftover write lock from this thread makes the attempt + * block, so the unsuccessful case is reported after a bounded wait + * rather than hanging the suite. Caller must have locking disabled + * (lockingDisabled) before the call. + */ + private static boolean takenAndReleasedByAnotherThread(PooledConnection pcon, + PoolConfiguration props) throws InterruptedException { + props.setUseLock(true); + CountDownLatch acquired = new CountDownLatch(1); + CountDownLatch released = new CountDownLatch(1); + Thread holder = new Thread(() -> { + pcon.lock(); + acquired.countDown(); + pcon.unlock(); + released.countDown(); + }, "test-lock-holder"); + holder.setDaemon(true); // a leftover lock pre-fix would block it forever + holder.start(); + boolean taken = acquired.await(AWAIT_TIMEOUT_MS, TimeUnit.MILLISECONDS); + boolean done = taken && released.await(AWAIT_TIMEOUT_MS, TimeUnit.MILLISECONDS); + props.setUseLock(false); + return done; + } + + @Test + void unlockingAfterTheLockWasSwitchedOffStillReleasesTheLock() throws Exception { + DataSource ds = newDataSource(); + PoolConfiguration props = ds.getPoolProperties(); + lockingDisabled(props); + + try (Connection c = ds.getConnection()) { + PooledConnection pcon = delegateOf(c); + props.setUseLock(true); + pcon.lock(); + props.setUseLock(false); + pcon.unlock(); + // Pre-fix the unlock() above was a no-op and the write lock + // stayed held by this thread forever. + Assertions.assertTrue(takenAndReleasedByAnotherThread(pcon, props), + "unlock() must release the lock even when locking was " + + "switched off between the calls"); + } + } + + @Test + void unlockingAfterTheLockWasSwitchedOnDoesNotThrow() throws Exception { + DataSource ds = newDataSource(); + PoolConfiguration props = ds.getPoolProperties(); + lockingDisabled(props); + + try (Connection c = ds.getConnection()) { + PooledConnection pcon = delegateOf(c); + pcon.lock(); // no-op: locking is off + props.setUseLock(true); + // Pre-fix this threw IllegalMonitorStateException from the + // unpaired release. + Assertions.assertDoesNotThrow(pcon::unlock, + "unlock() must be a no-op when lock() never acquired"); + Assertions.assertTrue(takenAndReleasedByAnotherThread(pcon, props), + "the connection lock must be free after a no-op " + + "lock/unlock pair"); + } + } +} --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
