diff --git a/dotCMS/src/main/java/com/dotcms/dotpubsub/JDBCPubSubImpl.java b/dotCMS/src/main/java/com/dotcms/dotpubsub/JDBCPubSubImpl.java index ea8d1bf821a7..32339cb434ce 100644 --- a/dotCMS/src/main/java/com/dotcms/dotpubsub/JDBCPubSubImpl.java +++ b/dotCMS/src/main/java/com/dotcms/dotpubsub/JDBCPubSubImpl.java @@ -9,14 +9,18 @@ import com.dotmarketing.util.StringUtils; import com.dotmarketing.util.UtilMethods; import com.google.common.annotations.VisibleForTesting; +import com.zaxxer.hikari.HikariDataSource; import io.vavr.Lazy; import io.vavr.control.Try; import org.postgresql.PGConnection; import org.postgresql.PGNotification; +import javax.sql.DataSource; import javax.validation.constraints.NotNull; import java.sql.Connection; +import java.sql.DriverManager; import java.sql.PreparedStatement; +import java.sql.SQLException; import java.sql.Statement; import java.util.Map; import java.util.Set; @@ -26,7 +30,8 @@ /** * Provides notifications for the postgres pub/sub connection. - * The secret sauce is that it borrows 1 DB connection and keeps it open forever. + * The secret sauce is that it opens 1 dedicated DB connection — outside the Hikari request pool, + * see {@code PGListener.listenerConnection()} — and keeps it open forever. * With this one long term connection, you "listen" to the topics on * postgres and continually do a "SELECT 1" in that connection to be notified * of any new messages to those topics. @@ -118,7 +123,7 @@ class PGListener extends Thread { private RUNSTATE runstate = RUNSTATE.STARTED; private final Set topics = ConcurrentHashMap.newKeySet(); - private final Lazy connection = Lazy.of(() -> Try.of(() -> DbConnectionFactory.getDataSource().getConnection()).getOrElseThrow(DotRuntimeException::new)); + private final Lazy connection = Lazy.of(() -> Try.of(PGListener::listenerConnection).getOrElseThrow(DotRuntimeException::new)); private final Lazy pgConnection = Lazy.of(() -> Try.of(() -> connection.get().unwrap(PGConnection.class)).getOrElseThrow(DotRuntimeException::new)); private final Pattern validTopicRegEx = Pattern.compile("[a-z0-9_]"); @@ -128,6 +133,52 @@ class PGListener extends Thread { pgConnection.get(); } + /** + * Opens the connection this listener holds open for the lifetime of the thread. + * + *

It is built from the pool's own JDBC coordinates rather than borrowed from the pool. + * A Postgres {@code LISTEN} needs a connection that is never returned, which is the one + * thing a request pool must not hand out: the slot is withdrawn for the lifetime of the + * JVM without ever showing up as in-use work, and HikariCP reports the hold as + * {@code Apparent connection leak detected} — a stack trace on every boot that looks like + * a bug and is not one (issue #36934).

+ * + *

Falls back to a pooled connection when the datasource does not expose a JDBC URL — a + * JNDI-provided or otherwise wrapped datasource — because a listener that works while + * logging a spurious warning beats no listener at all.

+ * + * @return a dedicated connection when possible, a pooled one otherwise + * @throws SQLException if the connection cannot be opened + */ + private static Connection listenerConnection() throws SQLException { + return listenerConnection(DbConnectionFactory.getDataSource()); + } + + /** + * {@link #listenerConnection()} against an explicit datasource, so the choice between a + * dedicated and a pooled connection can be exercised without a running pool. + * + * @param dataSource the datasource to derive the connection from + * @return a dedicated connection when possible, a pooled one otherwise + * @throws SQLException if the connection cannot be opened + */ + @VisibleForTesting + static Connection listenerConnection(final DataSource dataSource) throws SQLException { + if (dataSource instanceof HikariDataSource hikari + && UtilMethods.isSet(hikari.getJdbcUrl())) { + Logger.info(JDBCPubSubImpl.class, () -> "Opening a dedicated connection for the" + + " Postgres pub/sub listener, outside the Hikari pool."); + return DriverManager.getConnection(hikari.getJdbcUrl(), hikari.getUsername(), + hikari.getPassword()); + } + + Logger.warn(JDBCPubSubImpl.class, "The datasource exposes no JDBC URL, so the Postgres" + + " pub/sub listener has to borrow a pooled connection and hold it open. The" + + " pool loses that connection for the lifetime of the JVM and HikariCP will" + + " report it as an apparent connection leak."); + return dataSource.getConnection(); + } + private long failures = 0; diff --git a/dotCMS/src/test/java/com/dotcms/dotpubsub/JDBCPubSubListenerConnectionTest.java b/dotCMS/src/test/java/com/dotcms/dotpubsub/JDBCPubSubListenerConnectionTest.java new file mode 100644 index 000000000000..cdb0ee7ad6ae --- /dev/null +++ b/dotCMS/src/test/java/com/dotcms/dotpubsub/JDBCPubSubListenerConnectionTest.java @@ -0,0 +1,71 @@ +package com.dotcms.dotpubsub; + +import static org.junit.Assert.assertSame; +import static org.junit.Assert.assertThrows; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.spy; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +import com.zaxxer.hikari.HikariDataSource; +import java.sql.Connection; +import java.sql.SQLException; +import javax.sql.DataSource; +import org.junit.Test; + +/** + * Unit tests for where the Postgres pub/sub listener gets its connection from (issue #36934). + * + *

The listener holds its connection open for the lifetime of the JVM, which is exactly what a + * request pool must not give away: the slot never comes back and HikariCP reports the hold as an + * apparent connection leak on every boot. These tests pin the selection — dedicated when the + * datasource exposes JDBC coordinates, pooled only as a fallback — without needing a live pool.

+ * + * @author Fabrizzio Araya + */ +public class JDBCPubSubListenerConnectionTest { + + /** + * Given : a datasource that exposes no JDBC URL — a JNDI-provided or otherwise wrapped one. + * When : the listener asks for its connection. + * Then : it falls back to the pool, because a listener that works while logging a spurious + * warning beats no listener at all. + */ + @Test + public void datasourceWithoutJdbcCoordinates_fallsBackToThePool() throws SQLException { + + final DataSource pool = mock(DataSource.class); + final Connection pooled = mock(Connection.class); + when(pool.getConnection()).thenReturn(pooled); + + assertSame("Without JDBC coordinates the pool is the only option left", + pooled, JDBCPubSubImpl.PGListener.listenerConnection(pool)); + verify(pool).getConnection(); + } + + /** + * Given : a Hikari datasource that does expose a JDBC URL. + * When : the listener asks for its connection. + * Then : the pool is never asked for one — the connection is opened directly, so the pool + * keeps its full capacity and the leak detector has nothing to report. + * + *

The URL points at a closed port, so opening it fails; the assertion that matters is not + * the failure but that it was attempted instead of borrowing from the pool.

+ */ + @Test + public void hikariWithJdbcCoordinates_neverBorrowsFromThePool() throws SQLException { + + try (final HikariDataSource hikari = spy(new HikariDataSource())) { + hikari.setJdbcUrl("jdbc:postgresql://127.0.0.1:1/dotcms-does-not-exist"); + hikari.setUsername("dotcmsdbuser"); + hikari.setPassword("unused"); + + assertThrows("A closed port must surface as a SQLException from the direct connection", + SQLException.class, + () -> JDBCPubSubImpl.PGListener.listenerConnection(hikari)); + + verify(hikari, never()).getConnection(); + } + } +}