This is an automated email from the ASF dual-hosted git repository.
virajjasani pushed a commit to branch 5.3
in repository https://gitbox.apache.org/repos/asf/phoenix.git
The following commit(s) were added to refs/heads/5.3 by this push:
new 223d726ceb PHOENIX-8041 Client thread pools stop working after
ThreadGroup destruction (#2652)
223d726ceb is described below
commit 223d726cebbcad6959fef30b4a839ddbb8fd5e8d
Author: Viraj Jasani <[email protected]>
AuthorDate: Wed Nov 4 09:27:14 2026 -0800
PHOENIX-8041 Client thread pools stop working after ThreadGroup destruction
(#2652)
---
.../jdbc/PhoenixHAExecutorServiceProvider.java | 6 +-
.../java/org/apache/phoenix/job/JobManager.java | 9 +-
.../phoenix/job/ThreadGroupSafeThreadFactory.java | 56 ++++++++++
.../jdbc/PhoenixHAExecutorServiceProviderTest.java | 49 +++++++++
.../job/ThreadGroupSafeThreadFactoryTest.java | 117 +++++++++++++++++++++
5 files changed, 230 insertions(+), 7 deletions(-)
diff --git
a/phoenix-core-client/src/main/java/org/apache/phoenix/jdbc/PhoenixHAExecutorServiceProvider.java
b/phoenix-core-client/src/main/java/org/apache/phoenix/jdbc/PhoenixHAExecutorServiceProvider.java
index 0a51fcf770..442ca5d7dd 100644
---
a/phoenix-core-client/src/main/java/org/apache/phoenix/jdbc/PhoenixHAExecutorServiceProvider.java
+++
b/phoenix-core-client/src/main/java/org/apache/phoenix/jdbc/PhoenixHAExecutorServiceProvider.java
@@ -41,6 +41,7 @@ import java.util.concurrent.ThreadPoolExecutor;
import java.util.concurrent.TimeUnit;
import javax.annotation.Nullable;
import org.apache.hadoop.hbase.util.EnvironmentEdgeManager;
+import org.apache.phoenix.job.ThreadGroupSafeThreadFactory;
import org.apache.phoenix.monitoring.GlobalClientMetrics;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
@@ -197,8 +198,9 @@ public class PhoenixHAExecutorServiceProvider {
handler = new ThreadPoolExecutor.CallerRunsPolicy();
}
ThreadPoolExecutor pool = new PhoenixHAThreadPoolExecutor(maxPoolSize,
maxPoolSize,
- KEEP_ALIVE_TIME_SECONDS, TimeUnit.SECONDS, queue, new
ThreadFactoryBuilder().setDaemon(true)
- .setNameFormat(threadPoolNamePrefix + "-%d").build(),
+ KEEP_ALIVE_TIME_SECONDS, TimeUnit.SECONDS, queue,
+ new
ThreadFactoryBuilder().setDaemon(true).setNameFormat(threadPoolNamePrefix +
"-%d")
+ .setThreadFactory(new ThreadGroupSafeThreadFactory()).build(),
handler, metrics);
pool.allowCoreThreadTimeOut(true);
return pool;
diff --git
a/phoenix-core-client/src/main/java/org/apache/phoenix/job/JobManager.java
b/phoenix-core-client/src/main/java/org/apache/phoenix/job/JobManager.java
index 5c1c12fe97..a7731b2d76 100644
--- a/phoenix-core-client/src/main/java/org/apache/phoenix/job/JobManager.java
+++ b/phoenix-core-client/src/main/java/org/apache/phoenix/job/JobManager.java
@@ -25,7 +25,6 @@ import static
org.apache.phoenix.monitoring.GlobalClientMetrics.GLOBAL_TASK_QUEU
import java.util.concurrent.BlockingQueue;
import java.util.concurrent.Callable;
-import java.util.concurrent.Executors;
import java.util.concurrent.FutureTask;
import java.util.concurrent.RejectedExecutionException;
import java.util.concurrent.RejectedExecutionHandler;
@@ -209,9 +208,9 @@ public class JobManager<T> extends
AbstractRoundRobinQueue<T> {
}
/**
- * Extension of the default thread factory returned by {@code
Executors.defaultThreadFactory} that
- * sets the context classloader on newly-created threads to be a specific
classloader (and not the
- * context classloader of the calling thread).
+ * Extension of {@link ThreadGroupSafeThreadFactory} that sets the context
classloader on
+ * newly-created threads to be a specific classloader (and not the context
classloader of the
+ * calling thread).
* <p/>
* See {@link org.apache.phoenix.util.PhoenixContextExecutor} for the
rationale on changing the
* context classloader.
@@ -221,7 +220,7 @@ public class JobManager<T> extends
AbstractRoundRobinQueue<T> {
private final ClassLoader contextClassLoader;
public ContextClassLoaderThreadFactory(ClassLoader contextClassLoader) {
- baseFactory = Executors.defaultThreadFactory();
+ baseFactory = new ThreadGroupSafeThreadFactory();
this.contextClassLoader = contextClassLoader;
}
diff --git
a/phoenix-core-client/src/main/java/org/apache/phoenix/job/ThreadGroupSafeThreadFactory.java
b/phoenix-core-client/src/main/java/org/apache/phoenix/job/ThreadGroupSafeThreadFactory.java
new file mode 100644
index 0000000000..981f38c294
--- /dev/null
+++
b/phoenix-core-client/src/main/java/org/apache/phoenix/job/ThreadGroupSafeThreadFactory.java
@@ -0,0 +1,56 @@
+/*
+ * 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.phoenix.job;
+
+import java.util.concurrent.Executors;
+import java.util.concurrent.ThreadFactory;
+
+/**
+ * Delegates to {@link Executors#defaultThreadFactory()}, but keeps working
after the ThreadGroup
+ * captured by that factory is destroyed.
+ * <p/>
+ * The default factory places every new thread in the ThreadGroup of the
thread that created the
+ * factory. Long-lived pools (e.g. owned by the PhoenixDriver singleton) that
let idle core threads
+ * time out can end up with no live threads in that group. If the group is
then destroyed, every
+ * later attempt to add a worker fails with IllegalThreadStateException and
the pool is unusable. In
+ * that case this factory places the new thread in the ThreadGroup of the
calling thread instead,
+ * which is necessarily alive.
+ * <p/>
+ * ThreadGroups can be destroyed only up to JDK 18. From JDK 19,
ThreadGroup.destroy() is a no-op
+ * and the fallback is never used.
+ */
+public class ThreadGroupSafeThreadFactory implements ThreadFactory {
+ private final ThreadFactory delegate = Executors.defaultThreadFactory();
+
+ @Override
+ public Thread newThread(Runnable r) {
+ try {
+ return delegate.newThread(r);
+ } catch (IllegalThreadStateException e) {
+ // The captured group is destroyed, use the group of the calling thread.
+ Thread t = new Thread(r);
+ if (t.isDaemon()) {
+ t.setDaemon(false);
+ }
+ if (t.getPriority() != Thread.NORM_PRIORITY) {
+ t.setPriority(Thread.NORM_PRIORITY);
+ }
+ return t;
+ }
+ }
+}
diff --git
a/phoenix-core/src/test/java/org/apache/phoenix/jdbc/PhoenixHAExecutorServiceProviderTest.java
b/phoenix-core/src/test/java/org/apache/phoenix/jdbc/PhoenixHAExecutorServiceProviderTest.java
index bed7b5162d..4f85d62e93 100644
---
a/phoenix-core/src/test/java/org/apache/phoenix/jdbc/PhoenixHAExecutorServiceProviderTest.java
+++
b/phoenix-core/src/test/java/org/apache/phoenix/jdbc/PhoenixHAExecutorServiceProviderTest.java
@@ -21,10 +21,15 @@ import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertNotEquals;
import static org.junit.Assert.assertTrue;
+import static org.junit.Assume.assumeTrue;
+import java.util.Arrays;
import java.util.Properties;
import java.util.concurrent.CompletableFuture;
+import java.util.concurrent.ExecutorService;
import java.util.concurrent.ThreadPoolExecutor;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicReference;
import org.junit.After;
import org.junit.AfterClass;
import org.junit.Before;
@@ -120,6 +125,50 @@ public class PhoenixHAExecutorServiceProviderTest {
assertNotEquals(es1, es2);
}
+ @Test
+ public void testHAExecutorServiceSurvivesDestroyedCreatorThreadGroup()
throws Exception {
+ ThreadGroup creatorGroup = new
ThreadGroup("PhoenixHAExecutorServiceProviderTest-creator");
+ AtomicReference<Throwable> creatorError = new AtomicReference<>();
+ Thread creator = new Thread(creatorGroup, () -> {
+ try {
+ PhoenixHAExecutorServiceProvider.resetExecutor();
+ for (PhoenixHAExecutorServiceProvider.PhoenixHAClusterExecutorServices
c : PhoenixHAExecutorServiceProvider
+ .get(properties)) {
+ for (ExecutorService es : Arrays.asList(c.getExecutorService(),
+ c.getCloseExecutorService())) {
+ ((ThreadPoolExecutor) es).setKeepAliveTime(10,
TimeUnit.MILLISECONDS);
+ es.submit(() -> {
+ }).get();
+ }
+ }
+ } catch (Throwable t) {
+ creatorError.set(t);
+ }
+ }, "PhoenixHAExecutorServiceProviderTest-creator");
+ creator.start();
+ creator.join();
+ if (creatorError.get() != null) {
+ throw new AssertionError(creatorError.get());
+ }
+
+ // Wait for the idle core threads to time out so the next task needs a new
worker.
+ long deadline = System.currentTimeMillis() + 30000;
+ while (creatorGroup.activeCount() > 0 && System.currentTimeMillis() <
deadline) {
+ Thread.sleep(10);
+ }
+ creatorGroup.destroy();
+ // ThreadGroup.destroy() is a no-op on JDK 19+, so the problem cannot
occur there.
+ assumeTrue(creatorGroup.isDestroyed());
+
+ for (PhoenixHAExecutorServiceProvider.PhoenixHAClusterExecutorServices c :
PhoenixHAExecutorServiceProvider
+ .get(properties)) {
+ for (ExecutorService es : Arrays.asList(c.getExecutorService(),
+ c.getCloseExecutorService())) {
+ assertEquals("ok", es.submit(() -> "ok").get(30, TimeUnit.SECONDS));
+ }
+ }
+ }
+
private Runnable getWaitingRunnable(Object obj) {
return (() -> {
synchronized (obj) {
diff --git
a/phoenix-core/src/test/java/org/apache/phoenix/job/ThreadGroupSafeThreadFactoryTest.java
b/phoenix-core/src/test/java/org/apache/phoenix/job/ThreadGroupSafeThreadFactoryTest.java
new file mode 100644
index 0000000000..70636b7a0a
--- /dev/null
+++
b/phoenix-core/src/test/java/org/apache/phoenix/job/ThreadGroupSafeThreadFactoryTest.java
@@ -0,0 +1,117 @@
+/*
+ * 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.phoenix.job;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertSame;
+import static org.junit.Assume.assumeTrue;
+
+import java.util.concurrent.Callable;
+import java.util.concurrent.ThreadFactory;
+import java.util.concurrent.ThreadPoolExecutor;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicReference;
+import org.junit.Test;
+
+public class ThreadGroupSafeThreadFactoryTest {
+
+ /**
+ * Runs the callable in a new thread that belongs to the given group, and
returns its result.
+ */
+ private static <T> T runIn(ThreadGroup group, Callable<T> callable) throws
Exception {
+ AtomicReference<T> result = new AtomicReference<>();
+ AtomicReference<Throwable> error = new AtomicReference<>();
+ Thread creator = new Thread(group, () -> {
+ try {
+ result.set(callable.call());
+ } catch (Throwable t) {
+ error.set(t);
+ }
+ }, "creator");
+ creator.start();
+ creator.join();
+ if (error.get() != null) {
+ throw new AssertionError(error.get());
+ }
+ return result.get();
+ }
+
+ @Test
+ public void testUsesCreatorThreadGroup() throws Exception {
+ ThreadGroup creatorGroup = new
ThreadGroup("ThreadGroupSafeThreadFactoryTest-creator");
+ ThreadFactory factory = runIn(creatorGroup,
ThreadGroupSafeThreadFactory::new);
+ Thread caller = Thread.currentThread();
+ int callerPriority = caller.getPriority();
+ caller.setPriority(Thread.MIN_PRIORITY);
+ try {
+ Thread t = factory.newThread(() -> {
+ });
+ assertSame(creatorGroup, t.getThreadGroup());
+ assertFalse(t.isDaemon());
+ assertEquals(Thread.NORM_PRIORITY, t.getPriority());
+ } finally {
+ caller.setPriority(callerPriority);
+ creatorGroup.destroy();
+ }
+ }
+
+ @Test
+ public void testFallsBackToCallerThreadGroupWhenCreatorGroupIsDestroyed()
throws Exception {
+ ThreadGroup creatorGroup = new
ThreadGroup("ThreadGroupSafeThreadFactoryTest-creator");
+ ThreadFactory factory = runIn(creatorGroup,
ThreadGroupSafeThreadFactory::new);
+ creatorGroup.destroy();
+ // ThreadGroup.destroy() is a no-op on JDK 19+, so the problem cannot
occur there.
+ assumeTrue(creatorGroup.isDestroyed());
+
+ Thread t = factory.newThread(() -> {
+ });
+ assertSame(Thread.currentThread().getThreadGroup(), t.getThreadGroup());
+ assertFalse(t.isDaemon());
+ assertEquals(Thread.NORM_PRIORITY, t.getPriority());
+ }
+
+ @Test
+ public void testJobManagerExecutorSurvivesDestroyedCreatorThreadGroup()
throws Exception {
+ ThreadGroup creatorGroup = new
ThreadGroup("ThreadGroupSafeThreadFactoryTest-creator");
+ ThreadPoolExecutor exec = runIn(creatorGroup, () -> {
+ ThreadPoolExecutor e = JobManager.createThreadPoolExec(10, 2, 10, false);
+ e.submit(() -> {
+ }).get();
+ return e;
+ });
+ try {
+ // Wait for the idle core threads to time out so the next task needs a
new worker.
+ long deadline = System.currentTimeMillis() + 30000;
+ while (exec.getPoolSize() > 0 && System.currentTimeMillis() < deadline) {
+ Thread.sleep(10);
+ }
+ assertEquals(0, exec.getPoolSize());
+ while (creatorGroup.activeCount() > 0 && System.currentTimeMillis() <
deadline) {
+ Thread.sleep(10);
+ }
+ creatorGroup.destroy();
+ // ThreadGroup.destroy() is a no-op on JDK 19+, so the problem cannot
occur there.
+ assumeTrue(creatorGroup.isDestroyed());
+
+ assertEquals("ok", exec.submit(() -> "ok").get(30, TimeUnit.SECONDS));
+ } finally {
+ exec.shutdownNow();
+ }
+ }
+}
