Updated AttributeMap to not close ExecutorService on Java 21 (#4649)

* Updated the implementation to shutdown ExecutorService, instead of closing it as happens as of Java 21

* Fixed import order

---------

Co-authored-by: Dongie Agnir <[email protected]>

.changes/next-release/bugfix-AWSSDKforJavav2-be113b9.json

@@ -0,0 +1,6 @@
+{
+    "type": "bugfix",
+    "category": "AWS SDK for Java v2",
+    "contributor": "michaeldimchuk",
+    "description": "Fixed the AttributeMap#close method trying to close an ExecutorService instance instead of shutting it down, resulting in a deadlock starting with Java 21"
+}

utils/src/main/java/software/amazon/awssdk/utils/AttributeMap.java

@@ -84,14 +84,17 @@ public AttributeMap copy() {
 
     @Override
     public void close() {
-        attributes.values().forEach(v -> IoUtils.closeIfCloseable(v, null));
-        attributes.values().forEach(this::shutdownIfExecutorService);
+        attributes.values().forEach(this::closeIfPossible);
     }
 
-    private void shutdownIfExecutorService(Object object) {
+    private void closeIfPossible(Object object) {
+        // We're explicitly checking for whether the provided object is an ExecutorService instance, because as of
+        // Java 21, it extends AutoCloseable, which triggers an ExecutorService#close call, which in turn can
+        // result in deadlocks. Instead, we safely shut it down, and close any other objects that are closeable.
         if (object instanceof ExecutorService) {
-            ExecutorService executor = (ExecutorService) object;
-            executor.shutdown();
+            ((ExecutorService) object).shutdown();
+        } else {
+            IoUtils.closeIfCloseable(object, null);
         }
     }
 

utils/src/test/java/software/amazon/awssdk/utils/AttributeMapTest.java

@@ -19,11 +19,15 @@
 import static org.assertj.core.api.Assertions.assertThat;
 import static org.junit.Assert.assertTrue;
 import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.never;
 import static org.mockito.Mockito.verify;
 
 import java.util.HashMap;
 import java.util.Map;
+import java.util.concurrent.CompletableFuture;
 import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.TimeUnit;
 import org.junit.Test;
 
 public class AttributeMapTest {
@@ -104,4 +108,52 @@ public void close_closesAll() {
         verify(executor).shutdown();
     }
 
+    @Test
+    public void close_ExecutorDoesNotDeadlockOnClose() throws Exception {
+        SdkAutoCloseable closeable = mock(SdkAutoCloseable.class);
+        ExecutorService executor = Executors.newSingleThreadExecutor();
+
+        AttributeMap attributeMap = AttributeMap.builder()
+                                                .put(CLOSEABLE_KEY, closeable)
+                                                .put(EXECUTOR_SERVICE_KEY, executor)
+                                                .build();
+
+        // Previously, running AttributeMap#close from a thread managed by the ExecutorService
+        // that's stored in that AttributeMap instance would result in a deadlock, where this
+        // invocation would time out. This verifies that that scenario no longer happens.
+        CompletableFuture.runAsync(attributeMap::close, executor).get(5L, TimeUnit.SECONDS);
+
+        verify(closeable).close();
+        assertThat(executor.isShutdown()).isTrue();
+    }
+
+    /**
+     * This tests that the {@link ExecutorService} which as of Java 21 implements the {@link AutoCloseable}
+     * interface, doesn't have its {@link AutoCloseable#close()} method called, but instead the expected
+     * {@link ExecutorService#shutdown()} method is.
+     *
+     * This test scenario can be removed when the SDK upgrades its minimum supported version to Java 21,
+     * whereupon this scenario will be handled by {@link AttributeMapTest#close_closesAll}.
+     */
+    @Test
+    public void close_shutsDownExecutorService() throws Exception {
+        SdkAutoCloseable closeable = mock(SdkAutoCloseable.class);
+        CloseableExecutorService executor = mock(CloseableExecutorService.class);
+
+        AttributeMap.builder()
+                    .put(CLOSEABLE_KEY, closeable)
+                    .put(EXECUTOR_SERVICE_KEY, executor)
+                    .build()
+                    .close();
+
+        verify(closeable).close();
+        verify(executor, never()).close();
+        verify(executor).shutdown();
+    }
+
+    /**
+     * Simulates the API contract of the ExecutorService as of Java 21, where it extends the
+     * {@link AutoCloseable} interface and is susceptible to being closed by {@link AttributeMap#close()}.
+     */
+    private interface CloseableExecutorService extends ExecutorService, AutoCloseable {}
 }