This is an automated email from the ASF dual-hosted git repository. chrisdutz pushed a commit to branch develop in repository https://gitbox.apache.org/repos/asf/plc4x.git
commit c58082e0edbcdc4d39fef4bc65102f97bbccd469 Author: Christofer Dutz <[email protected]> AuthorDate: Fri Aug 14 09:23:38 2026 +0200 fix: Hardened the code against a potentally hanging close operation. --- .../java/utils/cache/ConnectionContainer.java | 20 +++- .../java/utils/cache/ConnectionContainerTest.java | 108 +++++++++++++++++++++ 2 files changed, 124 insertions(+), 4 deletions(-) diff --git a/plc4j/tools/connection-cache/src/main/java/org/apache/plc4x/java/utils/cache/ConnectionContainer.java b/plc4j/tools/connection-cache/src/main/java/org/apache/plc4x/java/utils/cache/ConnectionContainer.java index ff8c600ef9..4ac845bbe9 100644 --- a/plc4j/tools/connection-cache/src/main/java/org/apache/plc4x/java/utils/cache/ConnectionContainer.java +++ b/plc4j/tools/connection-cache/src/main/java/org/apache/plc4x/java/utils/cache/ConnectionContainer.java @@ -48,7 +48,10 @@ import java.util.concurrent.locks.ReentrantLock; * the connection state. The lock prevents race conditions during lease/return operations. * Using ReentrantLock provides better performance under contention compared to synchronized. * <p> - * Deadlock Prevention: Always acquires locks in the same order and releases them promptly. + * Deadlock Prevention: Always acquires locks in the same order and releases them promptly. The + * container lock is always the <em>inner</em> lock with respect to a {@link LeasedPlcConnection}'s + * own lock (lease -> container), which is why {@link #close()} closes an outstanding lease before + * taking the container lock rather than while holding it. * Timer tasks are canceled before attempting to close connections to prevent deadlocks. * Every blocking I/O performed under the lock is time-bounded — connect (see {@link #connectBounded()}, * bounded by {@code maxWaitTimeMs}), ping ({@code pingTimeoutMs}) and close ({@code closeTimeoutMs}) — so @@ -501,11 +504,20 @@ class ConnectionContainer { * the container should not be used anymore. */ public void close() { + // Close any outstanding lease WITHOUT holding the container lock. LeasedPlcConnection.close() + // takes the lease's own lock and only then re-enters this container via returnLease(), which + // takes the container lock — the lease -> container order every application thread follows + // when it closes its own lease. Doing it under the container lock here would take those two + // locks in the opposite order (container -> lease) and could deadlock against an application + // thread that is closing its lease at the same moment. The lease read is a volatile read, and + // LeasedPlcConnection.close() is idempotent, so racing with a concurrent lease return is safe. + LeasedPlcConnection outstandingLease = leasedConnection; + if (outstandingLease != null) { + outstandingLease.close(); + } + lock.lock(); try { - if (leasedConnection != null) { - leasedConnection.close(); - } closeInternal(); } finally { lock.unlock(); diff --git a/plc4j/tools/connection-cache/src/test/java/org/apache/plc4x/java/utils/cache/ConnectionContainerTest.java b/plc4j/tools/connection-cache/src/test/java/org/apache/plc4x/java/utils/cache/ConnectionContainerTest.java index 7ccc6399ca..1644379231 100644 --- a/plc4j/tools/connection-cache/src/test/java/org/apache/plc4x/java/utils/cache/ConnectionContainerTest.java +++ b/plc4j/tools/connection-cache/src/test/java/org/apache/plc4x/java/utils/cache/ConnectionContainerTest.java @@ -776,4 +776,112 @@ class ConnectionContainerTest { // The container recovered: it is not stuck leased and a subsequent caller can proceed. assertFalse(container.isLeased()); } + + /** + * Regression test for the lock-order inversion between the container lock and a + * {@link LeasedPlcConnection}'s own lock. + * <p> + * An application thread closing its lease takes the locks in the order lease -> container + * ({@code LeasedPlcConnection.close()} -> {@code returnLease()}). {@code ConnectionContainer.close()} + * used to take them in the opposite order, container -> lease, by calling {@code leasedConnection.close()} + * while holding the container lock. Running both concurrently — an application returning its connection + * while the manager shuts down or evicts the cached connection — could therefore deadlock, with neither + * lock being time-bounded and so no recovery. + * <p> + * The interleaving is forced deterministically rather than raced for. A third thread parks inside + * {@code scheduler.schedule(...)} while holding the container lock (the container schedules the wait + * timeout for a queued lease under that lock), which lets both contenders queue up behind it in a + * known order: first the container teardown, then the application's lease return. Releasing the + * container lock then hands it to the teardown, which — with the inversion — reaches for the lease + * lock the application thread is already holding while it waits for the container lock. Neither + * thread can make progress. With {@code close()} closing the lease <em>before</em> taking the + * container lock, both threads always finish. + */ + @Test + void testClose_ConcurrentWithLeaseClose_DoesNotDeadlock() throws Exception { + // Gate the scheduling of the queued lease's wait-timeout task: the container does that under + // its own lock, which gives the test a way to pin the container lock for as long as it needs. + CountDownLatch scheduleReached = new CountDownLatch(1); + CountDownLatch releaseContainerLock = new CountDownLatch(1); + java.util.concurrent.atomic.AtomicBoolean gateNextSchedule = new java.util.concurrent.atomic.AtomicBoolean(false); + ScheduledExecutorService gatedScheduler = mock(ScheduledExecutorService.class); + // The queued lease's wait-timeout lambda returns a value, so it binds to the Callable overload; + // the container's other scheduled tasks (idle / max-lease timeouts) are void and bind to the + // Runnable one. Stub both, but only park in the Callable one — that is the queued-lease path. + when(gatedScheduler.schedule(any(Runnable.class), anyLong(), any(TimeUnit.class))) + .thenAnswer(invocation -> mock(java.util.concurrent.ScheduledFuture.class)); + when(gatedScheduler.schedule(any(java.util.concurrent.Callable.class), anyLong(), any(TimeUnit.class))).thenAnswer(invocation -> { + if (gateNextSchedule.compareAndSet(true, false)) { + scheduleReached.countDown(); + assertTrue(releaseContainerLock.await(10, TimeUnit.SECONDS), "gate was never released"); + } + return mock(java.util.concurrent.ScheduledFuture.class); + }); + + ConnectionContainer container = new ConnectionContainer( + "test:tcp://localhost", + () -> mockConnection, + gatedScheduler, + 5000, // maxIdleTimeMs + 0, // maxLeaseTimeMs — no force-return, keep the test threads the only actors + 30000, // maxWaitTimeMs + 5000, // pingTimeoutMs + 30000 // idlePingThresholdMs + ); + + // Daemon threads: should this ever deadlock again, the stuck threads must not keep the JVM + // (and the surefire fork) alive after the assertion below has failed the test. + java.util.concurrent.ExecutorService executor = java.util.concurrent.Executors.newFixedThreadPool(3, runnable -> { + Thread thread = new Thread(runnable, "close-vs-lease-close"); + thread.setDaemon(true); + return thread; + }); + try { + PlcConnection leased = container.lease().get(2, TimeUnit.SECONDS); + + // Thread C: parks inside the container lock by queueing a second lease against the gated + // scheduler. Everything below then queues behind it on the container lock. + gateNextSchedule.set(true); + executor.submit(container::lease); + assertTrue(scheduleReached.await(5, TimeUnit.SECONDS), "container lock was never pinned"); + + CountDownLatch teardownDone = new CountDownLatch(1); + CountDownLatch leaseReturnDone = new CountDownLatch(1); + + // Thread B: the cache evicts / shuts down the container (container -> lease, pre-fix order). + executor.submit(() -> { + try { + container.close(); + } finally { + teardownDone.countDown(); + } + }); + // Let B reach the container lock before A queues behind it — with the fair lock this fixes + // the acquisition order, so the pre-fix inversion is hit every time rather than by chance. + Thread.sleep(250); + + // Thread A: the application returns its lease (lease -> container, the order every caller uses). + executor.submit(() -> { + try { + leased.close(); + } catch (Exception e) { + // Losing the race with the teardown is fine; only a hang is a failure. + } finally { + leaseReturnDone.countDown(); + } + }); + Thread.sleep(250); + + // Hand the container lock over to the two contenders. + releaseContainerLock.countDown(); + + assertTrue(teardownDone.await(10, TimeUnit.SECONDS), + "container.close() deadlocked against a concurrent lease return — the container lock " + + "must not be held while closing the lease"); + assertTrue(leaseReturnDone.await(10, TimeUnit.SECONDS), + "lease close() deadlocked against a concurrent container teardown"); + } finally { + executor.shutdownNow(); + } + } }
