From d0ee2b50eae365372530b80c0677120fd0173211 Mon Sep 17 00:00:00 2001 From: Andrew Azores Date: Tue, 10 Aug 2021 10:36:50 -0400 Subject: [PATCH 1/8] feat(targetconnectionmanager): configurable cache size --- pom.xml | 2 + .../java/io/cryostat/net/NetworkModule.java | 18 ++++- .../cryostat/net/TargetConnectionManager.java | 16 +++- .../net/TargetConnectionManagerTest.java | 74 ++++++++++++++++++- 4 files changed, 102 insertions(+), 8 deletions(-) diff --git a/pom.xml b/pom.xml index 868a9456d8..70a36365be 100644 --- a/pom.xml +++ b/pom.xml @@ -354,6 +354,8 @@ --mount type=tmpfs,target=/opt/cryostat.d/truststore.d --env + CRYOSTAT_MAX_TARGET_CONNECTIONS=1 + --env CRYOSTAT_DISABLE_JMX_AUTH=true --env CRYOSTAT_DISABLE_SSL=true diff --git a/src/main/java/io/cryostat/net/NetworkModule.java b/src/main/java/io/cryostat/net/NetworkModule.java index 48ed6f9db0..e61ec8933e 100644 --- a/src/main/java/io/cryostat/net/NetworkModule.java +++ b/src/main/java/io/cryostat/net/NetworkModule.java @@ -40,6 +40,7 @@ import java.net.SocketException; import java.net.UnknownHostException; +import javax.inject.Named; import javax.inject.Singleton; import io.cryostat.core.log.Logger; @@ -68,6 +69,8 @@ }) public abstract class NetworkModule { + static final String MAX_TARGET_CONNECTIONS = "CRYOSTAT_MAX_TARGET_CONNECTIONS"; + @Provides @Singleton static HttpServer provideHttpServer( @@ -88,12 +91,23 @@ static NetworkResolver provideNetworkResolver() { return new NetworkResolver(); } + @Provides + @Named(MAX_TARGET_CONNECTIONS) + static int provideMaxTargetConnections(Environment env) { + return Integer.parseInt(env.getEnv(MAX_TARGET_CONNECTIONS, "-1")); + } + @Provides @Singleton static TargetConnectionManager provideTargetConnectionManager( - Logger logger, Lazy connectionToolkit) { + Lazy connectionToolkit, + @Named(MAX_TARGET_CONNECTIONS) int maxTargetConnections, + Logger logger) { return new TargetConnectionManager( - connectionToolkit, TargetConnectionManager.DEFAULT_TTL, logger); + connectionToolkit, + TargetConnectionManager.DEFAULT_TTL, + maxTargetConnections, + logger); } @Provides diff --git a/src/main/java/io/cryostat/net/TargetConnectionManager.java b/src/main/java/io/cryostat/net/TargetConnectionManager.java index e4375e7170..d05200fd1c 100644 --- a/src/main/java/io/cryostat/net/TargetConnectionManager.java +++ b/src/main/java/io/cryostat/net/TargetConnectionManager.java @@ -76,14 +76,22 @@ public class TargetConnectionManager { private final LoadingCache connections; TargetConnectionManager( - Lazy jfrConnectionToolkit, Duration ttl, Logger logger) { + Lazy jfrConnectionToolkit, + Duration ttl, + int maxTargetConnections, + Logger logger) { this.jfrConnectionToolkit = jfrConnectionToolkit; this.logger = logger; + var cacheBuilder = + Caffeine.newBuilder().scheduler(Scheduler.systemScheduler()).expireAfterAccess(ttl); + + if (maxTargetConnections >= 0) { + cacheBuilder = cacheBuilder.maximumSize(maxTargetConnections); + } + this.connections = - Caffeine.newBuilder() - .scheduler(Scheduler.systemScheduler()) - .expireAfterAccess(ttl) + cacheBuilder .removalListener( new RemovalListener() { @Override diff --git a/src/test/java/io/cryostat/net/TargetConnectionManagerTest.java b/src/test/java/io/cryostat/net/TargetConnectionManagerTest.java index 6453f51356..7af3a7d0a4 100644 --- a/src/test/java/io/cryostat/net/TargetConnectionManagerTest.java +++ b/src/test/java/io/cryostat/net/TargetConnectionManagerTest.java @@ -68,7 +68,7 @@ class TargetConnectionManagerTest { @BeforeEach void setup() { - this.mgr = new TargetConnectionManager(() -> jfrConnectionToolkit, TTL, logger); + this.mgr = new TargetConnectionManager(() -> jfrConnectionToolkit, TTL, 1, logger); } @Test @@ -180,7 +180,7 @@ public JFRConnection answer(InvocationOnMock invocation) void shouldCreateNewConnectionForAccessDelayedLongerThanTTL() throws Exception { TargetConnectionManager mgr = new TargetConnectionManager( - () -> jfrConnectionToolkit, Duration.ofNanos(1), logger); + () -> jfrConnectionToolkit, Duration.ofNanos(1), 1, logger); Mockito.when(jfrConnectionToolkit.createServiceURL(Mockito.anyString(), Mockito.anyInt())) .thenAnswer( new Answer() { @@ -210,4 +210,74 @@ 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, Duration.ofSeconds(10), 0, logger); + Mockito.when(jfrConnectionToolkit.createServiceURL(Mockito.anyString(), Mockito.anyInt())) + .thenAnswer( + new Answer() { + @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() { + @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); + Thread.sleep(10); + 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, Duration.ofSeconds(10), 2, logger); + Mockito.when(jfrConnectionToolkit.createServiceURL(Mockito.anyString(), Mockito.anyInt())) + .thenAnswer( + new Answer() { + @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() { + @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))); + } } From aa3c6e3ccd0d567fac3a359e133cf57cf5e6870b Mon Sep 17 00:00:00 2001 From: Andrew Azores Date: Tue, 10 Aug 2021 10:37:42 -0400 Subject: [PATCH 2/8] feat(targetconnectionmanager): configurable cache TTL --- pom.xml | 2 ++ .../java/io/cryostat/net/NetworkModule.java | 14 +++++--- .../cryostat/net/TargetConnectionManager.java | 35 ++++++++----------- 3 files changed, 27 insertions(+), 24 deletions(-) diff --git a/pom.xml b/pom.xml index 70a36365be..71a5a31e96 100644 --- a/pom.xml +++ b/pom.xml @@ -356,6 +356,8 @@ --env CRYOSTAT_MAX_TARGET_CONNECTIONS=1 --env + CRYOSTAT_MAX_TARGET_TTL=5 + --env CRYOSTAT_DISABLE_JMX_AUTH=true --env CRYOSTAT_DISABLE_SSL=true diff --git a/src/main/java/io/cryostat/net/NetworkModule.java b/src/main/java/io/cryostat/net/NetworkModule.java index e61ec8933e..027225d775 100644 --- a/src/main/java/io/cryostat/net/NetworkModule.java +++ b/src/main/java/io/cryostat/net/NetworkModule.java @@ -39,6 +39,7 @@ import java.net.SocketException; import java.net.UnknownHostException; +import java.time.Duration; import javax.inject.Named; import javax.inject.Singleton; @@ -70,6 +71,7 @@ public abstract class NetworkModule { static final String MAX_TARGET_CONNECTIONS = "CRYOSTAT_MAX_TARGET_CONNECTIONS"; + static final String MAX_TARGET_TTL = "CRYOSTAT_MAX_TARGET_TTL"; @Provides @Singleton @@ -97,17 +99,21 @@ static int provideMaxTargetConnections(Environment env) { return Integer.parseInt(env.getEnv(MAX_TARGET_CONNECTIONS, "-1")); } + @Provides + @Named(MAX_TARGET_TTL) + static Duration provideMaxTargetTTL(Environment env) { + return Duration.ofSeconds(Integer.parseInt(env.getEnv(MAX_TARGET_TTL, "10"))); + } + @Provides @Singleton static TargetConnectionManager provideTargetConnectionManager( Lazy connectionToolkit, + @Named(MAX_TARGET_TTL) Duration maxTargetTtl, @Named(MAX_TARGET_CONNECTIONS) int maxTargetConnections, Logger logger) { return new TargetConnectionManager( - connectionToolkit, - TargetConnectionManager.DEFAULT_TTL, - maxTargetConnections, - logger); + connectionToolkit, maxTargetTtl, maxTargetConnections, logger); } @Provides diff --git a/src/main/java/io/cryostat/net/TargetConnectionManager.java b/src/main/java/io/cryostat/net/TargetConnectionManager.java index d05200fd1c..77ef9fc4fd 100644 --- a/src/main/java/io/cryostat/net/TargetConnectionManager.java +++ b/src/main/java/io/cryostat/net/TargetConnectionManager.java @@ -83,15 +83,10 @@ public class TargetConnectionManager { this.jfrConnectionToolkit = jfrConnectionToolkit; this.logger = logger; - var cacheBuilder = - Caffeine.newBuilder().scheduler(Scheduler.systemScheduler()).expireAfterAccess(ttl); - - if (maxTargetConnections >= 0) { - cacheBuilder = cacheBuilder.maximumSize(maxTargetConnections); - } - - this.connections = - cacheBuilder + Caffeine cacheBuilder = + Caffeine.newBuilder() + .scheduler(Scheduler.systemScheduler()) + .expireAfterAccess(ttl) .removalListener( new RemovalListener() { @Override @@ -127,8 +122,11 @@ public void onRemoval( } } } - }) - .build(this::connect); + }); + if (maxTargetConnections >= 0) { + cacheBuilder = cacheBuilder.maximumSize(maxTargetConnections); + } + this.connections = cacheBuilder.build(this::connect); } public T executeConnectedTask( @@ -194,15 +192,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; From d115cf1fd73f3f5a96fdf1664a50b9f40baea35c Mon Sep 17 00:00:00 2001 From: Andrew Azores Date: Tue, 10 Aug 2021 10:46:52 -0400 Subject: [PATCH 3/8] chore(targetconnectionmanager): removed unused constant --- src/main/java/io/cryostat/net/TargetConnectionManager.java | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/main/java/io/cryostat/net/TargetConnectionManager.java b/src/main/java/io/cryostat/net/TargetConnectionManager.java index 77ef9fc4fd..f8619897e4 100644 --- a/src/main/java/io/cryostat/net/TargetConnectionManager.java +++ b/src/main/java/io/cryostat/net/TargetConnectionManager.java @@ -68,8 +68,6 @@ 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; private final Logger logger; From d066193ffa122d148f854251a4baecf5b00224cb Mon Sep 17 00:00:00 2001 From: Andrew Azores Date: Tue, 10 Aug 2021 13:45:17 -0400 Subject: [PATCH 4/8] doc(targetconnectionmanager): document env vars --- README.md | 7 +++++++ src/main/java/io/cryostat/net/TargetConnectionManager.java | 4 ++-- 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index fe6da6ae19..365eea8a73 100644 --- a/README.md +++ b/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_MAX_TARGET_CONNECTIONS` and +`CRYOSTAT_MAX_TARGET_TTL` are used to control the target JMX connection cache +behaviour. `CRYOSTAT_MAX_TARGET_CONNECTIONS` specifies how many connections may +be held open at once. `-1` may be used to leave the cache size unlimited. +`CRYOSTAT_MAX_TARGET_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 diff --git a/src/main/java/io/cryostat/net/TargetConnectionManager.java b/src/main/java/io/cryostat/net/TargetConnectionManager.java index f8619897e4..5affb13781 100644 --- a/src/main/java/io/cryostat/net/TargetConnectionManager.java +++ b/src/main/java/io/cryostat/net/TargetConnectionManager.java @@ -134,8 +134,8 @@ public 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.MAX_TARGET_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 From 6c8663fef2101bdafdd62aed5724d731766f3a0b Mon Sep 17 00:00:00 2001 From: Andrew Azores Date: Tue, 10 Aug 2021 13:49:11 -0400 Subject: [PATCH 5/8] feat(run.sh): add target connections env vars --- run.sh | 2 ++ 1 file changed, 2 insertions(+) diff --git a/run.sh b/run.sh index 3cfa1e892a..b106de1f4e 100755 --- a/run.sh +++ b/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_MAX_TARGET_CONNECTIONS=$CRYOSTAT_MAX_TARGET_CONNECTIONS \ + -e CRYOSTAT_MAX_TARGET_TTL=$CRYOSTAT_MAX_TARGET_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" \ From 2234c00aa98e89f61803751a3158aa27dc531cf6 Mon Sep 17 00:00:00 2001 From: Andrew Azores Date: Tue, 10 Aug 2021 13:49:32 -0400 Subject: [PATCH 6/8] doc(targetconnectionmanager): update javadoc --- .../io/cryostat/net/TargetConnectionManager.java | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/main/java/io/cryostat/net/TargetConnectionManager.java b/src/main/java/io/cryostat/net/TargetConnectionManager.java index 5affb13781..7591a7c047 100644 --- a/src/main/java/io/cryostat/net/TargetConnectionManager.java +++ b/src/main/java/io/cryostat/net/TargetConnectionManager.java @@ -134,13 +134,13 @@ public T executeConnectedTask( /** * Mark a connection as still in use by the consumer. Connections expire from cache and are - * automatically closed after {@link NetworkModule.MAX_TARGET_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 - * future from the time this method is called. This may be done repeatedly as long as the - * connection is required to remain active. + * automatically closed after {@link NetworkModule.MAX_TARGET_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 future from the time + * this method is called. This may be done repeatedly as long as the connection is required to + * remain active. * * @return false if the connection for the specified {@link ConnectionDescriptor} was already * removed from cache, true if it is still active and was refreshed From f72d8ba13a67fe73356e24bcf1e98f178605bca4 Mon Sep 17 00:00:00 2001 From: Andrew Azores Date: Tue, 10 Aug 2021 13:56:19 -0400 Subject: [PATCH 7/8] fix(targetconnectionmanager): rename env vars --- README.md | 12 ++++++------ pom.xml | 4 ++-- run.sh | 4 ++-- src/main/java/io/cryostat/net/NetworkModule.java | 16 ++++++++-------- .../io/cryostat/net/TargetConnectionManager.java | 14 +++++++------- 5 files changed, 25 insertions(+), 25 deletions(-) diff --git a/README.md b/README.md index 365eea8a73..95aa391019 100644 --- a/README.md +++ b/README.md @@ -137,12 +137,12 @@ 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_MAX_TARGET_CONNECTIONS` and -`CRYOSTAT_MAX_TARGET_TTL` are used to control the target JMX connection cache -behaviour. `CRYOSTAT_MAX_TARGET_CONNECTIONS` specifies how many connections may -be held open at once. `-1` may be used to leave the cache size unlimited. -`CRYOSTAT_MAX_TARGET_TTL` specifies how long (in seconds) these connections will -be cached before they are closed due to inactivity. +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 diff --git a/pom.xml b/pom.xml index 71a5a31e96..e424e74d09 100644 --- a/pom.xml +++ b/pom.xml @@ -354,9 +354,9 @@ --mount type=tmpfs,target=/opt/cryostat.d/truststore.d --env - CRYOSTAT_MAX_TARGET_CONNECTIONS=1 + CRYOSTAT_TARGET_CACHE_MAX_CONNECTIONS=1 --env - CRYOSTAT_MAX_TARGET_TTL=5 + CRYOSTAT_TARGET_CACHE_TTL=5 --env CRYOSTAT_DISABLE_JMX_AUTH=true --env diff --git a/run.sh b/run.sh index b106de1f4e..1c4638b3cb 100755 --- a/run.sh +++ b/run.sh @@ -79,8 +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_MAX_TARGET_CONNECTIONS=$CRYOSTAT_MAX_TARGET_CONNECTIONS \ - -e CRYOSTAT_MAX_TARGET_TTL=$CRYOSTAT_MAX_TARGET_TTL \ + -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" \ diff --git a/src/main/java/io/cryostat/net/NetworkModule.java b/src/main/java/io/cryostat/net/NetworkModule.java index 027225d775..af0eeac1f9 100644 --- a/src/main/java/io/cryostat/net/NetworkModule.java +++ b/src/main/java/io/cryostat/net/NetworkModule.java @@ -70,8 +70,8 @@ }) public abstract class NetworkModule { - static final String MAX_TARGET_CONNECTIONS = "CRYOSTAT_MAX_TARGET_CONNECTIONS"; - static final String MAX_TARGET_TTL = "CRYOSTAT_MAX_TARGET_TTL"; + static final String TARGET_CACHE_SIZE = "CRYOSTAT_TARGET_CACHE_SIZE"; + static final String TARGET_CACHE_TTL = "CRYOSTAT_TARGET_CACHE_TTL"; @Provides @Singleton @@ -94,23 +94,23 @@ static NetworkResolver provideNetworkResolver() { } @Provides - @Named(MAX_TARGET_CONNECTIONS) + @Named(TARGET_CACHE_SIZE) static int provideMaxTargetConnections(Environment env) { - return Integer.parseInt(env.getEnv(MAX_TARGET_CONNECTIONS, "-1")); + return Integer.parseInt(env.getEnv(TARGET_CACHE_SIZE, "-1")); } @Provides - @Named(MAX_TARGET_TTL) + @Named(TARGET_CACHE_TTL) static Duration provideMaxTargetTTL(Environment env) { - return Duration.ofSeconds(Integer.parseInt(env.getEnv(MAX_TARGET_TTL, "10"))); + return Duration.ofSeconds(Integer.parseInt(env.getEnv(TARGET_CACHE_TTL, "10"))); } @Provides @Singleton static TargetConnectionManager provideTargetConnectionManager( Lazy connectionToolkit, - @Named(MAX_TARGET_TTL) Duration maxTargetTtl, - @Named(MAX_TARGET_CONNECTIONS) int maxTargetConnections, + @Named(TARGET_CACHE_TTL) Duration maxTargetTtl, + @Named(TARGET_CACHE_SIZE) int maxTargetConnections, Logger logger) { return new TargetConnectionManager( connectionToolkit, maxTargetTtl, maxTargetConnections, logger); diff --git a/src/main/java/io/cryostat/net/TargetConnectionManager.java b/src/main/java/io/cryostat/net/TargetConnectionManager.java index 7591a7c047..fc1379a9a9 100644 --- a/src/main/java/io/cryostat/net/TargetConnectionManager.java +++ b/src/main/java/io/cryostat/net/TargetConnectionManager.java @@ -134,13 +134,13 @@ public T executeConnectedTask( /** * Mark a connection as still in use by the consumer. Connections expire from cache and are - * automatically closed after {@link NetworkModule.MAX_TARGET_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 future from the time - * this method is called. This may be done repeatedly as long as the connection is required to - * remain active. + * 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 + * future from the time this method is called. This may be done repeatedly as long as the + * connection is required to remain active. * * @return false if the connection for the specified {@link ConnectionDescriptor} was already * removed from cache, true if it is still active and was refreshed From b5db1331b401cf6d40989f02aa7ca4135a66673b Mon Sep 17 00:00:00 2001 From: Andrew Azores Date: Tue, 10 Aug 2021 14:33:29 -0400 Subject: [PATCH 8/8] test(targetconnectionmanager): use direct execution and scheduling in tests --- .../java/io/cryostat/net/NetworkModule.java | 9 +++- .../cryostat/net/TargetConnectionManager.java | 6 ++- .../net/TargetConnectionManagerTest.java | 41 ++++++++++++++++--- 3 files changed, 49 insertions(+), 7 deletions(-) diff --git a/src/main/java/io/cryostat/net/NetworkModule.java b/src/main/java/io/cryostat/net/NetworkModule.java index af0eeac1f9..fa1af0aaad 100644 --- a/src/main/java/io/cryostat/net/NetworkModule.java +++ b/src/main/java/io/cryostat/net/NetworkModule.java @@ -40,6 +40,7 @@ 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; @@ -52,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; @@ -113,7 +115,12 @@ static TargetConnectionManager provideTargetConnectionManager( @Named(TARGET_CACHE_SIZE) int maxTargetConnections, Logger logger) { return new TargetConnectionManager( - connectionToolkit, maxTargetTtl, maxTargetConnections, logger); + connectionToolkit, + ForkJoinPool.commonPool(), + Scheduler.systemScheduler(), + maxTargetTtl, + maxTargetConnections, + logger); } @Provides diff --git a/src/main/java/io/cryostat/net/TargetConnectionManager.java b/src/main/java/io/cryostat/net/TargetConnectionManager.java index fc1379a9a9..a96ac0e9af 100644 --- a/src/main/java/io/cryostat/net/TargetConnectionManager.java +++ b/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; @@ -75,6 +76,8 @@ public class TargetConnectionManager { TargetConnectionManager( Lazy jfrConnectionToolkit, + Executor executor, + Scheduler scheduler, Duration ttl, int maxTargetConnections, Logger logger) { @@ -83,7 +86,8 @@ public class TargetConnectionManager { Caffeine cacheBuilder = Caffeine.newBuilder() - .scheduler(Scheduler.systemScheduler()) + .executor(executor) + .scheduler(scheduler) .expireAfterAccess(ttl) .removalListener( new RemovalListener() { diff --git a/src/test/java/io/cryostat/net/TargetConnectionManagerTest.java b/src/test/java/io/cryostat/net/TargetConnectionManagerTest.java index 7af3a7d0a4..1adbb0b401 100644 --- a/src/test/java/io/cryostat/net/TargetConnectionManagerTest.java +++ b/src/test/java/io/cryostat/net/TargetConnectionManagerTest.java @@ -39,6 +39,8 @@ import java.time.Duration; import java.util.List; +import java.util.concurrent.Executor; +import java.util.concurrent.ForkJoinPool; import javax.management.remote.JMXServiceURL; @@ -46,6 +48,7 @@ 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, 1, 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), 1, logger); + () -> jfrConnectionToolkit, + ForkJoinPool.commonPool(), + Scheduler.systemScheduler(), + Duration.ofNanos(1), + 1, + logger); Mockito.when(jfrConnectionToolkit.createServiceURL(Mockito.anyString(), Mockito.anyInt())) .thenAnswer( new Answer() { @@ -215,7 +230,12 @@ public JFRConnection answer(InvocationOnMock invocation) void shouldCreateNewConnectionWhenMaxSizeZeroed() throws Exception { TargetConnectionManager mgr = new TargetConnectionManager( - () -> jfrConnectionToolkit, Duration.ofSeconds(10), 0, logger); + () -> jfrConnectionToolkit, + new DirectExecutor(), + Scheduler.disabledScheduler(), + Duration.ofSeconds(1), + 0, + logger); Mockito.when(jfrConnectionToolkit.createServiceURL(Mockito.anyString(), Mockito.anyInt())) .thenAnswer( new Answer() { @@ -241,7 +261,6 @@ public JFRConnection answer(InvocationOnMock invocation) }); ConnectionDescriptor desc = new ConnectionDescriptor("foo"); JFRConnection conn1 = mgr.executeConnectedTask(desc, a -> a); - Thread.sleep(10); JFRConnection conn2 = mgr.executeConnectedTask(desc, a -> a); MatcherAssert.assertThat(conn1, Matchers.not(Matchers.sameInstance(conn2))); } @@ -250,7 +269,12 @@ public JFRConnection answer(InvocationOnMock invocation) void shouldCreateNewConnectionPerTarget() throws Exception { TargetConnectionManager mgr = new TargetConnectionManager( - () -> jfrConnectionToolkit, Duration.ofSeconds(10), 2, logger); + () -> jfrConnectionToolkit, + new DirectExecutor(), + Scheduler.disabledScheduler(), + Duration.ofNanos(1), + -1, + logger); Mockito.when(jfrConnectionToolkit.createServiceURL(Mockito.anyString(), Mockito.anyInt())) .thenAnswer( new Answer() { @@ -280,4 +304,11 @@ public JFRConnection answer(InvocationOnMock invocation) 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(); + } + } }