feat(targetconnectionmanager): cache TTL/size configuration

README.md

@@ -137,6 +137,13 @@ size may lead to the subprocess being forcibly killed and the parent process
 failing to detect the reason for the failure, leading to inaccurate failure
 error messages and API responses.
 
+The environment variables `CRYOSTAT_TARGET_CACHE_MAX_CONNECTIONS` and
+`CRYOSTAT_TARGET_CACHE_TTL` are used to control the target JMX connection cache
+behaviour. `CRYOSTAT_TARGET_CACHE_MAX_CONNECTIONS` specifies how many
+connections may be held open at once. `-1` may be used to leave the cache size
+unlimited.  `CRYOSTAT_TARGET_CACHE_TTL` specifies how long (in seconds) these
+connections will be cached before they are closed due to inactivity.
+
 For logging, Cryostat uses SLF4J with the java.util.logging binding.
 The default configuration can be overridden by mounting the desired
 configuration file in the container, and setting the environment variable

pom.xml

@@ -354,6 +354,10 @@
               <argument>--mount</argument>
               <argument>type=tmpfs,target=/opt/cryostat.d/truststore.d</argument>
               <argument>--env</argument>
+              <argument>CRYOSTAT_TARGET_CACHE_MAX_CONNECTIONS=1</argument>
+              <argument>--env</argument>
+              <argument>CRYOSTAT_TARGET_CACHE_TTL=5</argument>
+              <argument>--env</argument>
               <argument>CRYOSTAT_DISABLE_JMX_AUTH=true</argument>
               <argument>--env</argument>
               <argument>CRYOSTAT_DISABLE_SSL=true</argument>

run.sh

@@ -79,6 +79,8 @@ podman run \
     -e CRYOSTAT_WEB_PORT=$CRYOSTAT_WEB_PORT \
     -e CRYOSTAT_EXT_WEB_PORT=$CRYOSTAT_EXT_WEB_PORT \
     -e CRYOSTAT_AUTH_MANAGER=$CRYOSTAT_AUTH_MANAGER \
+    -e CRYOSTAT_TARGET_CACHE_SIZE=$CRYOSTAT_TARGET_CACHE_SIZE \
+    -e CRYOSTAT_TARGET_CACHE_TTL=$CRYOSTAT_TARGET_CACHE_TTL \
     -e CRYOSTAT_CONFIG_PATH="/opt/cryostat.d/conf.d" \
     -e CRYOSTAT_ARCHIVE_PATH="/opt/cryostat.d/recordings.d" \
     -e CRYOSTAT_TEMPLATE_PATH="/opt/cryostat.d/templates.d" \

src/main/java/io/cryostat/net/NetworkModule.java

@@ -39,7 +39,10 @@
 
 import java.net.SocketException;
 import java.net.UnknownHostException;
+import java.time.Duration;
+import java.util.concurrent.ForkJoinPool;
 
+import javax.inject.Named;
 import javax.inject.Singleton;
 
 import io.cryostat.core.log.Logger;
@@ -50,6 +53,7 @@
 import io.cryostat.net.reports.ReportsModule;
 import io.cryostat.net.web.WebModule;
 
+import com.github.benmanes.caffeine.cache.Scheduler;
 import dagger.Binds;
 import dagger.Lazy;
 import dagger.Module;
@@ -68,6 +72,9 @@
         })
 public abstract class NetworkModule {
 
+    static final String TARGET_CACHE_SIZE = "CRYOSTAT_TARGET_CACHE_SIZE";
+    static final String TARGET_CACHE_TTL = "CRYOSTAT_TARGET_CACHE_TTL";
+
     @Provides
     @Singleton
     static HttpServer provideHttpServer(
@@ -88,12 +95,32 @@ static NetworkResolver provideNetworkResolver() {
         return new NetworkResolver();
     }
 
+    @Provides
+    @Named(TARGET_CACHE_SIZE)
+    static int provideMaxTargetConnections(Environment env) {
+        return Integer.parseInt(env.getEnv(TARGET_CACHE_SIZE, "-1"));
+    }
+
+    @Provides
+    @Named(TARGET_CACHE_TTL)
+    static Duration provideMaxTargetTTL(Environment env) {
+        return Duration.ofSeconds(Integer.parseInt(env.getEnv(TARGET_CACHE_TTL, "10")));
+    }
+
     @Provides
     @Singleton
     static TargetConnectionManager provideTargetConnectionManager(
-            Logger logger, Lazy<JFRConnectionToolkit> connectionToolkit) {
+            Lazy<JFRConnectionToolkit> connectionToolkit,
+            @Named(TARGET_CACHE_TTL) Duration maxTargetTtl,
+            @Named(TARGET_CACHE_SIZE) int maxTargetConnections,
+            Logger logger) {
         return new TargetConnectionManager(
-                connectionToolkit, TargetConnectionManager.DEFAULT_TTL, logger);
+                connectionToolkit,
+                ForkJoinPool.commonPool(),
+                Scheduler.systemScheduler(),
+                maxTargetTtl,
+                maxTargetConnections,
+                logger);
     }
 
     @Provides

src/main/java/io/cryostat/net/TargetConnectionManager.java

@@ -41,6 +41,7 @@
 import java.time.Duration;
 import java.util.Collections;
 import java.util.Optional;
+import java.util.concurrent.Executor;
 import java.util.regex.Matcher;
 import java.util.regex.Pattern;
 
@@ -68,21 +69,25 @@ public class TargetConnectionManager {
     public static final Pattern HOST_PORT_PAIR_PATTERN =
             Pattern.compile("^([^:\\s]+)(?::(\\d{1,5}))?$");
 
-    static final Duration DEFAULT_TTL = Duration.ofSeconds(90);
-
     private final Lazy<JFRConnectionToolkit> jfrConnectionToolkit;
     private final Logger logger;
 
     private final LoadingCache<ConnectionDescriptor, JFRConnection> connections;
 
     TargetConnectionManager(
-            Lazy<JFRConnectionToolkit> jfrConnectionToolkit, Duration ttl, Logger logger) {
+            Lazy<JFRConnectionToolkit> jfrConnectionToolkit,
+            Executor executor,
+            Scheduler scheduler,
+            Duration ttl,
+            int maxTargetConnections,
+            Logger logger) {
         this.jfrConnectionToolkit = jfrConnectionToolkit;
         this.logger = logger;
 
-        this.connections =
+        Caffeine<ConnectionDescriptor, JFRConnection> cacheBuilder =
                 Caffeine.newBuilder()
-                        .scheduler(Scheduler.systemScheduler())
+                        .executor(executor)
+                        .scheduler(scheduler)
                         .expireAfterAccess(ttl)
                         .removalListener(
                                 new RemovalListener<ConnectionDescriptor, JFRConnection>() {
@@ -119,8 +124,11 @@ public void onRemoval(
                                             }
                                         }
                                     }
-                                })
-                        .build(this::connect);
+                                });
+        if (maxTargetConnections >= 0) {
+            cacheBuilder = cacheBuilder.maximumSize(maxTargetConnections);
+        }
+        this.connections = cacheBuilder.build(this::connect);
     }
 
     public <T> T executeConnectedTask(
@@ -130,8 +138,8 @@ public <T> T executeConnectedTask(
 
     /**
      * Mark a connection as still in use by the consumer. Connections expire from cache and are
-     * automatically closed after {@link TargetConnectionManager.DEFAULT_TTL}. For long-running
-     * operations which may hold the connection open and active for longer than the default TTL,
+     * automatically closed after {@link NetworkModule.TARGET_CACHE_TTL}. For long-running
+     * operations which may hold the connection open and active for longer than the configured TTL,
      * this method provides a way for the consumer to inform the {@link TargetConnectionManager} and
      * its internal cache that the connection is in fact still active and should not be
      * expired/closed. This will extend the lifetime of the cache entry by another TTL into the
@@ -186,15 +194,12 @@ private JFRConnection connect(
         logger.info("Creating connection for {}", url.toString());
         evt.begin();
         try {
-            JFRConnection connection =
-                    jfrConnectionToolkit
-                            .get()
-                            .connect(
-                                    url,
-                                    credentials.orElse(null),
-                                    Collections.singletonList(
-                                            () -> this.connections.invalidate(cacheKey)));
-            return connection;
+            return jfrConnectionToolkit
+                    .get()
+                    .connect(
+                            url,
+                            credentials.orElse(null),
+                            Collections.singletonList(() -> this.connections.invalidate(cacheKey)));
         } catch (Exception e) {
             evt.setExceptionThrown(true);
             throw e;

src/test/java/io/cryostat/net/TargetConnectionManagerTest.java

@@ -39,13 +39,16 @@
 
 import java.time.Duration;
 import java.util.List;
+import java.util.concurrent.Executor;
+import java.util.concurrent.ForkJoinPool;
 
 import javax.management.remote.JMXServiceURL;
 
 import io.cryostat.core.log.Logger;
 import io.cryostat.core.net.JFRConnection;
 import io.cryostat.core.net.JFRConnectionToolkit;
 
+import com.github.benmanes.caffeine.cache.Scheduler;
 import org.hamcrest.MatcherAssert;
 import org.hamcrest.Matchers;
 import org.junit.jupiter.api.BeforeEach;
@@ -68,7 +71,14 @@ class TargetConnectionManagerTest {
 
     @BeforeEach
     void setup() {
-        this.mgr = new TargetConnectionManager(() -> jfrConnectionToolkit, TTL, logger);
+        this.mgr =
+                new TargetConnectionManager(
+                        () -> jfrConnectionToolkit,
+                        ForkJoinPool.commonPool(),
+                        Scheduler.systemScheduler(),
+                        TTL,
+                        1,
+                        logger);
     }
 
     @Test
@@ -180,7 +190,12 @@ public JFRConnection answer(InvocationOnMock invocation)
     void shouldCreateNewConnectionForAccessDelayedLongerThanTTL() throws Exception {
         TargetConnectionManager mgr =
                 new TargetConnectionManager(
-                        () -> jfrConnectionToolkit, Duration.ofNanos(1), logger);
+                        () -> jfrConnectionToolkit,
+                        ForkJoinPool.commonPool(),
+                        Scheduler.systemScheduler(),
+                        Duration.ofNanos(1),
+                        1,
+                        logger);
         Mockito.when(jfrConnectionToolkit.createServiceURL(Mockito.anyString(), Mockito.anyInt()))
                 .thenAnswer(
                         new Answer<JMXServiceURL>() {
@@ -210,4 +225,90 @@ public JFRConnection answer(InvocationOnMock invocation)
         JFRConnection conn2 = mgr.executeConnectedTask(desc, a -> a);
         MatcherAssert.assertThat(conn1, Matchers.not(Matchers.sameInstance(conn2)));
     }
+
+    @Test
+    void shouldCreateNewConnectionWhenMaxSizeZeroed() throws Exception {
+        TargetConnectionManager mgr =
+                new TargetConnectionManager(
+                        () -> jfrConnectionToolkit,
+                        new DirectExecutor(),
+                        Scheduler.disabledScheduler(),
+                        Duration.ofSeconds(1),
+                        0,
+                        logger);
+        Mockito.when(jfrConnectionToolkit.createServiceURL(Mockito.anyString(), Mockito.anyInt()))
+                .thenAnswer(
+                        new Answer<JMXServiceURL>() {
+                            @Override
+                            public JMXServiceURL answer(InvocationOnMock args) throws Throwable {
+                                String host = args.getArgument(0);
+                                int port = args.getArgument(1);
+                                return new JMXServiceURL(
+                                        "rmi",
+                                        "",
+                                        0,
+                                        String.format("/jndi/rmi://%s:%d/jmxrmi", host, port));
+                            }
+                        });
+        Mockito.when(jfrConnectionToolkit.connect(Mockito.any(), Mockito.any(), Mockito.any()))
+                .thenAnswer(
+                        new Answer<JFRConnection>() {
+                            @Override
+                            public JFRConnection answer(InvocationOnMock invocation)
+                                    throws Throwable {
+                                return Mockito.mock(JFRConnection.class);
+                            }
+                        });
+        ConnectionDescriptor desc = new ConnectionDescriptor("foo");
+        JFRConnection conn1 = mgr.executeConnectedTask(desc, a -> a);
+        JFRConnection conn2 = mgr.executeConnectedTask(desc, a -> a);
+        MatcherAssert.assertThat(conn1, Matchers.not(Matchers.sameInstance(conn2)));
+    }
+
+    @Test
+    void shouldCreateNewConnectionPerTarget() throws Exception {
+        TargetConnectionManager mgr =
+                new TargetConnectionManager(
+                        () -> jfrConnectionToolkit,
+                        new DirectExecutor(),
+                        Scheduler.disabledScheduler(),
+                        Duration.ofNanos(1),
+                        -1,
+                        logger);
+        Mockito.when(jfrConnectionToolkit.createServiceURL(Mockito.anyString(), Mockito.anyInt()))
+                .thenAnswer(
+                        new Answer<JMXServiceURL>() {
+                            @Override
+                            public JMXServiceURL answer(InvocationOnMock args) throws Throwable {
+                                String host = args.getArgument(0);
+                                int port = args.getArgument(1);
+                                return new JMXServiceURL(
+                                        "rmi",
+                                        "",
+                                        0,
+                                        String.format("/jndi/rmi://%s:%d/jmxrmi", host, port));
+                            }
+                        });
+        Mockito.when(jfrConnectionToolkit.connect(Mockito.any(), Mockito.any(), Mockito.any()))
+                .thenAnswer(
+                        new Answer<JFRConnection>() {
+                            @Override
+                            public JFRConnection answer(InvocationOnMock invocation)
+                                    throws Throwable {
+                                return Mockito.mock(JFRConnection.class);
+                            }
+                        });
+        ConnectionDescriptor desc1 = new ConnectionDescriptor("foo");
+        ConnectionDescriptor desc2 = new ConnectionDescriptor("bar");
+        JFRConnection conn1 = mgr.executeConnectedTask(desc1, a -> a);
+        JFRConnection conn2 = mgr.executeConnectedTask(desc2, a -> a);
+        MatcherAssert.assertThat(conn1, Matchers.not(Matchers.sameInstance(conn2)));
+    }
+
+    static class DirectExecutor implements Executor {
+        @Override
+        public void execute(Runnable r) {
+            r.run();
+        }
+    }
 }