Broken thread pools in OpenEMS

While debugging some race conditions, I came across this thread pool pattern that is used in several places across OpenEMS (especially in the backend):

private final ScheduledThreadPoolExecutor pool = new ScheduledThreadPoolExecutor(0, Thread.ofVirtual().factory());

(This example was taken from io.openems.common.bridge.http.AsyncBridgeHttpExecutor)

However, this pattern is flawed: It only allows one concurrent execution to happen, as can be shown using this proof of concept:

package io.openems.common.bridge.http;

import java.util.concurrent.ScheduledThreadPoolExecutor;
import java.util.concurrent.TimeUnit;

public class ThreadPoolTest {

	private static final ScheduledThreadPoolExecutor pool = new ScheduledThreadPoolExecutor(0,
			Thread.ofVirtual().factory());

	public static void main(String[] args) {
		pool.setMaximumPoolSize(10);
		for (int i = 0; i < 5; i++) {
			final var j = i;
			pool.execute(() -> {
				System.out.println("Task " + j + " sleeping for 5 seconds...");
				try {
					Thread.sleep(5000);
				} catch (InterruptedException e) {
					e.printStackTrace();
				}
				System.out.println("Hello from task " + j);
			});
		}

		System.out.println("Submitted 5 tasks, waiting for termination...");

		try {
			pool.awaitTermination(30, TimeUnit.SECONDS);
		} catch (InterruptedException e) {
			e.printStackTrace();
		}
	}

}

Oracle’s documentation on Virtual Threads explicitly states that virtual threads should never be pooled, and recommends using the following executor instead:

Executors.newVirtualThreadPerTaskExecutor()

I was considering opening a PR to fix this in the HTTP Bridge, but considering that this pattern is used frequently across the code base, I thought it might be best to discuss it here first. I am assuminig that this can be a large performance bottleneck on backend instances with many edges, however I am not an expert in the area of multithreading.

That makes sense. Thank you for sharing the problem.

The thread pools there are mostly for concurrent executions without a lot of CPU usage in it. Most of the code is doing pretty basic CPU stuff or is using CompletableFuture’s, the slowest part are the IO operations to the timescale database and metadata provider.

I think the example code with Thread.sleep() is not ideal because Thread.sleep() puts the underlying thread into waiting. Can you maybe test it with a slow CompletableFuture?

I agree that we should change the codeplaces.
newVirtualThreadPerTaskExecutor() could be bad because there is no concurrent limit there. Do you know of a alternative with concurrency limitation?

Thread.sleep() is definitely not a perfect comparison, but I have observed this behavior on the actual OpenEMS Backend with the Odoo metadata provider. I believe this could be the root cause of the issue described in aa79e27, although I can’t be certain without more context. It would definitely fit this pattern though, as the thread pools that the commit reverted to use traditional platform threads.

Regarding concurrency limits, the idiomatic way to introduce that seems to be using a Semaphore as described in the Virtual Threads documentation. It gets a bit trickier when trying to introduce scheduling, but the general recommendation there seems to be using a single platform thread to schedule the virtual threads.

I have implemented a fix for the HTTP Bridge based on these patterns. I will submit it as a PR in the next few days. Maybe it can also serve as a blueprint for other places in which virtual threads are used for network requests.