This is an automated email from the ASF dual-hosted git repository. rmaucher pushed a commit to branch 11.0.x in repository https://gitbox.apache.org/repos/asf/tomcat.git
commit 17d4d27f54a4cfb36de72941dfcf60137ce3fb23 Author: opencode <[email protected]> AuthorDate: Sat Oct 31 11:46:38 2026 +0100 Pin that pool notification filters never run on the caller Listeners attached directly to the pool MBean with a notification filter used to have that filter evaluated by the JDK on whatever thread called notify() - a borrow, an abandon or a suspect check, several of which run under a connection's lock - so a filter that was slow stalled the pool path that only meant to report an event, against the plain intent of the delivery design that no listener code runs on those threads. Moving the whole hand-off into the single per-notification notifier task closed that, with filters included, but nothing yet pinned the filters' side of the contract: the saturation test pins the listener threads, no test had ever attached a filter directly. Add the missing pin in the class that owns the off-the-reporting-thread contract: a filter that holds the notifier while the reporting thread must still return promptly, and an assertion that the filter is evaluated on the pool's notifier thread, not the caller, and that the notification still reaches its listener afterwards. Test only - the behavior itself was fixed with the single-task hand-off. --- .../jdbc/pool/jmx/TestJmxNotificationDelivery.java | 91 ++++++++++++++++++++++ 1 file changed, 91 insertions(+) diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/jmx/TestJmxNotificationDelivery.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/jmx/TestJmxNotificationDelivery.java index 85869b9e2b..a61619c510 100644 --- a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/jmx/TestJmxNotificationDelivery.java +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/jmx/TestJmxNotificationDelivery.java @@ -24,8 +24,10 @@ import java.util.Set; import java.util.concurrent.CopyOnWriteArrayList; import java.util.concurrent.CountDownLatch; import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicReference; import javax.management.Notification; +import javax.management.NotificationFilter; import javax.management.NotificationListener; import org.junit.jupiter.api.Assertions; @@ -75,6 +77,12 @@ class TestJmxNotificationDelivery extends JdbcPoolBaseTest { */ private static final int SPLIT_FLOOD_SIZE = 2500; + /* + * The name the pool MBean's notifier thread factory gives its thread; the + * thread the whole delivery - filters and listeners alike - must run on. + */ + private static final String NOTIFIER_THREAD = "JDBC-Pool-JMX-Notifier"; + /** * Every notification type the pool MBean advertises, in the order the * burst test reports them. @@ -115,6 +123,33 @@ class TestJmxNotificationDelivery extends JdbcPoolBaseTest { } } + /* + * A JMX notification filter that records the thread evaluating it, + * signals that it has been reached, stays inside isNotificationEnabled + * until the test releases it - and never longer than + * STUCK_LISTENER_TIMEOUT_S, so that a filter evaluated on the reporting + * thread stalls notify() for that bound and no further. + */ + private static final class StuckFilter implements NotificationFilter { + private final CountDownLatch entered = new CountDownLatch(1); + private final CountDownLatch release = new CountDownLatch(1); + private final CountDownLatch completed = new CountDownLatch(1); + private final AtomicReference<String> thread = new AtomicReference<>(); + + @Override + public boolean isNotificationEnabled(Notification notification) { + thread.set(Thread.currentThread().getName()); + entered.countDown(); + try { + release.await(STUCK_LISTENER_TIMEOUT_S, TimeUnit.SECONDS); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + } + completed.countDown(); + return true; + } + } + /* * A listener that records the notifications it receives and blocks the * notifier thread on the first one until the test opens the gate - for at @@ -192,6 +227,62 @@ class TestJmxNotificationDelivery extends JdbcPoolBaseTest { jmxPool.removeListener(stuck); } + /* + * The filter a caller attaches directly to the MBean through the + * inherited addNotificationListener belongs to the notification's own + * delivery: the JDK evaluates it inside the hand-off the broadcaster + * makes for its listener, so while that hand-off ran on the thread which + * reported the condition, a slow or blocked filter stalled the pool path + * - with the listener handed off or not. Delivery is now one task that + * calls sendNotification on the notifier thread, and every filter of the + * MBean's own listener list is evaluated there, not on the caller. + */ + @Test + void aStuckFilterOnADirectlyAttachedListenerDoesNotStallTheThreadThatReports() + throws Exception { + DataSource ds = newDataSource(); + ds.getPoolProperties().setName("JmxDeliveryFilter-" + System.nanoTime()); + ds.createPool(); + // an unregistered MBean is enough: the filter sits on the MBean's own + // listener list, no MBean server is involved + ConnectionPool jmxPool = new ConnectionPool(ds.getPool()); + StuckFilter filter = new StuckFilter(); + RecordingListener listener = new RecordingListener(); + jmxPool.addNotificationListener(listener, filter, null); + try { + long start = System.currentTimeMillis(); + boolean reported = jmxPool.notify(ConnectionPool.NOTIFY_ABANDON, + "a connection was abandoned"); + long elapsedMs = System.currentTimeMillis() - start; + + Assertions.assertTrue(reported, + "the notification must be reported as handed off for delivery"); + Assertions.assertTrue(filter.entered.await(5, TimeUnit.SECONDS), + "the notification must still reach the filter"); + Assertions.assertTrue(elapsedMs < CALLER_MUST_RETURN_WITHIN_MS, + "reporting a condition must not wait for the filter: notify() took " + + elapsedMs + " ms, of the " + STUCK_LISTENER_TIMEOUT_S + + " s the filter holds on, so the filter ran on the" + + " thread that reported the condition"); + Assertions.assertEquals(1, filter.completed.getCount(), + "the filter must still be stuck at the point the caller was released"); + } finally { + filter.release.countDown(); + } + Assertions.assertTrue(filter.completed.await(5, TimeUnit.SECONDS), + "the released filter must have finished evaluating"); + Assertions.assertEquals(NOTIFIER_THREAD, filter.thread.get(), + "the filter must be evaluated on the pool's notification thread, not on the" + + " thread that reported the condition, which was " + + Thread.currentThread().getName()); + // the filter let the notification through, so the listener behind it + // must have been reached on the same delivery + awaitUntil(() -> !listener.received.isEmpty()); + Assertions.assertEquals(ConnectionPool.NOTIFY_ABANDON, listener.received.get(0).getType(), + "a notification the filter enabled must reach its listener"); + jmxPool.removeNotificationListener(listener); + } + @Test void aFailedBorrowOnAnExhaustedPoolReportsWithoutStallingTheBorrowThread() throws Exception { --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
