fix: set a 2 second API call attempt timeout on clients the cache builds

README.md

@@ -106,6 +106,21 @@ String secret = cache.getSecretString("my-secret-id");
 
 **NOTE**: PQ-TLS uses the AWS Common Runtime (CRT) which relies on system libraries and may not work as expected on macOS or Windows at this time ([ref](https://github.com/awslabs/aws-crt-java#tls-behavior)).
 
+### Request Timeouts
+
+When the cache builds the Secrets Manager client, each API call attempt times out after `SecretCacheConfiguration.DEFAULT_API_CALL_ATTEMPT_TIMEOUT` (2 seconds). That covers `new SecretCache()`, a `SecretCacheConfiguration` with no client (including the Post-Quantum TLS example above), and a `SecretsManagerClientBuilder` you pass in. The SDK sets no attempt timeout by default, so without this an endpoint that stops answering can block the call for a long time. A timed out attempt is retried like any other network error.
+
+To use a different value, pass in a builder that sets `apiCallAttemptTimeout`; the cache keeps your value. The SDK has no setting that turns the timeout off, so use a large value if you expect slow calls, or pass in a built client. Only `apiCallAttemptTimeout` counts: a builder that sets just `apiCallTimeout` or HTTP client timeouts still gets the 2 second attempt timeout.
+
+If you pass in a built `SecretsManagerClient`, the cache uses it unchanged, so set the timeout yourself:
+
+```java
+SecretsManagerClient client = SecretsManagerClient.builder()
+    .overrideConfiguration(c -> c.apiCallAttemptTimeout(Duration.ofSeconds(2)))
+    .build();
+SecretCache cache = new SecretCache(client);
+```
+
 ## License
 
 This library is licensed under the Apache 2.0 License. 

src/main/java/com/amazonaws/secretsmanager/caching/SecretCache.java

@@ -13,6 +13,9 @@
 package com.amazonaws.secretsmanager.caching;
 
 import java.nio.ByteBuffer;
+import java.util.concurrent.ScheduledExecutorService;
+import java.util.concurrent.ScheduledThreadPoolExecutor;
+import java.util.concurrent.TimeUnit;
 
 import com.amazonaws.secretsmanager.caching.cache.LRUCache;
 import com.amazonaws.secretsmanager.caching.cache.SecretCacheItem;
@@ -55,9 +58,18 @@ public class SecretCache implements AutoCloseable {
     /** The AWS Secrets Manager client to use when requesting secrets. */
     private final SecretsManagerClient client;
 
+    /**
+     * Runs the API call attempt timeouts of every client the cache builds, unless its
+     * builder sets its own executor. Without it, each client would start its own pool
+     * of SDK timer threads, which only stop when the client is closed, and the cache
+     * does not close its client. The SDK never shuts down an executor it is given.
+     */
+    private static final ScheduledExecutorService TIMEOUT_EXECUTOR = timeoutExecutor();
+
     /**
      * Constructs a new secret cache using the standard AWS Secrets Manager client
-     * with default options.
+     * with default options, and
+     * {@link SecretCacheConfiguration#DEFAULT_API_CALL_ATTEMPT_TIMEOUT}.
      */
     public SecretCache() {
         this(SecretsManagerClient.builder());
@@ -66,7 +78,12 @@ public SecretCache() {
     /**
      * Constructs a new secret cache using an AWS Secrets Manager client created
      * using the
-     * provided builder.
+     * provided builder. Unless the builder sets its own API call attempt timeout,
+     * {@link SecretCacheConfiguration#DEFAULT_API_CALL_ATTEMPT_TIMEOUT} is used. The
+     * timeout, user agent and, unless the builder sets its own scheduled executor, the
+     * cache's shared timeout executor are written into the builder itself, so other
+     * clients later built from it get them too. Do not shut that executor down; every
+     * cache uses it.
      *
      * @param builder The builder to use for creating the AWS Secrets Manager
      *                client.
@@ -75,10 +92,7 @@ public SecretCache() {
         justification = "Delegates to constructor that validates before field initialization")
     public SecretCache(SecretsManagerClientBuilder builder) {
         this(new SecretCacheConfiguration().withClient(builder
-                .overrideConfiguration(
-                        builder.overrideConfiguration().toBuilder()
-                                .putAdvancedOption(SdkAdvancedClientOption.USER_AGENT_SUFFIX, VersionInfo.USER_AGENT)
-                                .build())
+                .overrideConfiguration(withCacheDefaults(builder.overrideConfiguration()))
                 .build()));
     }
 
@@ -95,7 +109,9 @@ public SecretCache(SecretsManagerClient client) {
     }
 
     /**
-     * Constructs a new secret cache using the provided cache configuration.
+     * Constructs a new secret cache using the provided cache configuration. If the
+     * configuration has no client, the cache builds one with
+     * {@link SecretCacheConfiguration#DEFAULT_API_CALL_ATTEMPT_TIMEOUT}.
      *
      * @param config The secret cache configuration.
      * @throws IllegalArgumentException if both a custom client and postQuantumTlsEnabled 
@@ -116,9 +132,6 @@ public SecretCache(SecretCacheConfiguration config) {
 
         this.cache = new LRUCache<String, SecretCacheItem>(config.getMaxCacheSize());
         this.config = config;
-        ClientOverrideConfiguration defaultOverride = ClientOverrideConfiguration.builder()
-                .putAdvancedOption(SdkAdvancedClientOption.USER_AGENT_SUFFIX, VersionInfo.USER_AGENT).build();
-
         if (config.getClient() != null) {
             this.client = config.getClient();
         } else {
@@ -130,8 +143,53 @@ public SecretCache(SecretCacheConfiguration config) {
                     .build());
             }
 
-            this.client = builder.overrideConfiguration(defaultOverride).build();
+            this.client = builder.overrideConfiguration(
+                    withCacheDefaults(ClientOverrideConfiguration.builder().build())).build();
+        }
+    }
+
+    /**
+     * Adds the cache's user agent suffix to the given override configuration and,
+     * unless it already sets them, the default API call attempt timeout and the
+     * cache's shared timeout executor.
+     *
+     * @param override The override configuration of the client being built.
+     * @return The override configuration to build the client with.
+     */
+    private static ClientOverrideConfiguration withCacheDefaults(ClientOverrideConfiguration override) {
+        ClientOverrideConfiguration.Builder builder = override.toBuilder()
+                .putAdvancedOption(SdkAdvancedClientOption.USER_AGENT_SUFFIX, VersionInfo.USER_AGENT);
+        if (!override.apiCallAttemptTimeout().isPresent()) {
+            builder.apiCallAttemptTimeout(SecretCacheConfiguration.DEFAULT_API_CALL_ATTEMPT_TIMEOUT);
         }
+        if (!override.scheduledExecutorService().isPresent()) {
+            builder.scheduledExecutorService(TIMEOUT_EXECUTOR);
+        }
+        return builder.build();
+    }
+
+    /**
+     * Creates the shared timeout executor. Its one daemon thread is started by the
+     * first timed request and exits after a minute with nothing to do, so an
+     * application server can unload the application. Cancelled timeouts are removed
+     * straight away.
+     *
+     * @return The shared timeout executor.
+     */
+    private static ScheduledExecutorService timeoutExecutor() {
+        ScheduledThreadPoolExecutor executor = new ScheduledThreadPoolExecutor(1, runnable -> {
+            Thread thread = new Thread(runnable, "aws-secretsmanager-caching-timeout");
+            thread.setDaemon(true);
+            // Don't keep the class loader of whichever application thread happened to start it.
+            thread.setContextClassLoader(SecretCache.class.getClassLoader());
+            return thread;
+        });
+        executor.setRemoveOnCancelPolicy(true);
+        // The keep-alive must be set first: it is 0 by default on Java 8, which
+        // allowCoreThreadTimeOut rejects.
+        executor.setKeepAliveTime(1, TimeUnit.MINUTES);
+        executor.allowCoreThreadTimeOut(true);
+        return executor;
     }
 
     /**

src/main/java/com/amazonaws/secretsmanager/caching/SecretCacheConfiguration.java

@@ -13,6 +13,7 @@
 
 package com.amazonaws.secretsmanager.caching;
 
+import java.time.Duration;
 import java.util.concurrent.TimeUnit;
 
 import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
@@ -39,6 +40,16 @@ public class SecretCacheConfiguration {
      */
     public static final long DEFAULT_FORCE_REFRESH_JITTER = 100;
 
+    /**
+     * The default timeout for each API call attempt made by a client that the cache
+     * builds, unless the client builder already sets one. The SDK has no attempt
+     * timeout by default, so an endpoint that accepts a connection and never answers
+     * blocks the caller until the HTTP client's own timeouts, if any, give up. A timed
+     * out attempt is retried like any other network error. A client passed in
+     * directly keeps its own timeouts.
+     */
+    public static final Duration DEFAULT_API_CALL_ATTEMPT_TIMEOUT = Duration.ofSeconds(2);
+
     /** The client this cache instance will use for accessing AWS Secrets Manager. */
     private SecretsManagerClient client = null;
 

src/test/java/com/amazonaws/secretsmanager/caching/SecretCacheTest.java

@@ -13,14 +13,20 @@
 
 package com.amazonaws.secretsmanager.caching;
 
+import java.net.InetAddress;
+import java.net.ServerSocket;
+import java.net.URI;
 import java.nio.ByteBuffer;
 import java.nio.charset.StandardCharsets;
+import java.time.Duration;
 import java.util.Arrays;
 import java.util.Collection;
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
+import java.util.concurrent.TimeUnit;
 import java.util.function.IntConsumer;
+import java.util.stream.Collectors;
 
 import org.mockito.ArgumentMatcher;
 import org.mockito.ArgumentMatchers;
@@ -31,7 +37,12 @@
 import org.testng.annotations.BeforeMethod;
 import org.testng.annotations.Test;
 
+import software.amazon.awssdk.auth.credentials.AwsBasicCredentials;
+import software.amazon.awssdk.auth.credentials.StaticCredentialsProvider;
+import software.amazon.awssdk.awscore.retry.AwsRetryStrategy;
 import software.amazon.awssdk.core.SdkBytes;
+import software.amazon.awssdk.core.exception.ApiCallAttemptTimeoutException;
+import software.amazon.awssdk.regions.Region;
 import software.amazon.awssdk.services.secretsmanager.SecretsManagerClient;
 import software.amazon.awssdk.services.secretsmanager.SecretsManagerClientBuilder;
 import software.amazon.awssdk.services.secretsmanager.model.DescribeSecretRequest;
@@ -526,4 +537,134 @@ public void testSecretCacheWithPQTLSEnabledNoClient() {
             }
         }
     }
+
+    /**
+     * Opens a socket that is never accepted. It completes the TCP handshake but
+     * never answers, like an endpoint that hangs after connecting.
+     */
+    private static ServerSocket hungEndpoint() throws Exception {
+        return new ServerSocket(0, 50, InetAddress.getByName("127.0.0.1"));
+    }
+
+    private static SecretsManagerClientBuilder hungEndpointClientBuilder(ServerSocket hung) {
+        return SecretsManagerClient.builder()
+                .endpointOverride(URI.create("http://127.0.0.1:" + hung.getLocalPort()))
+                .region(Region.US_WEST_2)
+                .credentialsProvider(StaticCredentialsProvider.create(AwsBasicCredentials.create("akid", "secret")));
+    }
+
+    private static long millisToTimeOut(SecretCache cache) {
+        long start = System.nanoTime();
+        Assert.expectThrows(ApiCallAttemptTimeoutException.class, () -> cache.getSecretString("test"));
+        return TimeUnit.NANOSECONDS.toMillis(System.nanoTime() - start);
+    }
+
+    @Test
+    public void testDefaultApiCallAttemptTimeout() {
+        Assert.assertEquals(SecretCacheConfiguration.DEFAULT_API_CALL_ATTEMPT_TIMEOUT, Duration.ofSeconds(2));
+    }
+
+    @Test
+    public void testBuilderClientTimesOutOnHungEndpoint() throws Exception {
+        try (ServerSocket hung = hungEndpoint();
+             SecretCache cache = new SecretCache(hungEndpointClientBuilder(hung)
+                     .overrideConfiguration(c -> c.retryStrategy(AwsRetryStrategy.doNotRetry())))) {
+            long elapsed = millisToTimeOut(cache);
+            Assert.assertTrue(elapsed >= 2000 && elapsed < 5000, "Took " + elapsed + " ms to give up");
+        }
+    }
+
+    @Test
+    public void testBuilderClientRetriesTimedOutAttempt() throws Exception {
+        try (ServerSocket hung = hungEndpoint();
+             SecretCache cache = new SecretCache(hungEndpointClientBuilder(hung)
+                     .overrideConfiguration(c -> c.retryStrategy(
+                             AwsRetryStrategy.standardRetryStrategy().toBuilder().maxAttempts(2).build())))) {
+            // Two 2 second attempts, plus a short backoff between them.
+            long elapsed = millisToTimeOut(cache);
+            Assert.assertTrue(elapsed >= 4000 && elapsed < 8000, "Took " + elapsed + " ms to give up");
+        }
+    }
+
+    @Test
+    public void testBuilderKeepsOwnAttemptTimeout() throws Exception {
+        try (ServerSocket hung = hungEndpoint();
+             SecretCache cache = new SecretCache(hungEndpointClientBuilder(hung)
+                     .overrideConfiguration(c -> c.retryStrategy(AwsRetryStrategy.doNotRetry())
+                             .apiCallAttemptTimeout(Duration.ofMillis(200))))) {
+            long elapsed = millisToTimeOut(cache);
+            Assert.assertTrue(elapsed < 2000,
+                    "Took " + elapsed + " ms to give up, so the builder's own timeout was replaced");
+        }
+    }
+
+    @Test
+    public void testDefaultClientTimesOutOnHungEndpoint() throws Exception {
+        String[] names = {"aws.region", "aws.endpointUrl", "aws.accessKeyId", "aws.secretAccessKey", "aws.maxAttempts"};
+        Map<String, String> original = new HashMap<>();
+        for (String name : names) {
+            original.put(name, System.getProperty(name));
+        }
+        try (ServerSocket hung = hungEndpoint()) {
+            System.setProperty("aws.region", "us-west-2");
+            System.setProperty("aws.endpointUrl", "http://127.0.0.1:" + hung.getLocalPort());
+            System.setProperty("aws.accessKeyId", "akid");
+            System.setProperty("aws.secretAccessKey", "secret");
+            System.setProperty("aws.maxAttempts", "1");
+            try (SecretCache cache = new SecretCache(new SecretCacheConfiguration())) {
+                long elapsed = millisToTimeOut(cache);
+                Assert.assertTrue(elapsed >= 2000 && elapsed < 5000, "Took " + elapsed + " ms to give up");
+            }
+        } finally {
+            for (String name : names) {
+                if (original.get(name) != null) {
+                    System.setProperty(name, original.get(name));
+                } else {
+                    System.clearProperty(name);
+                }
+            }
+        }
+    }
+
+    private static List<Thread> threadsNamed(String prefix) {
+        return Thread.getAllStackTraces().keySet().stream()
+                .filter(t -> t.getName().startsWith(prefix))
+                .collect(Collectors.toList());
+    }
+
+    @Test
+    public void testCachesShareOneTimeoutExecutor() throws Exception {
+        // Each client would otherwise start its own SDK timer threads, which stay
+        // alive because the cache doesn't close its client. That includes a builder
+        // that sets its own timeout.
+        int sdkThreadsBefore = threadsNamed("sdk-ScheduledExecutor").size();
+        try (ServerSocket hung = hungEndpoint()) {
+            for (int i = 0; i < 3; i++) {
+                try (SecretCache cache = new SecretCache(hungEndpointClientBuilder(hung)
+                        .overrideConfiguration(c -> c.retryStrategy(AwsRetryStrategy.doNotRetry())));
+                     SecretCache ownTimeout = new SecretCache(hungEndpointClientBuilder(hung)
+                        .overrideConfiguration(c -> c.retryStrategy(AwsRetryStrategy.doNotRetry())
+                                .apiCallAttemptTimeout(Duration.ofMillis(200))))) {
+                    millisToTimeOut(cache);
+                    millisToTimeOut(ownTimeout);
+                }
+            }
+        }
+        Assert.assertEquals(threadsNamed("sdk-ScheduledExecutor").size(), sdkThreadsBefore);
+
+        List<Thread> cacheThreads = threadsNamed("aws-secretsmanager-caching-timeout");
+        Assert.assertEquals(cacheThreads.size(), 1);
+        Assert.assertTrue(cacheThreads.get(0).isDaemon());
+        Assert.assertSame(cacheThreads.get(0).getContextClassLoader(), SecretCache.class.getClassLoader());
+    }
+
+    @Test
+    public void testCacheStillWorksAfterClose() {
+        Mockito.when(asm.describeSecret(Mockito.any(DescribeSecretRequest.class))).thenReturn(describeSecretResponse);
+        Mockito.when(describeSecretResponse.versionIdsToStages()).thenReturn(null);
+        SecretCache cache = new SecretCache(asm);
+        cache.close();
+        Assert.assertNull(cache.getSecretString("test"));
+        Mockito.verify(asm, Mockito.never()).close();
+    }
 }