Skip to content

Commit db76156

Browse files
hinermCopilot
hinerm
and
Copilot
committed
Stop waiting on idle workers for completion
TextEditor.Executer waits for child threads in its run ThreadGroup to terminate before clearing the running state. SciJava ThreadService workers inherit that group, and cached-pool workers remain alive while idle for roughly 60 seconds. As a result, the Script Editor can continue to report a completed script as running after its work has finished. Wait for ordinary child threads and active ThreadService work owned by the run, but exclude workers only when they are demonstrably idle in ThreadPoolExecutor.getTask(). Keep descendant enumeration for cancellation unchanged. Add regression tests for idle pooled workers, active ThreadService tasks, direct ThreadService-created threads, and ordinary child threads. Noted in fiji/fiji-llm#45 Co-authored-by: Copilot <copilot@github.com>
1 parent c8a248d commit db76156

2 files changed

Lines changed: 246 additions & 11 deletions

File tree

‎src/main/java/org/scijava/ui/swing/script/TextEditor.java‎

Lines changed: 76 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -2618,24 +2618,24 @@ public abstract class Executer extends ThreadGroup {
26182618
public void run() {
26192619
try {
26202620
execute();
2621-
// Wait until any children threads die:
2622-
int activeCount = getThreadGroup().activeCount();
2623-
while (activeCount > 1) {
2621+
// Wait for script-owned child threads, but not idle pooled workers.
2622+
List<Thread> threads = getThreadsToWaitFor();
2623+
while (threads.size() > 1) {
26242624
if (isInterrupted()) break;
26252625
try {
26262626
Thread.sleep(500);
2627-
final List<Thread> ts = getAllThreads();
2628-
activeCount = ts.size();
2629-
if (activeCount <= 1) break;
2630-
log.debug("Waiting for " + ts.size() + " threads to die");
2631-
int count_zSelector = 0;
2632-
for (final Thread t : ts) {
2627+
threads = getThreadsToWaitFor();
2628+
if (threads.size() <= 1) break;
2629+
log.debug("Waiting for " + threads.size() +
2630+
" threads to die");
2631+
int zSelectorCount = 0;
2632+
for (final Thread t : threads) {
26332633
if (t.getName().equals("zSelector")) {
2634-
count_zSelector++;
2634+
zSelectorCount++;
26352635
}
26362636
log.debug("THREAD: " + t.getName());
26372637
}
2638-
if (activeCount == count_zSelector + 1) {
2638+
if (threads.size() == zSelectorCount + 1) {
26392639
// Do not wait on the stack slice selector thread.
26402640
break;
26412641
}
@@ -2665,6 +2665,16 @@ public void run() {
26652665
};
26662666
}
26672667

2668+
private List<Thread> getThreadsToWaitFor() {
2669+
final Set<Thread> seen = new HashSet<>();
2670+
final List<Thread> threads = new ArrayList<>();
2671+
for (final Thread thread : getAllThreads()) {
2672+
if (seen.add(thread) && isScriptOwnedThread(thread, this,
2673+
threadService)) threads.add(thread);
2674+
}
2675+
return threads;
2676+
}
2677+
26682678
/** The method to extend, that will do the actual work. */
26692679
abstract void execute();
26702680

@@ -2726,6 +2736,61 @@ public String toString() {
27262736
}
27272737
}
27282738

2739+
static boolean isScriptOwnedThread(final Thread thread,
2740+
final ThreadGroup runGroup, final ThreadService threadService)
2741+
{
2742+
if (thread == null || runGroup == null) return false;
2743+
if (threadService == null) return isInThreadGroup(thread, runGroup);
2744+
2745+
final ThreadService.ThreadContext threadContext;
2746+
try {
2747+
threadContext = threadService.getThreadContext(thread);
2748+
}
2749+
catch (final RuntimeException e) {
2750+
return isInThreadGroup(thread, runGroup);
2751+
}
2752+
if (threadContext == null || threadContext == ThreadService.ThreadContext.NONE) {
2753+
return isInThreadGroup(thread, runGroup);
2754+
}
2755+
2756+
final Set<Thread> seenParents = new HashSet<>();
2757+
Thread parent = threadService.getParent(thread);
2758+
while (parent != null && seenParents.add(parent)) {
2759+
if (isInThreadGroup(parent, runGroup)) return true;
2760+
parent = threadService.getParent(parent);
2761+
}
2762+
return isInThreadGroup(thread, runGroup) && !isIdleThreadServiceWorker(thread);
2763+
}
2764+
2765+
private static boolean isIdleThreadServiceWorker(final Thread thread) {
2766+
final Thread.State state = thread.getState();
2767+
if (state != Thread.State.WAITING && state != Thread.State.TIMED_WAITING) {
2768+
return false;
2769+
}
2770+
2771+
final StackTraceElement[] stack = thread.getStackTrace();
2772+
for (int i = 0; i + 1 < stack.length; i++) {
2773+
if (stack[i].getClassName().equals(
2774+
"java.util.concurrent.ThreadPoolExecutor") && stack[i].getMethodName()
2775+
.equals("getTask") && stack[i + 1].getClassName().equals(
2776+
"java.util.concurrent.ThreadPoolExecutor") && stack[i + 1]
2777+
.getMethodName().equals("runWorker")) return true;
2778+
}
2779+
return false;
2780+
}
2781+
2782+
private static boolean isInThreadGroup(final Thread thread,
2783+
final ThreadGroup targetGroup)
2784+
{
2785+
if (thread == null) return false;
2786+
ThreadGroup group = thread.getThreadGroup();
2787+
while (group != null) {
2788+
if (group == targetGroup) return true;
2789+
group = group.getParent();
2790+
}
2791+
return false;
2792+
}
2793+
27292794
/** Returns a list of currently executing tasks */
27302795
public List<Executer> getExecutingTasks() {
27312796
return executingTasks;

‎src/test/java/org/scijava/ui/swing/script/TextEditorThreadOwnershipTest.java‎

Lines changed: 170 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,170 @@
1+
package org.scijava.ui.swing.script;
2+
3+
import static org.junit.Assert.assertFalse;
4+
import static org.junit.Assert.assertNotNull;
5+
import static org.junit.Assert.assertNull;
6+
import static org.junit.Assert.assertSame;
7+
import static org.junit.Assert.assertTrue;
8+
9+
import java.util.concurrent.CountDownLatch;
10+
import java.util.concurrent.Future;
11+
import java.util.concurrent.TimeUnit;
12+
import java.util.concurrent.atomic.AtomicReference;
13+
14+
import org.junit.After;
15+
import org.junit.Before;
16+
import org.junit.Test;
17+
import org.scijava.Context;
18+
import org.scijava.thread.ThreadService;
19+
20+
public class TextEditorThreadOwnershipTest {
21+
22+
private Context context;
23+
private ThreadService threadService;
24+
25+
@Before
26+
public void setUp() {
27+
context = new Context(ThreadService.class);
28+
threadService = context.getService(ThreadService.class);
29+
}
30+
31+
@After
32+
public void tearDown() {
33+
context.dispose();
34+
}
35+
36+
@Test
37+
public void testOrdinaryChildThreadBelongsToRun() {
38+
final ThreadGroup runGroup = new ThreadGroup("script-run");
39+
final Thread child = new Thread(runGroup, () -> {});
40+
child.setDaemon(true);
41+
42+
assertTrue(TextEditor.isScriptOwnedThread(child, runGroup, threadService));
43+
}
44+
45+
@Test
46+
public void testIdleThreadServiceWorkerDoesNotBelongToRun() throws Exception {
47+
final ThreadGroup runGroup = new ThreadGroup("script-run");
48+
final AtomicReference<Thread> worker = new AtomicReference<>();
49+
final AtomicReference<Future<?>> task = new AtomicReference<>();
50+
final Thread creator = new Thread(runGroup, () -> task.set(threadService.run(
51+
() -> worker.set(Thread.currentThread()))));
52+
creator.start();
53+
creator.join(5000);
54+
task.get().get(5, TimeUnit.SECONDS);
55+
awaitIdlePoolWorker(worker.get());
56+
57+
assertFalse("worker factory thread should finish", creator.isAlive());
58+
assertNotNull(worker.get());
59+
assertSame(runGroup, worker.get().getThreadGroup());
60+
assertNull(threadService.getParent(worker.get()));
61+
assertFalse(TextEditor.isScriptOwnedThread(worker.get(), runGroup,
62+
threadService));
63+
}
64+
65+
@Test
66+
public void testActiveThreadServiceWorkBelongsToRun() throws Exception {
67+
final ThreadGroup runGroup = new ThreadGroup("script-run");
68+
final CountDownLatch taskStarted = new CountDownLatch(1);
69+
final CountDownLatch releaseTask = new CountDownLatch(1);
70+
final CountDownLatch releaseScriptThread = new CountDownLatch(1);
71+
final AtomicReference<Thread> worker = new AtomicReference<>();
72+
final AtomicReference<Future<?>> task = new AtomicReference<>();
73+
final Thread scriptThread = new Thread(runGroup, () -> {
74+
task.set(threadService.run(() -> {
75+
worker.set(Thread.currentThread());
76+
taskStarted.countDown();
77+
await(releaseTask);
78+
}));
79+
await(releaseScriptThread);
80+
});
81+
82+
scriptThread.start();
83+
try {
84+
assertTrue("service task should start", taskStarted.await(5,
85+
TimeUnit.SECONDS));
86+
assertNotNull(worker.get());
87+
assertTrue(TextEditor.isScriptOwnedThread(worker.get(), runGroup,
88+
threadService));
89+
90+
releaseTask.countDown();
91+
task.get().get(5, TimeUnit.SECONDS);
92+
awaitIdlePoolWorker(worker.get());
93+
assertFalse(TextEditor.isScriptOwnedThread(worker.get(), runGroup,
94+
threadService));
95+
}
96+
finally {
97+
releaseTask.countDown();
98+
releaseScriptThread.countDown();
99+
scriptThread.join(5000);
100+
}
101+
assertFalse("script parent should finish", scriptThread.isAlive());
102+
}
103+
104+
@Test
105+
public void testActiveThreadServiceFactoryThreadBelongsToRun() throws Exception {
106+
final ThreadGroup runGroup = new ThreadGroup("script-run");
107+
final CountDownLatch taskStarted = new CountDownLatch(1);
108+
final CountDownLatch releaseTask = new CountDownLatch(1);
109+
final CountDownLatch releaseScriptThread = new CountDownLatch(1);
110+
final AtomicReference<Thread> worker = new AtomicReference<>();
111+
final Thread scriptThread = new Thread(runGroup, () -> {
112+
final Thread serviceThread = threadService.newThread(() -> {
113+
worker.set(Thread.currentThread());
114+
taskStarted.countDown();
115+
await(releaseTask);
116+
});
117+
serviceThread.start();
118+
await(releaseScriptThread);
119+
});
120+
121+
scriptThread.start();
122+
try {
123+
assertTrue("service thread should start", taskStarted.await(5,
124+
TimeUnit.SECONDS));
125+
assertNotNull(worker.get());
126+
assertNull(threadService.getParent(worker.get()));
127+
assertTrue(TextEditor.isScriptOwnedThread(worker.get(), runGroup,
128+
threadService));
129+
}
130+
finally {
131+
releaseTask.countDown();
132+
releaseScriptThread.countDown();
133+
scriptThread.join(5000);
134+
if (worker.get() != null) worker.get().join(5000);
135+
}
136+
assertFalse("script parent should finish", scriptThread.isAlive());
137+
assertFalse("service thread should finish", worker.get().isAlive());
138+
}
139+
140+
private static void await(final CountDownLatch latch) {
141+
try {
142+
latch.await();
143+
}
144+
catch (final InterruptedException e) {
145+
Thread.currentThread().interrupt();
146+
}
147+
}
148+
149+
private static void awaitIdlePoolWorker(final Thread worker) throws Exception {
150+
final long deadline = System.currentTimeMillis() + 5000;
151+
while (!isIdlePoolWorker(worker) && System.currentTimeMillis() < deadline) {
152+
Thread.sleep(10);
153+
}
154+
assertTrue("worker should be idle in ThreadPoolExecutor.getTask",
155+
isIdlePoolWorker(worker));
156+
}
157+
158+
private static boolean isIdlePoolWorker(final Thread thread) {
159+
if (thread == null || thread.getState() != Thread.State.TIMED_WAITING) return false;
160+
final StackTraceElement[] stack = thread.getStackTrace();
161+
for (int i = 0; i + 1 < stack.length; i++) {
162+
if (stack[i].getClassName().equals(
163+
"java.util.concurrent.ThreadPoolExecutor") && stack[i].getMethodName()
164+
.equals("getTask") && stack[i + 1].getClassName().equals(
165+
"java.util.concurrent.ThreadPoolExecutor") && stack[i + 1]
166+
.getMethodName().equals("runWorker")) return true;
167+
}
168+
return false;
169+
}
170+
}

0 commit comments

Comments
 (0)