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

[seratch]

This pull request resolves #1409 by adding a new "concurrency" option to the bolt-socket-mode and bolt-jakarta-socket-mode libraries.

The current implementation delays picking a new queued WebSocket message until the preceding operation returns an ack() WebSocket message to Slack. For context, we recommend that developers move all time-consuming tasks to app.executorService().execute (or a similar solution). If an app is built this way, there should be no issue with this mechanism. However, if a listener code takes a long time for a particular scenario, this situation can negatively impact the runtime performance of subsequent event handling. You can reproduce the issue with the following code snippet:

app.event(MessageEvent.class, (req, ctx) -> {
    // Without concurrency option, this time-consuming task slows the whole message processing mechanism down
    try {
        Thread.sleep(1000L);
    } catch (InterruptedException e) {
        throw new RuntimeException(e);
    }
    ctx.asyncClient().reactionsAdd(r -> r
            .channel(req.getEvent().getChannel())
            .name("eyes")
            .timestamp(req.getEvent().getTs())
    );
    return ctx.ack();
});

String appToken = System.getenv(Constants.SLACK_SDK_TEST_SOCKET_MODE_APP_TOKEN);
SocketModeApp socketModeApp = new SocketModeApp(appToken, app);
socketModeApp.start();

With the new concurrency option, Bolt apps can resiliently handle events at a constant pace (as long as there are available threads in the thread pool).

// SocketModeApp socketModeApp = new SocketModeApp(appToken, app);
SocketModeApp socketModeApp = new SocketModeApp(appToken, app, 10);

We won't change the default behavior this time, but it's worth considering enabling concurrency by default in future releases.

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

• bolt (Bolt for Java)
• bolt-{sub modules} (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

Please read the Contributing guidelines and Code of Conduct before creating this issue or pull request. By submitting, you agree to those rules.

[codecov]

Codecov Report

Attention: Patch coverage is 36.66667% with 57 lines in your changes missing coverage. Please review.

Project coverage is 73.33%. Comparing base (9e815b0) to head (b3a3273).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
.../com/slack/api/bolt/socket_mode/SocketModeApp.java	39.58%	27 Missing and 2 partials ⚠️
...ck/api/bolt/jakarta_socket_mode/SocketModeApp.java	33.33%	25 Missing and 3 partials ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #1411      +/-   ##
============================================
- Coverage     73.59%   73.33%   -0.27%     
- Complexity     4384     4386       +2     
============================================
  Files           476      476              
  Lines         14171    14223      +52     
  Branches       1430     1434       +4     
============================================
+ Hits          10429    10430       +1     
- Misses         2901     2950      +49     
- Partials        841      843       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[Mugenor]

Is there a reason for hiding ExecutorService initialisation?
IMO, it would be nice to be able to provide any type of executor service. This way consumers of the library could specify service with virtual threads, for example, or something completely custom (with metrics collection, for example)

[seratch]

We encourage making a daemon thread one like this code does. Are you planning to use a different one?

[seratch]

OK, I'd like to keep this parameter hidden from developers for now, but we may consider accepting ExecutorService in future updates. Thanks for your comment!

[WilliamBergamin]

Elegant implementation for a complex problem 💯

This looks good to me, left a question in a comment but should not be blocking

[WilliamBergamin]

Would there be any shortcomings from always using an executorService where the default concurrency = 1

Other then maintaining backwards compatible behavior is there a use case where

runBoltApp(message, app, client, requestParser, errorHandler, gson);

is preferred over

ExecutorService executorService = buildExecutorService(1);
executorService.execute(() -> runBoltApp(message, app, client, requestParser, errorHandler, gson));

[seratch]

Thanks for the suggestion! Indeed, this approach is also an option, but for now I'd like to avoid moving the runBoltApp to a different thread, even though it's a slight difference. When we make the concurrent mode the default, simplifying the code as you suggest is worth considering.