Fix #1060 Enable Slack#close() method to shutdown thread pools behind…

… its SlackConfig (#1064)
slackapi · Sep 29, 2022 · 505d437 · 505d437
1 parent 00c916e
commit 505d437
Show file tree

Hide file tree

Showing 7 changed files with 164 additions and 2 deletions.
diff --git a/slack-api-client/src/main/java/com/slack/api/Slack.java b/slack-api-client/src/main/java/com/slack/api/Slack.java
@@ -113,7 +113,12 @@ public SlackHttpClient getHttpClient() {
 
     @Override
     public void close() throws Exception {
-        getHttpClient().close();
+        if (getHttpClient() != null) {
+            getHttpClient().close();
+        }
+        if (getConfig() != null) {
+            getConfig().close();
+        }
     }
 
     /**

diff --git a/slack-api-client/src/main/java/com/slack/api/SlackConfig.java b/slack-api-client/src/main/java/com/slack/api/SlackConfig.java
@@ -23,9 +23,21 @@
 
 /**
  * The basic configuration of this SDK. Some settings can be propagated to submodules such as Bolt.
+ * <p>
+ * Please note that, if you are fine with the same settings for all Slack instances across an application,
+ * using a singleton instance of this config class is highly recommended.
+ * Also, a Slack instance is thread-safe, so you can use singleton for it too.
+ * <p>
+ * If you create a new SlackConfig instance for each Slack instance creation, each SlackConfig object
+ * can create thread pools for maintaining metrics data and async executions.
+ * In most use cases, this should not be intended. To avoid this, consider reusing SlackConfig objects
+ * as much as possible.
+ * <p>
+ * An alternative way is to set the statsEnabled flag to false.
+ * As long as you don't use the metrics and async API calls at all, there is no downside by doing so.
  */
 @Data
-public class SlackConfig {
+public class SlackConfig implements AutoCloseable {
 
     /**
      * The default instance is immutable. It's not allowed to modify the value runtime for any reasons.
@@ -36,6 +48,11 @@ void throwException() {
             throw new UnsupportedOperationException("This config is immutable");
         }
 
+        @Override
+        public void close() {
+            // We disable closing resources for this default singleton object to avoid misbehaviors
+        }
+
         @Override
         public void setFailOnUnknownProperties(boolean failOnUnknownProperties) {
             throwException();
@@ -342,4 +359,10 @@ public void synchronizeLibraryMaintainerMode() {
         sCIMConfig.getMetricsDatastore().setTraceMode(this.isLibraryMaintainerMode());
     }
 
+    @Override
+    public void close() throws Exception {
+        com.slack.api.methods.impl.ThreadPools.shutdownAll(methodsConfig);
+        com.slack.api.audit.impl.ThreadPools.shutdownAll(auditConfig);
+        com.slack.api.scim.impl.ThreadPools.shutdownAll(sCIMConfig);
+    }
 }
diff --git a/slack-api-client/src/main/java/com/slack/api/audit/impl/ThreadPools.java b/slack-api-client/src/main/java/com/slack/api/audit/impl/ThreadPools.java
@@ -36,6 +36,25 @@ public static ExecutorService getOrCreate(AuditConfig config, String enterpriseI
         }
     }
 
+    public static void shutdownAll(AuditConfig config) {
+        String providerInstanceId = config.getExecutorServiceProvider().getInstanceId();
+        if (ENTERPRISE_CUSTOM.get(providerInstanceId) != null) {
+            for (ConcurrentMap<String, ExecutorService> each : ENTERPRISE_CUSTOM.get(providerInstanceId).values()) {
+                for (ExecutorService es : each.values()) {
+                    es.shutdownNow();
+                }
+                each.clear();
+            }
+            ENTERPRISE_CUSTOM.remove(providerInstanceId);
+        }
+        if (ALL_DEFAULT.get(providerInstanceId) != null) {
+            for (ExecutorService es : ALL_DEFAULT.get(providerInstanceId).values()) {
+                es.shutdownNow();
+            }
+            ALL_DEFAULT.remove(providerInstanceId);
+        }
+    }
+
     private static ExecutorService buildNewExecutorService(AuditConfig config) {
         String threadGroupName = "slack-audit-logs-" + config.getExecutorName();
         int poolSize = config.getDefaultThreadPoolSize();

diff --git a/slack-api-client/src/main/java/com/slack/api/methods/impl/ThreadPools.java b/slack-api-client/src/main/java/com/slack/api/methods/impl/ThreadPools.java
@@ -36,6 +36,25 @@ public static ExecutorService getOrCreate(MethodsConfig config, String teamId) {
         }
     }
 
+    public static void shutdownAll(MethodsConfig config) {
+        String providerInstanceId = config.getExecutorServiceProvider().getInstanceId();
+        if (TEAM_CUSTOM.get(providerInstanceId) != null) {
+            for (ConcurrentMap<String, ExecutorService> each : TEAM_CUSTOM.get(providerInstanceId).values()) {
+                for (ExecutorService es : each.values()) {
+                    es.shutdownNow();
+                }
+                each.clear();
+            }
+            TEAM_CUSTOM.remove(providerInstanceId);
+        }
+        if (ALL_DEFAULT.get(providerInstanceId) != null) {
+            for (ExecutorService es : ALL_DEFAULT.get(providerInstanceId).values()) {
+                es.shutdownNow();
+            }
+            ALL_DEFAULT.remove(providerInstanceId);
+        }
+    }
+
     private static ExecutorService buildNewExecutorService(MethodsConfig config) {
         String threadGroupName = "slack-methods-" + config.getExecutorName();
         int poolSize = config.getDefaultThreadPoolSize();

diff --git a/slack-api-client/src/main/java/com/slack/api/scim/impl/ThreadPools.java b/slack-api-client/src/main/java/com/slack/api/scim/impl/ThreadPools.java
@@ -36,6 +36,25 @@ public static ExecutorService getOrCreate(SCIMConfig config, String enterpriseId
         }
     }
 
+    public static void shutdownAll(SCIMConfig config) {
+        String providerInstanceId = config.getExecutorServiceProvider().getInstanceId();
+        if (ENTERPRISE_CUSTOM.get(providerInstanceId) != null) {
+            for (ConcurrentMap<String, ExecutorService> each : ENTERPRISE_CUSTOM.get(providerInstanceId).values()) {
+                for (ExecutorService es : each.values()) {
+                    es.shutdownNow();
+                }
+                each.clear();
+            }
+            ENTERPRISE_CUSTOM.remove(providerInstanceId);
+        }
+        if (ALL_DEFAULT.get(providerInstanceId) != null) {
+            for (ExecutorService es : ALL_DEFAULT.get(providerInstanceId).values()) {
+                es.shutdownNow();
+            }
+            ALL_DEFAULT.remove(providerInstanceId);
+        }
+    }
+
     private static ExecutorService buildNewExecutorService(SCIMConfig config) {
         String threadGroupName = "slack-scim-" + config.getExecutorName();
         int poolSize = config.getDefaultThreadPoolSize();

diff --git a/slack-api-client/src/test/java/test_locally/SlackConfigTest.java b/slack-api-client/src/test/java/test_locally/SlackConfigTest.java
@@ -3,13 +3,15 @@
 import com.slack.api.Slack;
 import com.slack.api.SlackConfig;
 import com.slack.api.methods.MethodsConfig;
+import com.slack.api.methods.impl.ThreadPools;
 import com.slack.api.util.thread.DaemonThreadExecutorServiceProvider;
 import com.slack.api.util.thread.ExecutorServiceProvider;
 import org.junit.Test;
 
 import java.util.Collections;
 import java.util.concurrent.ExecutorService;
 import java.util.concurrent.ScheduledExecutorService;
+import java.util.concurrent.TimeUnit;
 
 import static org.hamcrest.CoreMatchers.is;
 import static org.hamcrest.CoreMatchers.notNullValue;
@@ -107,6 +109,17 @@ public void immutableDefaultConfig() {
             fail();
         } catch (UnsupportedOperationException ignored) {
         }
+        try {
+            SlackConfig.DEFAULT.setExecutorServiceProvider(null);
+            fail();
+        } catch (UnsupportedOperationException ignored) {
+        }
+        try {
+            // Calling close() should not work anything
+            SlackConfig.DEFAULT.close();
+        } catch (Exception ignored) {
+            fail();
+        }
     }
 
     @Test
@@ -140,4 +153,33 @@ public ScheduledExecutorService createThreadScheduledExecutor(String threadGroup
         }
     }
 
+    @Test
+    public void closeMethod() throws Exception {
+        SlackConfig config = new SlackConfig();
+        ExecutorService methods = ThreadPools.getDefault(config.getMethodsConfig());
+        ExecutorService scim = com.slack.api.scim.impl.ThreadPools.getDefault(config.getSCIMConfig());
+        ExecutorService audit = com.slack.api.audit.impl.ThreadPools.getDefault(config.getAuditConfig());
+        assertThat(methods.isShutdown(), is(false));
+        assertThat(scim.isShutdown(), is(false));
+        assertThat(audit.isShutdown(), is(false));
+        assertThat(methods.isTerminated(), is(false));
+        assertThat(scim.isTerminated(), is(false));
+        assertThat(audit.isTerminated(), is(false));
+
+        config.close();
+
+        assertThat(methods.isShutdown(), is(true));
+        assertThat(scim.isShutdown(), is(true));
+        assertThat(audit.isShutdown(), is(true));
+
+        // await for the termination for test stability
+        methods.awaitTermination(1, TimeUnit.MINUTES);
+        scim.awaitTermination(1, TimeUnit.MINUTES);
+        audit.awaitTermination(1, TimeUnit.MINUTES);
+
+        assertThat(methods.isTerminated(), is(true));
+        assertThat(scim.isTerminated(), is(true));
+        assertThat(audit.isTerminated(), is(true));
+    }
+
 }
diff --git a/slack-api-client/src/test/java/test_locally/SlackTest.java b/slack-api-client/src/test/java/test_locally/SlackTest.java
@@ -4,6 +4,7 @@
 import com.slack.api.SlackConfig;
 import com.slack.api.audit.AuditClient;
 import com.slack.api.methods.SlackApiException;
+import com.slack.api.methods.impl.ThreadPools;
 import com.slack.api.methods.response.api.ApiTestResponse;
 import com.slack.api.rtm.RTMClient;
 import com.slack.api.scim.SCIMClient;
@@ -16,8 +17,12 @@
 import util.MockWebhookServer;
 
 import java.io.IOException;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.TimeUnit;
 
 import static com.slack.api.webhook.WebhookPayloads.payload;
+import static org.hamcrest.CoreMatchers.is;
+import static org.hamcrest.MatcherAssert.assertThat;
 import static org.junit.Assert.*;
 
 public class SlackTest {
@@ -160,4 +165,34 @@ public void scimStats() {
         }
     }
 
+    @Test
+    public void closeMethod() throws Exception {
+        SlackConfig config = new SlackConfig();
+        Slack slack = Slack.getInstance(config);
+        ExecutorService methods = ThreadPools.getDefault(config.getMethodsConfig());
+        ExecutorService scim = com.slack.api.scim.impl.ThreadPools.getDefault(config.getSCIMConfig());
+        ExecutorService audit = com.slack.api.audit.impl.ThreadPools.getDefault(config.getAuditConfig());
+        assertThat(methods.isShutdown(), is(false));
+        assertThat(scim.isShutdown(), is(false));
+        assertThat(audit.isShutdown(), is(false));
+        assertThat(methods.isTerminated(), is(false));
+        assertThat(scim.isTerminated(), is(false));
+        assertThat(audit.isTerminated(), is(false));
+
+        slack.close();
+
+        assertThat(methods.isShutdown(), is(true));
+        assertThat(scim.isShutdown(), is(true));
+        assertThat(audit.isShutdown(), is(true));
+
+        // await for the termination for test stability
+        methods.awaitTermination(1, TimeUnit.MINUTES);
+        scim.awaitTermination(1, TimeUnit.MINUTES);
+        audit.awaitTermination(1, TimeUnit.MINUTES);
+
+        assertThat(methods.isTerminated(), is(true));
+        assertThat(scim.isTerminated(), is(true));
+        assertThat(audit.isTerminated(), is(true));
+    }
+
 }