WangzJi commented on code in PR #8023:
URL: https://github.com/apache/incubator-seata/pull/8023#discussion_r3151744014
##########
pom.xml:
##########
@@ -130,6 +131,16 @@
</dependencyManagement>
<profiles>
+ <!-- profile: onlyBuildOnJDK21+ -->
+ <profile>
+ <id>JDK21Plus</id>
+ <activation>
+ <jdk>[21,)</jdk>
+ </activation>
+ <modules>
+ <module>threadpool-loom</module>
Review Comment:
**[Critical] `seata-threadpool-loom` is only added to the reactor build
modules and to `server/pom.xml`. It is NOT added to `all/pom.xml` (and
therefore not to `seata-spring-boot-starter`, which depends on `seata-all`).**
Result: an end user pulling `seata-spring-boot-starter` on JDK 21+ never
gets the virtual-thread provider on the classpath.
`transport.threadpool=virtual` (and `auto` on JDK 25+) silently falls back to
platform threads via `ThreadPoolExecutorFactory.loadOptional` returning `null`,
defeating the PR's stated goal for the most common client entry point.
Suggested fix: add a JDK21+ profile to `all/pom.xml` (and ideally
`seata-spring-boot-starter/pom.xml`) that pulls in `seata-threadpool-loom`,
mirroring `server/pom.xml:452-464`.
##########
threadpool-loom/src/main/java/org/apache/seata/common/thread/VirtualThreadPoolExecutor.java:
##########
@@ -0,0 +1,47 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.seata.common.thread;
+
+import java.util.concurrent.BlockingQueue;
+import java.util.concurrent.RejectedExecutionHandler;
+import java.util.concurrent.ThreadPoolExecutor;
+import java.util.concurrent.TimeUnit;
+
+/**
+ * Virtual-thread-backed thread pool implementation.
+ */
+public class VirtualThreadPoolExecutor extends ThreadPoolExecutor {
+
+ public VirtualThreadPoolExecutor(
+ String threadPrefix,
+ int corePoolSize,
+ int maximumPoolSize,
+ long keepAliveTime,
+ TimeUnit unit,
+ BlockingQueue<Runnable> workQueue,
+ boolean daemon,
+ RejectedExecutionHandler rejectedHandler) {
+ super(
+ corePoolSize,
+ maximumPoolSize,
+ keepAliveTime,
+ unit,
+ workQueue,
+ VirtualThreadFactoryHelper.newThreadFactory(threadPrefix,
daemon),
+ rejectedHandler);
Review Comment:
**[Critical] Implementation contradicts the PR description.**
The PR description says:
> For regular executors, virtual mode ignores `maximumPoolSize` and
`keepAliveTime`. Uses `Integer.MAX_VALUE` and `SynchronousQueue` for non-cached
virtual-thread execution.
But the actual code preserves the caller-provided `corePoolSize`,
`maximumPoolSize`, `keepAliveTime`, and `workQueue`. So this is functionally a
bounded platform-style pool with virtual workers — none of the unbounded
virtual-thread benefits are realized, and
`VirtualThreadPoolProviderTest.testVirtualThreadPoolExecutorKeepsConfiguredBounds`
actually locks in the wrong behavior with an assertion.
Please pick one and align all three (PR description / impl / tests):
1. Implement the design as described (`Integer.MAX_VALUE` +
`SynchronousQueue`, ignore caller bounds), OR
2. Update the PR description and Javadoc to clarify that virtual mode still
respects the caller's bounds.
This was also flagged independently by the Copilot review on 2026-04-10.
##########
threadpool/src/main/java/org/apache/seata/common/thread/ThreadPoolRuntimeEnvironment.java:
##########
@@ -0,0 +1,98 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.seata.common.thread;
+
+import org.apache.seata.common.ConfigurationKeys;
+import org.apache.seata.common.DefaultValues;
+import org.apache.seata.config.Configuration;
+import org.apache.seata.config.ConfigurationFactory;
+
+import java.util.function.IntSupplier;
+import java.util.function.Supplier;
+
+/**
+ * Runtime helper used to resolve the thread pool mode.
+ */
+final class ThreadPoolRuntimeEnvironment {
+
+ private static final Supplier<String> DEFAULT_THREAD_POOL_TYPE_SUPPLIER =
+ ThreadPoolRuntimeEnvironment::loadConfiguredThreadPoolType;
+ private static final IntSupplier DEFAULT_JDK_FEATURE_SUPPLIER =
ThreadPoolRuntimeEnvironment::javaFeatureVersion;
+
+ private static volatile Supplier<String> threadPoolTypeSupplier =
DEFAULT_THREAD_POOL_TYPE_SUPPLIER;
+ private static volatile IntSupplier jdkFeatureSupplier =
DEFAULT_JDK_FEATURE_SUPPLIER;
+
+ private ThreadPoolRuntimeEnvironment() {}
+
+ static ThreadPoolType resolveThreadPoolType() {
+ ThreadPoolType configuredType =
ThreadPoolType.from(threadPoolTypeSupplier.get());
+ if (configuredType == ThreadPoolType.PLATFORM) {
+ return ThreadPoolType.PLATFORM;
+ }
+ int jdkFeature = jdkFeatureSupplier.getAsInt();
+ if (configuredType == ThreadPoolType.VIRTUAL) {
+ return jdkFeature >= 21 ? ThreadPoolType.VIRTUAL :
ThreadPoolType.PLATFORM;
Review Comment:
**[Critical] Silent fallback when user explicitly requests virtual but JDK
is too old.**
This class has no `Logger` at all. When an operator sets
`transport.threadpool=virtual` on JDK <21, this line silently returns
`PLATFORM` with zero log output. Capacity-planning assumptions become wrong
with no breadcrumb to discover the issue.
The same issue applies to `ThreadPoolExecutorFactory.loadOptional` (line
174), which swallows `EnhancedServiceNotFoundException` without logging — so
when `transport.threadpool=virtual` is set on JDK 21+ but
`seata-threadpool-loom` is missing from the classpath (very likely on the
client side per the comment on `pom.xml:141`), there is again no log.
Suggested fix: add a `Logger`, and emit a one-shot WARN on the first
downgrade for each of:
- `virtual` requested on JDK <21 (this line)
- `virtual` requested but provider missing
(`ThreadPoolExecutorFactory.loadOptional`)
- unknown configured value coerced to `AUTO` (`ThreadPoolType.from`)
Use an `AtomicBoolean` to dedupe the warning.
##########
threadpool-loom/src/main/java/org/apache/seata/common/thread/VirtualScheduledThreadPoolExecutor.java:
##########
@@ -0,0 +1,31 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.seata.common.thread;
+
+import java.util.concurrent.RejectedExecutionHandler;
+import java.util.concurrent.ScheduledThreadPoolExecutor;
+
+/**
+ * Virtual-thread-backed scheduled thread pool implementation.
+ */
+public class VirtualScheduledThreadPoolExecutor extends
ScheduledThreadPoolExecutor {
+
+ public VirtualScheduledThreadPoolExecutor(
+ String threadPrefix, int corePoolSize, boolean daemon,
RejectedExecutionHandler rejectedHandler) {
+ super(corePoolSize,
VirtualThreadFactoryHelper.newThreadFactory(threadPrefix, daemon),
rejectedHandler);
Review Comment:
**[Critical] `ScheduledThreadPoolExecutor` + virtual threads is an
anti-pattern that re-introduces the very problem this PR aims to solve (see
issue #6724 "Prevent virtual thread pinned").**
`ScheduledThreadPoolExecutor` keeps `corePoolSize` worker threads parked
indefinitely on `DelayedWorkQueue.take()` (a native park inside a
`ReentrantLock` region). Virtual threads parking there will pin their carrier
threads for the lifetime of the pool, so:
1. The expected virtual-thread benefits (lightweight, scalable concurrency)
are zero.
2. Native code paths in registry clients (Nacos/etcd/JNI) can pin carriers
indefinitely.
3. Netty-adjacent paths lose `FastThreadLocalThread` and fall back to the
slower `InternalThreadLocalMap`.
Impact is wide: `RegistryHeartBeats`,
`DefaultCoordinator.{retryRollbacking,asyncCommitting,xaTwoPhaseTimeoutChecker}`,
`CommonFenceConfig`, etc. — 12+ migrated call sites all go through this path.
Recommended: have `VirtualThreadPoolProvider.newScheduledThreadPoolExecutor`
delegate to the platform implementation (or document explicitly that scheduled
executors stay on platform threads even when virtual mode is selected).
##########
seata-spring-boot-starter/src/main/java/org/apache/seata/spring/boot/autoconfigure/SeataSagaAutoConfiguration.java:
##########
@@ -118,20 +118,13 @@ public ThreadPoolExecutor sagaAsyncThreadPoolExecutor(
SagaAsyncThreadPoolProperties properties,
@Qualifier(SAGA_REJECTED_EXECUTION_HANDLER_BEAN_NAME)
RejectedExecutionHandler rejectedExecutionHandler) {
- ThreadPoolExecutorFactoryBean threadFactory = new
ThreadPoolExecutorFactoryBean();
-
threadFactory.setBeanName("sagaStateMachineThreadPoolExecutorFactory");
- threadFactory.setThreadNamePrefix("sagaAsyncExecute-");
- threadFactory.setCorePoolSize(properties.getCorePoolSize());
- threadFactory.setMaxPoolSize(properties.getMaxPoolSize());
- threadFactory.setKeepAliveSeconds(properties.getKeepAliveTime());
-
- return new ThreadPoolExecutor(
+ return ThreadPoolExecutorFactory.newThreadPoolExecutor(
+ "sagaAsyncExecute",
properties.getCorePoolSize(),
properties.getMaxPoolSize(),
properties.getKeepAliveTime(),
TimeUnit.SECONDS,
new LinkedBlockingQueue<>(),
- threadFactory,
rejectedExecutionHandler);
Review Comment:
**[Critical] Behavioral regression: Saga async threads change from
non-daemon to daemon.**
The previous code used Spring's `ThreadPoolExecutorFactoryBean`, whose
default `ThreadFactory` (`CustomizableThreadFactory`) creates **non-daemon**
threads unless explicitly set otherwise.
`ThreadPoolExecutorFactory.newThreadPoolExecutor(...)` (this overload, no
`daemon` arg) defaults to **`daemon=true`**.
Impact: in-flight Saga state-machine async tasks no longer block JVM
shutdown. On a graceful shutdown, running saga branches can be force-terminated
mid-execution, potentially leaving the saga in an inconsistent state.
Suggested fix: use the overload that takes an explicit `daemon=false` to
preserve prior semantics:
```java
return ThreadPoolExecutorFactory.newThreadPoolExecutor(
"sagaAsyncExecute",
properties.getCorePoolSize(),
properties.getMaxPoolSize(),
properties.getKeepAliveTime(),
TimeUnit.SECONDS,
new LinkedBlockingQueue<>(),
false, // daemon: preserve prior non-daemon behavior so shutdown
waits for in-flight saga branches
rejectedExecutionHandler);
```
This was also flagged in an existing PR inline review comment.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
