Repository: redis/lettuce (java)

Issue:
Introduce `Predicate<T extends StatefulConnection>` to `ConnectionPoolSupport` to customize pooled connection validation
## Feature Request


#### Is your feature request related to a problem? Please describe

It would be good to customize how pooled connections are validated. Out of the discussion at https://github.com/spring-projects/spring-data-redis/issues/3067#issuecomment-2545134759 (which boils down to defunct connections that end up with timeouts as the remote peer isn't responsive) it would be neat to customize the connection validation. Right now, `RedisPooledObjectFactory.validateObject(…)` performs `isOpen` validation. 

#### Describe the solution you'd like

By accepting a functional interface returning a `boolean` flag, it would be possible that callers provide their own validation function. Having another overload of `ConnectionPoolSupport.createGenericObjectPool(…)` could accept either a `Predicate` or, better an argument object that encapsulates `Predicate isValid` and `boolean wrapConnections`.


Pull request description:
Add custom connection validation to ConnectionPoolSupport #3081
Overview
---
- Introduce Predicate<T extends StatefulConnection> for connection validation
- Add new overload to ConnectionPoolSupport.createGenericObjectPool()
- Implement ConnectionPoolConfig to encapsulate validation and wrapping options
- Update RedisPooledObjectFactory to use custom validation logic

Closes #3081 



Patch:
diff --git a/src/main/java/io/lettuce/core/support/ConnectionPoolSupport.java b/src/main/java/io/lettuce/core/support/ConnectionPoolSupport.java
index f3ab8cb85..a048dfe27 100644
--- a/src/main/java/io/lettuce/core/support/ConnectionPoolSupport.java
+++ b/src/main/java/io/lettuce/core/support/ConnectionPoolSupport.java
@@ -6,6 +6,7 @@ import java.util.concurrent.CompletableFuture;
 import java.util.concurrent.atomic.AtomicReference;
 import java.util.concurrent.locks.Lock;
 import java.util.concurrent.locks.ReentrantLock;
+import java.util.function.Predicate;
 import java.util.function.Supplier;
 
 import org.apache.commons.pool2.BasePooledObjectFactory;
@@ -60,6 +61,7 @@ import io.lettuce.core.support.ConnectionWrapping.Origin;
  * </pre>
  *
  * @author Mark Paluch
+ * @author dae won
  * @since 4.3
  */
 public abstract class ConnectionPoolSupport {
@@ -69,7 +71,8 @@ public abstract class ConnectionPoolSupport {
 
     /**
      * Creates a new {@link GenericObjectPool} using the {@link Supplier}. Allocated instances are wrapped and must not be
-     * returned with {@link ObjectPool#returnObject(Object)}.
+     * returned with {@link ObjectPool#returnObject(Object)}. By default, connections are validated by checking their
+     * {@link StatefulConnection#isOpen()} method.
      *
      * @param connectionSupplier must not be {@code null}.
      * @param config must not be {@code null}.
@@ -78,16 +81,32 @@ public abstract class ConnectionPoolSupport {
      */
     public static <T extends StatefulConnection<?, ?>> GenericObjectPool<T> createGenericObjectPool(
             Supplier<T> connectionSupplier, GenericObjectPoolConfig<T> config) {
-        return createGenericObjectPool(connectionSupplier, config, true);
+        return createGenericObjectPool(connectionSupplier, config, true, (c) -> c.isOpen());
     }
 
     /**
-     * Creates a new {@link GenericObjectPool} using the {@link Supplier}.
+     * Creates a new {@link GenericObjectPool} using the {@link Supplier}. Allocated instances are wrapped and must not be
+     * returned with {@link ObjectPool#returnObject(Object)}.
+     *
+     * @param connectionSupplier must not be {@code null}.
+     * @param config must not be {@code null}.
+     * @param validationPredicate a {@link Predicate} to help validate connections
+     * @param <T> connection type.
+     * @return the connection pool.
+     */
+    public static <T extends StatefulConnection<?, ?>> GenericObjectPool<T> createGenericObjectPool(
+            Supplier<T> connectionSupplier, GenericObjectPoolConfig<T> config, Predicate<T> validationPredicate) {
+        return createGenericObjectPool(connectionSupplier, config, true, validationPredicate);
+    }
+
+    /**
+     * Creates a new {@link GenericObjectPool} using the {@link Supplier}. By default, connections are validated by checking
+     * their {@link StatefulConnection#isOpen()} method.
      *
      * @param connectionSupplier must not be {@code null}.
      * @param config must not be {@code null}.
      * @param wrapConnections {@code false} to return direct connections that need to be returned to the pool using
-     *        {@link ObjectPool#returnObject(Object)}. {@code true} to return wrapped connection that are returned to the pool
+     *        {@link ObjectPool#returnObject(Object)}. {@code true} to return wrapped connections that are returned to the pool
      *        when invoking {@link StatefulConnection#close()}.
      * @param <T> connection type.
      * @return the connection pool.
@@ -95,13 +114,34 @@ public abstract class ConnectionPoolSupport {
     @SuppressWarnings("unchecked")
     public static <T extends StatefulConnection<?, ?>> GenericObjectPool<T> createGenericObjectPool(
             Supplier<T> connectionSupplier, GenericObjectPoolConfig<T> config, boolean wrapConnections) {
+        return createGenericObjectPool(connectionSupplier, config, wrapConnections, (c) -> c.isOpen());
+    }
+
+    /**
+     * Creates a new {@link GenericObjectPool} using the {@link Supplier}.
+     *
+     * @param connectionSupplier must not be {@code null}.
+     * @param config must not be {@code null}.
+     * @param wrapConnections {@code false} to return direct connections that need to be returned to the pool using
+     *        {@link ObjectPool#returnObject(Object)}. {@code true} to return wrapped connections that are returned to the pool
+     *        when invoking {@link StatefulConnection#close()}.
+     * @param validationPredicate a {@link Predicate} to help validate connections
+     * @param <T> connection type.
+     * @return the connection pool.
+     */
+    @SuppressWarnings("unchecked")
+    public static <T extends StatefulConnection<?, ?>> GenericObjectPool<T> createGenericObjectPool(
+            Supplier<T> connectionSupplier, GenericObjectPoolConfig<T> config, boolean wrapConnections,
+            Predicate<T> validationPredicate) {
 
         LettuceAssert.notNull(connectionSupplier, "Connection supplier must not be null");
         LettuceAssert.notNull(config, "GenericObjectPoolConfig must not be null");
+        LettuceAssert.notNull(validationPredicate, "Connection validator must not be null");
 
         AtomicReference<Origin<T>> poolRef = new AtomicReference<>();
 
-        GenericObjectPool<T> pool = new GenericObjectPool<T>(new RedisPooledObjectFactory<T>(connectionSupplier), config) {
+        GenericObjectPool<T> pool = new GenericObjectPool<T>(
+                new RedisPooledObjectFactory<>(connectionSupplier, validationPredicate), config) {
 
             @Override
             public T borrowObject() throws Exception {
@@ -144,7 +184,7 @@ public abstract class ConnectionPoolSupport {
      *
      * @param connectionSupplier must not be {@code null}.
      * @param wrapConnections {@code false} to return direct connections that need to be returned to the pool using
-     *        {@link ObjectPool#returnObject(Object)}. {@code true} to return wrapped connection that are returned to the pool
+     *        {@link ObjectPool#returnObject(Object)}. {@code true} to return wrapped connections that are returned to the pool
      *        when invoking {@link StatefulConnection#close()}.
      * @param <T> connection type.
      * @return the connection pool.
@@ -152,12 +192,30 @@ public abstract class ConnectionPoolSupport {
     @SuppressWarnings("unchecked")
     public static <T extends StatefulConnection<?, ?>> SoftReferenceObjectPool<T> createSoftReferenceObjectPool(
             Supplier<T> connectionSupplier, boolean wrapConnections) {
+        return createSoftReferenceObjectPool(connectionSupplier, wrapConnections, (c) -> c.isOpen());
+    }
+
+    /**
+     * Creates a new {@link SoftReferenceObjectPool} using the {@link Supplier}.
+     *
+     * @param connectionSupplier must not be {@code null}.
+     * @param wrapConnections {@code false} to return direct connections that need to be returned to the pool using
+     *        {@link ObjectPool#returnObject(Object)}. {@code true} to return wrapped connections that are returned to the pool
+     *        when invoking {@link StatefulConnection#close()}.
+     * @param validationPredicate a {@link Predicate} to help validate connections
+     * @param <T> connection type.
+     * @return the connection pool.
+     */
+    @SuppressWarnings("unchecked")
+    public static <T extends StatefulConnection<?, ?>> SoftReferenceObjectPool<T> createSoftReferenceObjectPool(
+            Supplier<T> connectionSupplier, boolean wrapConnections, Predicate<T> validationPredicate) {
 
         LettuceAssert.notNull(connectionSupplier, "Connection supplier must not be null");
 
         AtomicReference<Origin<T>> poolRef = new AtomicReference<>();
 
-        SoftReferenceObjectPool<T> pool = new SoftReferenceObjectPool<T>(new RedisPooledObjectFactory<>(connectionSupplier)) {
+        SoftReferenceObjectPool<T> pool = new SoftReferenceObjectPool<T>(
+                new RedisPooledObjectFactory<>(connectionSupplier, validationPredicate)) {
 
             private final Lock lock = new ReentrantLock();
 
@@ -200,8 +258,11 @@ public abstract class ConnectionPoolSupport {
 
         private final Supplier<T> connectionSupplier;
 
-        RedisPooledObjectFactory(Supplier<T> connectionSupplier) {
+        private final Predicate<T> validationPredicate;
+
+        RedisPooledObjectFactory(Supplier<T> connectionSupplier, Predicate<T> validationPredicate) {
             this.connectionSupplier = connectionSupplier;
+            this.validationPredicate = validationPredicate;
         }
 
         @Override
@@ -221,7 +282,7 @@ public abstract class ConnectionPoolSupport {
 
         @Override
         public boolean validateObject(PooledObject<T> p) {
-            return p.getObject().isOpen();
+            return this.validationPredicate.test(p.getObject());
         }
 
     }


Test patch:
diff --git a/src/test/java/io/lettuce/core/dynamic/RedisCommandsIntegrationTests.java b/src/test/java/io/lettuce/core/dynamic/RedisCommandsIntegrationTests.java
index 21c2b1e98..aa0098796 100644
--- a/src/test/java/io/lettuce/core/dynamic/RedisCommandsIntegrationTests.java
+++ b/src/test/java/io/lettuce/core/dynamic/RedisCommandsIntegrationTests.java
@@ -112,6 +112,28 @@ class RedisCommandsIntegrationTests extends TestSupport {
         pool.close();
     }
 
+    @Test
+    void shouldWorkWithPooledConnectionAndCustomValidation() throws Exception {
+
+        GenericObjectPool<StatefulRedisConnection<String, String>> pool = ConnectionPoolSupport
+                .createGenericObjectPool(client::connect, new GenericObjectPoolConfig<>(), connection -> {
+                    try {
+                        return "PONG".equals(connection.sync().ping());
+                    } catch (Exception e) {
+                        return false;
+                    }
+                });
+
+        try (StatefulRedisConnection<String, String> connection = pool.borrowObject()) {
+
+            RedisCommandFactory factory = new RedisCommandFactory(connection);
+            SimpleCommands commands = factory.getCommands(SimpleCommands.class);
+            commands.get("foo");
+        }
+
+        pool.close();
+    }
+
     @Test
     void shouldWorkWithAsyncPooledConnection() {
 
diff --git a/src/test/java/io/lettuce/core/support/ConnectionPoolSupportIntegrationTests.java b/src/test/java/io/lettuce/core/support/ConnectionPoolSupportIntegrationTests.java
index ed38435ed..db64d7c33 100644
--- a/src/test/java/io/lettuce/core/support/ConnectionPoolSupportIntegrationTests.java
+++ b/src/test/java/io/lettuce/core/support/ConnectionPoolSupportIntegrationTests.java
@@ -131,6 +131,27 @@ class ConnectionPoolSupportIntegrationTests extends TestSupport {
         pool.close();
     }
 
+    @Test
+    void genericPoolShouldWorkWithValidationPredicate() throws Exception {
+
+        GenericObjectPool<StatefulRedisConnection<String, String>> pool = ConnectionPoolSupport
+                .createGenericObjectPool(() -> client.connect(), new GenericObjectPoolConfig<>(), false, connection -> {
+                    try {
+                        return "PONG".equals(connection.sync().ping());
+                    } catch (Exception e) {
+                        return false;
+                    }
+                });
+
+        borrowAndReturn(pool);
+
+        StatefulRedisConnection<String, String> connection = pool.borrowObject();
+        assertThat(Proxy.isProxyClass(connection.getClass())).isFalse();
+        pool.returnObject(connection);
+
+        pool.close();
+    }
+
     @Test
     void softReferencePoolShouldWorkWithPlainConnections() throws Exception {
 
@@ -147,6 +168,28 @@ class ConnectionPoolSupportIntegrationTests extends TestSupport {
         pool.close();
     }
 
+    @Test
+    void softReferencePoolShouldWorkWithValidationPredicate() throws Exception {
+
+        SoftReferenceObjectPool<StatefulRedisConnection<String, String>> pool = ConnectionPoolSupport
+                .createSoftReferenceObjectPool(() -> client.connect(), false, connection -> {
+                    try {
+                        return "PONG".equals(connection.sync().ping());
+                    } catch (Exception e) {
+                        return false;
+                    }
+                });
+
+        borrowAndReturn(pool);
+
+        StatefulRedisConnection<String, String> connection = pool.borrowObject();
+        assertThat(Proxy.isProxyClass(connection.getClass())).isFalse();
+        pool.returnObject(connection);
+
+        connection.close();
+        pool.close();
+    }
+
     @Test
     void genericPoolUsingWrappingShouldPropagateExceptionsCorrectly() throws Exception {