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

CarlosMOGoncalves commented 1 year ago

Hello,

I am not sure whether this is a bug or really some misuse of the Slack SDK by my side.

I stumbled into an issue where a long-running JakartaEE-based application was getting overwhelmed with slack-api-metrics threads. By the two week mark it had over 3900 of these threads, despite all of them being parked.

After some digging through the code and some debugging I realised that these threads are created everytime I instantiate a SlackConfig object which I did everytime I needed to call Slack API throught the SDK to send a message to a conversation.

Although I wasn't aware that these threads were created at all, I was expecting that by closing the Slack instance, these kind of resources could be freed or at least that the SDK realised that there were already some threads being used.

Below is the general aspect of the code I was using. Some time during the normal operation of the application there would be a need to send some messages, so I would invoke an EJB that would create a SlackConfig object (with slightly different settings), would get a Slack instance with that config and send the messages, closing it afterwards.

Reproducible in:

private void sendMessagesToChannel(List<SlackMessageDTO> slackMessages) {

        SlackConfig slackConfig = new SlackConfig();
        slackConfig.setHttpClientWriteTimeoutMillis(SECURITY_MARGIN_IN_MILLISSECONDS);
        slackConfig.setHttpClientReadTimeoutMillis(SECURITY_MARGIN_IN_MILLISSECONDS);
        slackConfig.setHttpClientCallTimeoutMillis(slackMessages.size() * SECURITY_MARGIN_IN_MILLISSECONDS);

        try (Slack slack = Slack.getInstance(slackConfig)) { 
            for (SlackMessageDTO slackMessage : slackMessages) {

                // Compose message

               // Send message
                slack.methods(slackToken)
                    .chatPostMessage(req -> req
                        .channel(slackMessage.channel)
                        .attachments(Collections.singletonList(attachment))
                        .text(slackMessage.message));
            }
        } catch (Exception e) {
           // Log issue
        }

    }

What happens here is:

the SDK creates an Executor with some initial worker (I think 6-9) threads called slack-api-metrics
everytime new messages are sent through that code, on each new SlackConfig object instantiated a further 3 new slack-api-metrics threads are created and added to the Executor
never terminates any of those, even when the close(). is called on Slack object, because that frees only the underlying OkHttpClient resources.

Naturally, when I realised that 3 slack-api-metrics threads were being created everytime I created a SlackConfig object (one thread for MethodsConfig, AuditConfig and SCIMConfig) I solved the problem by just having a single SlackConfig object for the whole application that would be used whenever I needed a new Slack instance.

However, I am wondering if:

it was only ever meant to be used one instance of SlackConfig on the entire application (if so, I think it would be useful to point that out somewhere, maybe in Javadoc) and maybe enforce it as a Singleton
if one can create multiple instances of SlackConfig then somehow it should clean its resources after the associated instance was closed, otherwise it can happen this kind of thread pollution in long-running apps
if I disable these metrics (one can do since recently) what will I lose? Only metric collection? Or does that impact other kind of functionality?
Also, one cannot completetely disable these metrics as stated here https://github.com/slackapi/java-slack-sdk/issues/987

Thanks in advance.

The Slack SDK version

1.25.1

Java Runtime version

OpenJDK 64-Bit Server VM Corretto-11.0.16.9.1

OS info

Windows 10 Enterprise

Steps to reproduce:

Well, the kind of code I posted there should do it. Sorry for not providing a reproducer project.

Expected result:

Created metrics threads should be removed after the Slack instance is closed.

Actual result:

The JVM keeps the created threads and adds 3 more threads per each call

Requirements

N/A

seratch commented 1 year ago

Hi @CarlosMOGoncalves, thanks a lot for writing in with details.

it was only ever meant to be used one instance of SlackConfig on the entire application (if so, I think it would be useful to point that out somewhere, maybe in Javadoc) and maybe enforce it as a Singleton

Yes, SlackConfig can be shared among Slack instances. So, sharing the config object for Slack instances is recommended when going with the same settings. Indeed, this should be clearly mentioned in the Javadoc and the api-client section on our website. We will improve this.

if one can create multiple instances of SlackConfig then somehow it should clean its resources after the associated instance was closed, otherwise it can happen this kind of thread pollution in long-running apps

I will look into this later on. I agree that calling the close() method should eliminate this type of closeable resource.

if I disable these metrics (one can do since recently) what will I lose? Only metric collection? Or does that impact other kind of functionality?

As long as you don't use the async API client and its smart rate limiter, there is no significant impact by disabling the metrics feature. Refer to https://slack.dev/java-slack-sdk/guides/web-api-basics#rate-limits for more details on the async client.

CarlosMOGoncalves commented 1 year ago

Hello @seratch ,

thank you for your quick answer. I think that was pretty elucidative.

I have opted for a single SlackConfig for the application, avoiding the creation of multiple threads and this sorted it out.

Thanks for your will to check into this. I think it might really be worth it given the possibility of the continuous thread creation. I think you can close this, then.

Cheers

slackapi / java-slack-sdk