spring-projects / spring-framework

Spring Framework
https://spring.io/projects/spring-framework
Apache License 2.0
55.74k stars 37.8k forks source link

Handle SimpleAsyncTaskExecutor in WebSocketMessageBrokerStats #33104

Closed csh0034 closed 1 week ago

csh0034 commented 3 weeks ago

When using the following executor with virtual threads:

SimpleAsyncTaskExecutor executor = new SimpleAsyncTaskExecutor();
executor.setVirtualThreads(true);

The WebSocketMessageBrokerStats#getExecutorStatsInfo method returns "unknown".

private String getExecutorStatsInfo(@Nullable Executor executor) {
  if (executor == null) {
    return "null";
  }

  if (executor instanceof ThreadPoolTaskExecutor threadPoolTaskScheduler) {
    executor = threadPoolTaskScheduler.getThreadPoolExecutor();
  }

  if (executor instanceof ThreadPoolExecutor) {
    // It is assumed that the implementation of toString() in ThreadPoolExecutor
    // generates text that ends similar to the following:
    // pool size = #, active threads = #, queued tasks = #, completed tasks = #]
    String str = executor.toString();
    int indexOfPool = str.indexOf("pool");
    if (indexOfPool != -1) {
      // (length - 1) omits the trailing "]"
      return str.substring(indexOfPool, str.length() - 1);
    }
  }

  return "unknown";
}

It seems that only thread-pooling TaskExecutor was considered originally. How about changing the wording?

Currently, it seems that there is no way to know if SimpleAsyncTaskExecutor is using virtual threads.

bclozel commented 3 weeks ago

What kind of statistics would you expect here? It doesn't seem we can get anything like "pool size = #, active threads = #, queued tasks = #, completed tasks = #" in this case.

csh0034 commented 3 weeks ago

Sorry, if the purpose is to expose pool information, then "unknown" is correct.

My intention was that after switching to using virtual threads, it showed "unknown" because virtual threads do not use pooling. It might be better to explicitly indicate that virtual threads are being used instead of showing "unknown".

snicoll commented 2 weeks ago

Thanks for the feedback. I think you have a point it should indicate it's using virtual threads. However, AFAIK, we do not have a way to figure out that an Executor is using VT behind the scenes.

Paging @jhoeller for feedback to see if I missed it or if we could consider adding that.

rstoyanchev commented 1 week ago

Maybe what really matters is that it's not a pool based strategy rather than whether it's virtual threads or not. In Spring for GraphQL we have a check like this to determine whether the executor is pooling or not.

snicoll commented 1 week ago

The stats being about pooling information, we should indeed expose something like thread-per-task rather than unknown for this case.