You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@netbeans.apache.org by "mbien (via GitHub)" <gi...@apache.org> on 2024/02/05 18:28:41 UTC
[PR] Fixed concurrency issues in GradleDaemonExecutor [netbeans]
mbien opened a new pull request, #7030:
URL: https://github.com/apache/netbeans/pull/7030
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
example:
```
java.lang.NullPointerException: Cannot invoke "org.netbeans.modules.gradle.execute.GradleDaemonExecutor$GradleTask.finish(int)" because "this.gradleTask" is null
at org.netbeans.modules.gradle.execute.GradleDaemonExecutor.run(GradleDaemonExecutor.java:263)
at org.netbeans.core.execution.RunClassThread.doRun(RunClassThread.java:132)
at org.netbeans.modules.openide.util.GlobalLookup.execute(GlobalLookup.java:45)
at org.openide.util.lookup.Lookups.executeWith(Lookups.java:287)
[catch] at org.netbeans.core.execution.RunClassThread.run(RunClassThread.java:81)
```
fixes:
- block on `CountDownLatch` instead of delegate
- create wrapper right when the task starts
- the other commit fixes an issue where a `start()` is called from within the thread's constructor
--
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: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
Re: [PR] Fixed concurrency issues in GradleDaemonExecutor [netbeans]
Posted by "mbien (via GitHub)" <gi...@apache.org>.
mbien commented on code in PR #7030:
URL: https://github.com/apache/netbeans/pull/7030#discussion_r1484947404
##########
extide/gradle/src/org/netbeans/modules/gradle/execute/GradleDaemonExecutor.java:
##########
@@ -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());
Review Comment:
> I've found paths in GradleDaemonExecutor::run - the exception handlers, that do not call gradleTask.finish at all, so these all will reach this lambda and produce result 0 ?
@sdedic this was the case before too, no?
```
if (result != null) {
return result;
}
return this.delegate.result();
```
this would have blocked on the delegate task and return the result. I am not 100% sure what you mean by 0 since the current behavior should do the same.
--
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: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
Re: [PR] Fixed concurrency issues in GradleDaemonExecutor [netbeans]
Posted by "mbien (via GitHub)" <gi...@apache.org>.
mbien commented on code in PR #7030:
URL: https://github.com/apache/netbeans/pull/7030#discussion_r1484947404
##########
extide/gradle/src/org/netbeans/modules/gradle/execute/GradleDaemonExecutor.java:
##########
@@ -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());
Review Comment:
> I've found paths in GradleDaemonExecutor::run - the exception handlers, that do not call gradleTask.finish at all, so these all will reach this lambda and produce result 0 ?
@sdedic this was the case before too, no?
```
if (result != null) {
return result;
}
return this.delegate.result();
```
this would have blocked on the delegate task and return the result. I am not 100% sure what you mean by 0.
--
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: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
Re: [PR] Fixed concurrency issues in GradleDaemonExecutor [netbeans]
Posted by "mbien (via GitHub)" <gi...@apache.org>.
mbien merged PR #7030:
URL: https://github.com/apache/netbeans/pull/7030
--
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: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
Re: [PR] Fixed concurrency issues in GradleDaemonExecutor [netbeans]
Posted by "mbien (via GitHub)" <gi...@apache.org>.
mbien commented on code in PR #7030:
URL: https://github.com/apache/netbeans/pull/7030#discussion_r1480318120
##########
extide/gradle/src/org/netbeans/modules/gradle/execute/GradleDaemonExecutor.java:
##########
@@ -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());
Review Comment:
not sure if I understand the question correctly. If someone calls `result()` and nothing calls the fast-path `finish(int)` the delegate task will count down the latch once it completes and the blocking `result()` method will now unblock and return `delegate.result()`.
the second case is that `finish(int)` is called before the delegate task completes, this would also unblock all waiting threads which wait on the latch, but this time it returns the `result` field without waiting for the delegate task.
the second case seems to be the intended purpose of the task wrapper to speed up the time-to-result.
--
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: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
Re: [PR] Fixed concurrency issues in GradleDaemonExecutor [netbeans]
Posted by "mbien (via GitHub)" <gi...@apache.org>.
mbien commented on PR #7030:
URL: https://github.com/apache/netbeans/pull/7030#issuecomment-1967406791
planning to merge this soon unless there are objections
--
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: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
Re: [PR] Fixed concurrency issues in GradleDaemonExecutor [netbeans]
Posted by "mbien (via GitHub)" <gi...@apache.org>.
mbien commented on PR #7030:
URL: https://github.com/apache/netbeans/pull/7030#issuecomment-1927874670
all green, removing all-tests label
--
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: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
Re: [PR] Fixed concurrency issues in GradleDaemonExecutor [netbeans]
Posted by "sdedic (via GitHub)" <gi...@apache.org>.
sdedic commented on code in PR #7030:
URL: https://github.com/apache/netbeans/pull/7030#discussion_r1479368186
##########
extide/gradle/src/org/netbeans/modules/gradle/execute/GradleDaemonExecutor.java:
##########
@@ -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());
Review Comment:
In this case, if called before finish, the `result` will appear to be 0 == success. Is that OK ? When searching for `finish` calls, I've found paths in `GradleDaemonExecutor::run` - the exception handlers, that do not call gradleTask.finish at all, so these all will reach this lambda and produce result 0 ?
--
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: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
Re: [PR] Fixed concurrency issues in GradleDaemonExecutor [netbeans]
Posted by "mbien (via GitHub)" <gi...@apache.org>.
mbien commented on code in PR #7030:
URL: https://github.com/apache/netbeans/pull/7030#discussion_r1480318120
##########
extide/gradle/src/org/netbeans/modules/gradle/execute/GradleDaemonExecutor.java:
##########
@@ -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());
Review Comment:
not sure if I understand the question correctly. If someone calls `result()` and nothing calls the fast-path `finish(int)` the delegate task will count down the latch once it completes and the blocking `result()` method will now unblock and return `delegate.result()`.
the second case is that `finish(int)` is called before the delegate task completes, this would also unblock all waiting threads which wait on the latch, but this time it returns the `result` field without waiting for the delegate task.
the second case seems to be the intended purpose of the task wrapper to speed up the time-to-result. Git blame msg luckily mentions it too https://github.com/apache/netbeans/commit/d7d4faf678c5817aadf6b04dd6d8673d7dadcf1b I wished this would be mentioned in code.
--
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: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists