From 505d4378276dd3599c0fc1ecbde2d9d82a35f855 Mon Sep 17 00:00:00 2001 From: Kazuhiro Sera Date: Fri, 30 Sep 2022 05:58:52 +0900 Subject: [PATCH] Fix #1060 Enable Slack#close() method to shutdown thread pools behind its SlackConfig (#1064) --- .../src/main/java/com/slack/api/Slack.java | 7 +++- .../main/java/com/slack/api/SlackConfig.java | 25 ++++++++++- .../com/slack/api/audit/impl/ThreadPools.java | 19 +++++++++ .../slack/api/methods/impl/ThreadPools.java | 19 +++++++++ .../com/slack/api/scim/impl/ThreadPools.java | 19 +++++++++ .../java/test_locally/SlackConfigTest.java | 42 +++++++++++++++++++ .../src/test/java/test_locally/SlackTest.java | 35 ++++++++++++++++ 7 files changed, 164 insertions(+), 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 index 5848c67f5..dd42758b6 100644 --- 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 index 5251a64d9..88e48ec4e 100644 --- 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. + *

+ * 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. + *

+ * 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. + *

+ * 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 index c5bc23ca3..ab595f332 100644 --- 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 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 index 78c9c9bea..1c8a1eae2 100644 --- 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 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 index 182a2b801..d78144ada 100644 --- 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 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 index 3a30c5563..6981d3c8a 100644 --- a/slack-api-client/src/test/java/test_locally/SlackConfigTest.java +++ b/slack-api-client/src/test/java/test_locally/SlackConfigTest.java @@ -3,6 +3,7 @@ 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; @@ -10,6 +11,7 @@ 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 index cc89acf62..92d7a2e5c 100644 --- 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)); + } + }