WebSocketMessageBrokerStats.getExecutorStatsInfo() throws exception if Executor is not a ThreadPoolExecutor

[leandrodalbo]

spring-framework/spring-websocket/src/main/java/org/springframework/web/socket/config/WebSocketMessageBrokerStats.java

Line 209 in d034a1f

return str.substring(str.indexOf("pool"), str.length() - 1);

Hardcoded "pool" doesn't work, the class name ThreadPoolExecutor contains capital "P" instead of "p".

[sbrannen]

Hi @leandrodalbo,

Thanks for submitting your first issue for the Spring Framework!

Hardcoded "pool" doesn't work, the class name ThreadPoolExecutor contains capital "P" instead of "p".

The expected "pool" string comes from the implementation of java.util.concurrent.ThreadPoolExecutor.toString(). Thus, the lowercase "p" is not the issue.

However, I can see that an exception could be thrown if the Executor supplied to WebSocketMessageBrokerStats.getExecutorStatsInfo() is not an instance of ThreadPoolTaskExecutor or ThreadPoolExecutor.

For the sake of clarity, could you please provide the stack trace you encountered when the exception was thrown?

[sbrannen]

Have you overridden the clientInboundChannelExecutor bean or clientOutboundChannelExecutor bean to use a custom-configured TaskExecutor other than a ThreadPoolTaskExecutor?

[leandrodalbo]

I didn't override any of those beans when I discovered this. I was using a simple configuration class in combination with netty-reactor and netty-all libraries.

I haven't got access to the code right now but it was simple Message broker configuration with stomp relay.

When I was debugging it was using ThreadPoolTaskExecutor but because of the "P" instead "p" you get a -1 in the substring() method.

[leandrodalbo]

Overriding the the stats bean and using str.toLowercase() is a quick fix but maybe not the best option.

[sbrannen]

I haven't got access to the code right now but it was simple Message broker configuration with stomp relay.

I already have a fix in place locally, but I'd appreciate if you could supply the stack trace once you get a chance, since that will give us a better idea of what exactly went wrong, possibly allowing us to provide a more intelligent fix.

[leandrodalbo]

Exception

21-07-23 15:24:42.973 [ERROR] o.s.s.s.TaskUtils$LoggingErrorHandler - Unexpected error occurred in scheduled task
java.lang.StringIndexOutOfBoundsException: String index out of range: -1
    at java.lang.String.substring(String.java:1960)
    at org.springframework.web.socket.config.WebSocketMessageBrokerStats.getExecutorStatsInfo(WebSocketMessageBrokerStats.java:209)
    at org.springframework.web.socket.config.WebSocketMessageBrokerStats.getClientInboundExecutorStatsInfo(WebSocketMessageBrokerStats.java:17**8)

Configuration Class

@Configuration
@EnableWebSocketMessageBroker
public class WebSocketConfiguration implements WebSocketMessageBrokerConfigurer {
    private static final int BROKER_HEARTBEAT_INTERVAL_MILLIS = 20000;

    @Autowired
    private Environment env;

    @Override
    public void registerStompEndpoints(StompEndpointRegistry registry) {
        registry.addEndpoint("/ws")
                .withSockJS();
    }

    @Override
    public void configureMessageBroker(MessageBrokerRegistry registry) {

        if (Boolean.parseBoolean(env.getProperty("myprops.rabbitmq.enabled"))) {
            registry.setApplicationDestinationPrefixes("/cantsayit");
            registry.enableStompBrokerRelay("/topic/", "/queue/")
                    .setClientLogin(env.getProperty("myprops.rabbitmq.username"))
                    .setClientPasscode(env.getProperty("myprops.rabbitmq.password"))
                    .setSystemLogin(env.getProperty("myprops.rabbitmq.username"))
                    .setSystemPasscode(env.getProperty("myprops.rabbitmq.password"))
                    .setRelayHost(env.getProperty("myprops.rabbitmq.hostname"))
                    .setRelayPort(Integer.parseInt(env.getProperty("myprops.rabbitmq.port")))
                    .setSystemHeartbeatSendInterval(BROKER_HEARTBEAT_INTERVAL_MILLIS)
                    .setAutoStartup(true);
        } else {
            registry.setApplicationDestinationPrefixes("/cantsayit")
                    .enableSimpleBroker("/queue", "/topic");
        }
    }

}

Dependencies

	<dependencies>

		<dependency>
			<groupId>org.springframework.boot</groupId>
			<artifactId>spring-boot-starter-web</artifactId>
		</dependency>

		<dependency>
			<groupId>org.springframework.boot</groupId>
			<artifactId>spring-boot-starter-websocket</artifactId>
		</dependency>

		<dependency>
			<groupId>org.springframework.boot</groupId>
			<artifactId>spring-boot-starter-freemarker</artifactId>
		</dependency>

		<dependency>
			<groupId>org.springframework.boot</groupId>
			<artifactId>spring-boot-starter-security</artifactId>
		</dependency>

		<dependency>
			<groupId>org.springframework.boot</groupId>
			<artifactId>spring-boot-devtools</artifactId>
			<optional>true</optional>
		</dependency>
           
            <dependency>
               <groupId>io.projectreactor.netty</groupId>
              <artifactId>reactor-netty</artifactId>
            </dependency>

           <dependency>
               <groupId>io.netty</groupId>
               <artifactId>netty-all</artifactId>
         </dependency>

</dependencies>	

Using the netty-reactor starter gave me the same result.

[sbrannen]

Thanks for providing the feedback!

The exception stack trace is as I expected. It indicates that the clientInboundChannelExecutor bean has been overridden with a TaskExecutor that is not a ThreadPoolTaskExecutor. Another option is that something invoked WebSocketMessageBrokerStats.setInboundChannelExecutor(TaskExecutor) with such a TaskExecutor.

I tried to reproduce the error using the configuration you supplied, but I unfortunately was not able to reproduce the failure scenario.

In any case, I can fix the bug to ensure that a StringIndexOutOfBoundsException is not thrown in such scenarios.

Having said that, I would still like to reproduce the bug using your application in order to see if there is anything better we can do (for example, to support stats for other types of TaskExecutor). So if you could provide a minimal example application that reproduces the bug I would be grateful.

---

As a side note, adding the following bean to the ApplicationContext can be used to reproduce the bug.

@Bean
public TaskExecutor clientInboundChannelExecutor() {
	SimpleAsyncTaskExecutor executor = new SimpleAsyncTaskExecutor();
	executor.setThreadNamePrefix("clientInboundChannel-");
	return executor;
}

[sbrannen]

@leandrodalbo, I fixed this in 42edef0.

So please feel free to try this out in the upcoming 5.3.10 snapshot.

If you are able to provide us a minimal example that reproduces the error you encountered in your application, we will consider reopening this issue to investigate further options.

[leandrodalbo]

Hi, can't provide the app because it is not mine, but I think I found what is overriding the bean.

@Configuration
@EnableScheduling
@EnableAsync( mode = AdviceMode.ASPECTJ )
public class SchedulerConfiguration
{
	@Bean
	@Primary
	public TaskScheduler taskScheduler()
	{
		ThreadPoolTaskScheduler lThreadPoolTaskScheduler = new ThreadPoolTaskScheduler();
		lThreadPoolTaskScheduler.setPoolSize( 4 );
		return lThreadPoolTaskScheduler;
	}
}

I am not sure, I would need to debug it a bit more, anyway what you have done fixes the problem.

• I would like it to depend on the interface and not a concrete implementation.
• I having the a hardcoded value like that might be dangerous, what if somebody completely rename the class in the future.

Thanks a lot.

[sbrannen]

I would like it to depend on the interface and not a concrete implementation.

java.util.concurrent.ThreadPoolExecutor does not implement an interface that provides an API to access its stats.

Granted, depending on the format of the text returned by java.util.concurrent.ThreadPoolExecutor.toString() is a bit brittle, but that format has not changed from JDK 8 through JDK 16. And if it does change...

1. Our test suite will eventually fail, thereby notifying us of the change.
2. The code in my fix ensures that an exception is not thrown, resulting in "unknown" as the stats if the format changes.

having the a hardcoded value like that might be dangerous, what if somebody completely rename the class in the future.

java.util.concurrent.ThreadPoolExecutor is part of the standard JDK library and will not be renamed.

Thanks a lot.

You're welcome!