This is an automated email from the ASF dual-hosted git repository. rmaucher pushed a commit to branch 9.0.x in repository https://gitbox.apache.org/repos/asf/tomcat.git
commit cb2b2d2f82a69607aa3d33a6ab4aab92fe8e0d0d Author: opencode <[email protected]> AuthorDate: Sat Oct 31 12:31:21 2026 +0100 Contain a throwing local listener at the listener that threw The pool MBean hands each notification to the listeners registered with it outside JMX one after another, and the loop that walks them stood inside a single try. A listener that threw therefore ended the iteration: every listener registered behind it missed that notification - all of them, every time, if the thrower threw on principle - while the notification had been accepted, the throw was logged only under debug, and the method's own javadoc promised that a throwing listener was contained. Nothing suggested which listeners had been skipped, so the gap in the event stream was silent. Hand each listener the notification on its own, so a throw is contained at the one listener that threw and delivery goes on to the listeners behind it and to the next notification. The failure is logged where the sibling containment paths log it - this is not a lost notification, which is what the delivery queue's warning is for, and a listener that fails on a hot reporting path would otherwise fill the log. An Error is left alone, as it is on the JMX half of the delivery where the JDK broadcaster catches only what a listener throws: it takes the notifier thread down and the next notification starts a new one, which the method javadoc now states together with the per-listener posture. --- .../tomcat/jdbc/pool/jmx/ConnectionPool.java | 28 ++++--- .../jdbc/pool/jmx/TestJmxNotificationDelivery.java | 87 ++++++++++++++++++++++ 2 files changed, 106 insertions(+), 9 deletions(-) diff --git a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/jmx/ConnectionPool.java b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/jmx/ConnectionPool.java index 07ade4194d..60f6c8c8f5 100644 --- a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/jmx/ConnectionPool.java +++ b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/jmx/ConnectionPool.java @@ -273,21 +273,31 @@ public class ConnectionPool extends NotificationBroadcasterSupport implements Co } /** - * Hands a notification to the listeners registered with this MBean outside JMX. Runs on the notifier thread. A - * listener that throws is contained here: its exception may not kill the notifier thread that every pool in the JVM - * delivers its notifications on. + * Hands a notification to the listeners registered with this MBean outside JMX. Runs on the notifier thread. Each + * listener is handed the notification on its own and what one of them throws is contained at that one listener: it + * may not kill the notifier thread that every pool in the JVM delivers its notifications on, and it may not + * suppress the delivery of that notification to the listeners registered behind the thrower either. An + * {@link Error} is not contained - as it is not on the JMX half of the delivery, where the JDK broadcaster catches + * only what a listener throws - and takes the notifier thread down; the next notification starts a new one. * * @param n the notification to deliver * @param type the notification type, for the failure log */ private void deliverLocally(Notification n, String type) { - try { - for (NotificationListener listener : listeners) { + for (NotificationListener listener : listeners) { + try { listener.handleNotification(n, this); - } - } catch (Exception x) { - if (log.isDebugEnabled()) { - log.debug("Local notification listener failed. Type=" + type, x); + } catch (Exception x) { + // Contained at the listener that threw: the notification + // still reaches the listeners behind it and the notifier + // thread goes on to the next notification. An Error is left + // alone, as on the JMX half of the delivery - the JDK + // broadcaster's hand-off catches only a listener's exceptions + // - and takes this thread down, to be replaced when the next + // notification arrives + if (log.isDebugEnabled()) { + log.debug("Local notification listener failed. Type=" + type, x); + } } } } 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 a61619c510..e00d7f8ad1 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 @@ -181,6 +181,21 @@ class TestJmxNotificationDelivery extends JdbcPoolBaseTest { } } + /* + * A listener that records the notifications it receives and then throws - + * the shape of a monitoring listener that is simply broken, and throws on + * every notification it is handed. + */ + private static final class ThrowingListener implements NotificationListener { + private final List<Notification> received = new CopyOnWriteArrayList<>(); + + @Override + public void handleNotification(Notification notification, Object handback) { + received.add(notification); + throw new RuntimeException("this listener is broken"); + } + } + /* * A listener that only records the notifications it receives. */ @@ -227,6 +242,78 @@ class TestJmxNotificationDelivery extends JdbcPoolBaseTest { jmxPool.removeListener(stuck); } + /* + * The local fan-out hands the notification to each listener on its own: + * what one of them throws is contained at that one listener. It may not + * kill the notifier thread, and it may not suppress the delivery of that + * very notification to the listeners registered behind the thrower + * either. The queue is iterated in registration order, so with the thrower + * first, a fan-out that lets the exception escape its loop leaves every + * listener behind it hungry for exactly those notifications the thrower + * broke on - a silent, event-dependent gap that never surfaces anywhere + * else. + */ + @Test + void aThrowingLocalListenerDoesNotSuppressTheListenersBehindIt() throws Exception { + DataSource ds = newDataSource(); + ds.getPoolProperties().setName("JmxDeliveryThrower-" + System.nanoTime()); + ds.createPool(); + // an unregistered MBean is enough: this is about the local fan-out + ConnectionPool jmxPool = new ConnectionPool(ds.getPool()); + ThrowingListener throwing = new ThrowingListener(); + RecordingListener second = new RecordingListener(); + RecordingListener third = new RecordingListener(); + // registration order is the local delivery order: the thrower goes first + jmxPool.addListener(throwing); + jmxPool.addListener(second); + jmxPool.addListener(third); + try { + Assertions.assertTrue(jmxPool.notify(ConnectionPool.NOTIFY_ABANDON, "first"), + "the notification must be reported as handed off for delivery"); + + // the notification the thrower broke on must still reach both + // listeners behind it, once each, as one and the same notification + awaitUntil(() -> !second.received.isEmpty() && !third.received.isEmpty()); + Assertions.assertEquals(1, second.received.size(), + "the notification that broke the first listener must have reached the" + + " listener behind it once"); + Assertions.assertEquals(1, third.received.size(), + "the notification that broke the first listener must have reached the" + + " third listener once too"); + Assertions.assertSame(second.received.get(0), third.received.get(0), + "both listeners behind the thrower must have received that very" + + " notification, not a later one"); + Assertions.assertEquals(ConnectionPool.NOTIFY_ABANDON, second.received.get(0).getType(), + "what reached the listeners behind the thrower must be the notification" + + " the thrower broke on"); + Assertions.assertEquals(1, throwing.received.size(), + "the throwing listener must have been reached too - its own failure is" + + " what the containment is about"); + + // and the contained failure does not break the notifier: the next + // notification runs the same fan-out, thrower and all, whole + Assertions.assertTrue(jmxPool.notify(ConnectionPool.NOTIFY_ABANDON, "second"), + "a notification reported after a listener threw must still be handed off"); + String secondMessage = "[" + ConnectionPool.NOTIFY_ABANDON + "] second"; + awaitUntil(() -> containsMessage(second.received, secondMessage) + && containsMessage(third.received, secondMessage)); + Assertions.assertEquals(2, second.received.size(), + "each listener behind the thrower must have received exactly the two" + + " notifications, with nothing lost and nothing duplicated"); + Assertions.assertEquals(2, third.received.size(), + "each listener behind the thrower must have received exactly the two" + + " notifications, with nothing lost and nothing duplicated"); + Assertions.assertTrue(second.received.get(1).getSequenceNumber() + > second.received.get(0).getSequenceNumber(), + "a listener behind the thrower must observe the notifications in the" + + " order the pool reported them"); + } finally { + jmxPool.removeListener(throwing); + jmxPool.removeListener(second); + jmxPool.removeListener(third); + } + } + /* * The filter a caller attaches directly to the MBean through the * inherited addNotificationListener belongs to the notification's own --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
