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.

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

Socket Mode: Slow message consumption when listeners do not immediately return ack() #1409

Closed
1 of 6 tasks
Mugenor opened this issue Dec 27, 2024 · 3 comments · Fixed by #1411
Closed
1 of 6 tasks
Assignees
Labels
bug M-T: confirmed bug report. Issues are confirmed when the reproduction steps are documented enhancement M-T: A feature request for new functionality project:bolt project:slack-api-client project:slack-api-client
Milestone

Comments

@Mugenor
Copy link

Mugenor commented Dec 27, 2024

(Describe your issue and goal here)

Category (place an x in each of the [ ])

  • bolt (Bolt for Java)
  • bolt-jakarta-socket-mode (Bolt for Java - optional modules)
  • slack-api-client (Slack API Clients)
  • slack-api-model (Slack API Data Models)
  • slack-api-*-kotlin-extension (Kotlin Extensions for Slack API Clients)
  • slack-app-backend (The primitive layer of Bolt for Java)

Requirements

We are using java sdk in socket mode with tyrus client.
I just found out that it runs message processing single threaded and:

  1. it looks like it's not really intended.
    com.slack.api.socket_mode.SocketModeClient#initializeMessageProcessorExecutor
    It creates 10 (by default) jobs for processing messages with executor service created by the com.slack.api.util.thread.DaemonThreadExecutorServiceProvider#createThreadScheduledExecutor
    which creates a single threaded executor
    I've tried to test it by putting Thread.sleep for a couple of seconds in event processor and spamming messages and the message processing was very slow which proves the point.
    It looks like a bug to me.
  2. It's hard to configure concurrency – it's configured in the clients constructor, the client is created via JakartaSocketModeClientFactory#create(Slack, String) method which is called in private method SocketModeApp#buildSocketModeClientFactory that doesn't use any top-level properties that can overwrite the behaviour.
    It would be nice to have an easy option to change concurrency.
  3. It uses scheduled executor which significantly reduces the number of ExecutorServices that is possible to use for message processing. Is there a reason for using scheduled instead of a regular one? From my perspective switching to a regular one should simplify things, because it already encapsulates the queueing logic. + it would allow to use virtual threads executor
    If the used executor type were changed and the ability to provide the concrete implementation were given, it would mitigate the second problem in this list from my perspective

Let me know if I can clarify anything else

@seratch seratch added project:bolt project:slack-api-client project:slack-api-client and removed untriaged labels Jan 6, 2025
@seratch seratch added this to the 1.x milestone Jan 6, 2025
@seratch seratch changed the title No concurrent support in socket mode Socket Mode: Slow message consumption when listeners do not immediately return ack() Jan 6, 2025
@seratch
Copy link
Member

seratch commented Jan 6, 2025

Hi @Mugenor, thanks for sharing the detailed feedback. Indeed, the message processor part could be improved for better runtime performance when listeners have slow synchronous code execution.

I did some quick prototyping to explore ways to improve the framework's internals today but I still need more time. A quick recap is that both 1) just replacing the scheduled executor service and 2) increasing the concurrency do not bring significant improvements when sync code in listeners are slow. If you're interested, you can try the code out here.

Now I am thinking that enhancing bolt-jakarta-socket-mode/bolt-socket-mode may bring much more positive impact. I will check if this idea works tomorrow.

As a quick suggestion for you, please avoid having slow synchronous code in your event listeners. As long as your code returns ack() immediately, the message processor won't get stuck. Here is a quick code snippet demonstrating how to run time-consuming operations asynchronously:

app.event(MessageEvent.class, (req, ctx) -> {
  // You can use your own ExecutorService too
  app.executorService().submit(() -> {
    // Do anything asynchronously here
  });
  // This line will be synchronously executed
  return ctx.ack();
});

The document should have clearly mentioned this, but using ExecutorService within a listener is the recommended way to implement complex and time-consuming event listeners. Nonetheless, the message processor can indeed be improved, and I will continue working on it.

Thanks again for sharing this feedback.

seratch added a commit to seratch/java-slack-sdk that referenced this issue Jan 7, 2025
@seratch seratch self-assigned this Jan 7, 2025
@seratch seratch modified the milestones: 1.x, 1.45.0 Jan 7, 2025
@seratch seratch added bug M-T: confirmed bug report. Issues are confirmed when the reproduction steps are documented enhancement M-T: A feature request for new functionality labels Jan 7, 2025
@seratch
Copy link
Member

seratch commented Jan 7, 2025

Qiuck update: I've submitted pull request #1411 resolving this issue. If the changes are fine, we will include new "concurrency" option in the next release. If you're interested in trying it out earlier, you can copy and paste the code into your project for now.

@Mugenor
Copy link
Author

Mugenor commented Jan 7, 2025

That was quick, thank you!

About the asynchronous event handling: I'm doing it already, but, unfortunately, not every operation can be done asynchronously. At least the one I encountered – view submission. If I understand it correctly, it's expected that I'll handle the request synchronously and either will return an ack() (which will close the modal), or validation errors (which will be displayed on UI)

seratch added a commit that referenced this issue Jan 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug M-T: confirmed bug report. Issues are confirmed when the reproduction steps are documented enhancement M-T: A feature request for new functionality project:bolt project:slack-api-client project:slack-api-client
Projects
None yet
2 participants