From 19f2887754ba752020933af81f4436287c860b5a Mon Sep 17 00:00:00 2001 From: Michael Bien Date: Mon, 5 Feb 2024 18:55:00 +0100 Subject: [PATCH 1/2] Threads should not call start() from within their own constructor - small cleanup: added missing overrides, made some fields final etc --- .../org/netbeans/core/execution/ExecutionEngine.java | 12 ++++++++---- .../org/netbeans/core/execution/RunClassThread.java | 2 +- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/platform/core.execution/src/org/netbeans/core/execution/ExecutionEngine.java b/platform/core.execution/src/org/netbeans/core/execution/ExecutionEngine.java index 78c2e51d2239..3ae04281f636 100644 --- a/platform/core.execution/src/org/netbeans/core/execution/ExecutionEngine.java +++ b/platform/core.execution/src/org/netbeans/core/execution/ExecutionEngine.java @@ -70,13 +70,13 @@ private static final IOTable taskIOs = new IOTable(base, systemIO); /* table of window:threadgrp */ - private static WindowTable wtable = new WindowTable(); + private static final WindowTable wtable = new WindowTable(); /** list of ExecutionListeners */ - private HashSet executionListeners = new HashSet(); + private final HashSet executionListeners = new HashSet<>(); /** List of running executions */ - private List runningTasks = Collections.synchronizedList(new ArrayList( 5 )); + private final List runningTasks = Collections.synchronizedList(new ArrayList<>(5)); static { systemIO.out = new OutputStreamWriter(System.out); @@ -131,13 +131,14 @@ public String getRunningTaskName( ExecutorTask task ) { * @param executor to start * @param info about class to start */ + @Override public ExecutorTask execute(String name, Runnable run, final InputOutput inout) { TaskThreadGroup g = new TaskThreadGroup(base, "exec_" + name + "_" + number); // NOI18N g.setDaemon(true); ExecutorTaskImpl task = new ExecutorTaskImpl(); synchronized (task.lock) { try { - new RunClassThread(g, name, number++, inout, this, task, Lookup.getDefault(), run); + new RunClassThread(g, name, number++, inout, this, task, Lookup.getDefault(), run).start(); task.lock.wait(); } catch (InterruptedException e) { // #171795 inout.closeInputOutput(); @@ -157,6 +158,7 @@ public ExecutorTask execute(String name, Runnable run, final InputOutput inout) * * @return class path to libraries */ + @Override protected NbClassPath createLibraryPath() { List l = Main.getModuleSystem().getModuleJars(); return new NbClassPath (l.toArray (new File[0])); @@ -181,6 +183,7 @@ public final void removeExecutionListener (ExecutionListener l) { * @param io an InputOutput * @return PermissionCollection for given CodeSource and InputOutput */ + @Override protected final PermissionCollection createPermissions(CodeSource cs, InputOutput io) { PermissionCollection pc = Policy.getPolicy().getPermissions(cs); ThreadGroup grp = Thread.currentThread().getThreadGroup(); @@ -252,6 +255,7 @@ static class SysOut extends OutputStream { this.std = std; } + @Override public void write(int b) throws IOException { if (std) { getTaskIOs().getOut().write(b); diff --git a/platform/core.execution/src/org/netbeans/core/execution/RunClassThread.java b/platform/core.execution/src/org/netbeans/core/execution/RunClassThread.java index e7776115126c..0ec910ff2056 100644 --- a/platform/core.execution/src/org/netbeans/core/execution/RunClassThread.java +++ b/platform/core.execution/src/org/netbeans/core/execution/RunClassThread.java @@ -71,7 +71,6 @@ public RunClassThread(TaskThreadGroup base, this.originalLookup = originalLookup; // #33789 - this thread must not be daemon otherwise it is immediately destroyed setDaemon(false); - this.start(); } /** runs the thread @@ -166,6 +165,7 @@ private void doRun() { } } // run method + @Override public InputOutput getInputOutput() { return io; } From dcb25800d6894baedc394f0293d392e99eebe1c0 Mon Sep 17 00:00:00 2001 From: Michael Bien Date: Mon, 5 Feb 2024 19:11:39 +0100 Subject: [PATCH 2/2] Fixed concurrency issues in GradleDaemonExecutor the internal GradleTask appears to cover s similar usecase as the CompletableFuture class. The implementation however caused a three state race condition: - executor calls finish(int) before someone calls result(): this seems to be the intended usecase to create a fast-path which finishes the task early - result() is called before finish(int): this will block on the wrapped task and finish has no effect. This behavior can be reproduced by attempting to delete a gradle project, the delete project dialog will stay open for almost a minute. - third state is when createTask() is called too late: this will cause a NPE since the task is already started via setTask(), the executor would try to call finish on a task wrapper which is null. Can be easily reproduced by setting a breakpoint in the codepath which calls createTask() or the method itself fixes: - block on CountDownLatch instead of delegate - create wrapper right when the task starts --- .../gradle/execute/GradleDaemonExecutor.java | 51 +++++++++++++++---- 1 file changed, 41 insertions(+), 10 deletions(-) diff --git a/extide/gradle/src/org/netbeans/modules/gradle/execute/GradleDaemonExecutor.java b/extide/gradle/src/org/netbeans/modules/gradle/execute/GradleDaemonExecutor.java index fee532381932..d3262bb02a68 100644 --- a/extide/gradle/src/org/netbeans/modules/gradle/execute/GradleDaemonExecutor.java +++ b/extide/gradle/src/org/netbeans/modules/gradle/execute/GradleDaemonExecutor.java @@ -37,7 +37,9 @@ import java.util.Collection; import java.util.HashMap; import java.util.Map; +import java.util.concurrent.CountDownLatch; import java.util.concurrent.ExecutionException; +import java.util.concurrent.TimeUnit; import java.util.logging.Level; import java.util.logging.Logger; import javax.swing.AbstractAction; @@ -439,41 +441,70 @@ private static String gradleExecutable() { return Utilities.isWindows() ? "bin\\gradle.bat" : "bin/gradle"; //NOI18N } - public final ExecutorTask createTask(ExecutorTask process) { + // TODO one of the two methods can likely go from the API + // setTask is called first and signals the runnable to start + // the started runnable requires (!) the gradleTask so it can't be created in createTask + @Override + public void setTask(ExecutorTask task) { assert gradleTask == null; - gradleTask = new GradleTask(process); + gradleTask = new GradleTask(task); + super.setTask(task); + } + + @Override + public final ExecutorTask createTask(ExecutorTask process) { + assert gradleTask != null; + assert task == process; return gradleTask; } + // task which can finish early, like a CompletableFuture private static final class GradleTask extends ExecutorTask { - private final ExecutorTask delegate; - private Integer result; + private final ExecutorTask delegate; + private volatile Integer result; + // is 0 when wrapper or delegate finished + private final CountDownLatch doneSignal = new CountDownLatch(1); + GradleTask(ExecutorTask delegate) { super(() -> {}); this.delegate = delegate; + this.delegate.addTaskListener(t -> doneSignal.countDown()); } @Override public void stop() { - this.delegate.stop(); + delegate.stop(); } @Override public int result() { - if (result != null) { - return result; - } - return this.delegate.result(); + waitFinished(); + return result != null ? result : delegate.result(); } @Override public InputOutput getInputOutput() { - return this.delegate.getInputOutput(); + return delegate.getInputOutput(); + } + + // FIXME isFinished() is final... this is still broken + + @Override + public boolean waitFinished(long milliseconds) throws InterruptedException { + return doneSignal.await(milliseconds, TimeUnit.MILLISECONDS); + } + + @Override + public void waitFinished() { + try { + doneSignal.await(); + } catch (InterruptedException ex) {} } public void finish(int result) { this.result = result; + doneSignal.countDown(); notifyFinished(); } }