Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix #1060 Enable Slack#close() method to shutdown thread pools behind its SlackConfig #1064

Merged
merged 2 commits into from
Sep 29, 2022
Merged

Fix #1060 Enable Slack#close() method to shutdown thread pools behind its SlackConfig #1064

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ec54466
Fix #1060 Enable Slack#close() method to shutdown thread pools behind…
seratch Sep 29, 2022
860be8a
Improve test code
seratch Sep 29, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion slack-api-client/src/main/java/com/slack/api/Slack.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}
}

/**
Expand Down
25 changes: 24 additions & 1 deletion slack-api-client/src/main/java/com/slack/api/SlackConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just realized that these docs are hosted on sonatype.org, and as far as I can tell, can't be previewed locally in Jekyll 😢 .
I wanted to check to see if the references to various classes (i.e. SlackConfig) and maybe even properties (i.e. statsEnabled) would link properly. I suppose we won't be able to tell until we merge + deploy?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI: You can package javadoc.jar files by running mvn command and can unzip the file to check the generated HTML!

* 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.
Expand All @@ -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();
Expand Down Expand Up @@ -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);
}
}
19 changes: 19 additions & 0 deletions slack-api-client/src/main/java/com/slack/api/audit/impl/ThreadPools.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
19 changes: 19 additions & 0 deletions slack-api-client/src/main/java/com/slack/api/methods/impl/ThreadPools.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
19 changes: 19 additions & 0 deletions slack-api-client/src/main/java/com/slack/api/scim/impl/ThreadPools.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
42 changes: 42 additions & 0 deletions slack-api-client/src/test/java/test_locally/SlackConfigTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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));
}

}
35 changes: 35 additions & 0 deletions slack-api-client/src/test/java/test_locally/SlackTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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 {
Expand Down Expand Up @@ -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));
}

}