javanna commented on code in PR #12689:
URL: https://github.com/apache/lucene/pull/12689#discussion_r1362620375
##########
lucene/core/src/java/org/apache/lucene/search/TaskExecutor.java:
##########
@@ -64,64 +67,124 @@ public final class TaskExecutor {
* @param <T> the return type of the task execution
*/
public <T> List<T> invokeAll(Collection<Callable<T>> callables) throws
IOException {
- List<Task<T>> tasks = new ArrayList<>(callables.size());
- boolean runOnCallerThread = numberOfRunningTasksInCurrentThread.get() > 0;
- for (Callable<T> callable : callables) {
- Task<T> task = new Task<>(callable);
- tasks.add(task);
- if (runOnCallerThread) {
- task.run();
- } else {
- executor.execute(task);
+ TaskGroup<T> taskGroup = new TaskGroup<>(callables);
+ return taskGroup.invokeAll(executor);
+ }
+
+ /**
+ * Holds all the sub-tasks that a certain operation gets split into as it
gets parallelized and
+ * exposes the ability to invoke such tasks and wait for them all to
complete their execution and
+ * provide their results. Ensures that each task does not get parallelized
further: this is
+ * important to avoid a deadlock in situations where one executor thread
waits on other executor
+ * threads to complete before it can progress. This happens in situations
where for instance
+ * {@link Query#createWeight(IndexSearcher, ScoreMode, float)} is called as
part of searching each
+ * slice, like {@link TopFieldCollector#populateScores(ScoreDoc[],
IndexSearcher, Query)} does.
+ * Additionally, if one task throws an exception, all other tasks from the
same group are
+ * cancelled, to avoid needless computation as their results would not be
exposed anyways. Creates
+ * one {@link FutureTask} for each {@link Callable} provided
+ *
+ * @param <T> the return type of all the callables
+ */
+ private static final class TaskGroup<T> {
Review Comment:
the diff is hard to read. The task is the same as before, with the addition
of some FutureTask methods override:
- setException to handle the exception and cancel all tasks on exception. we
can't simply catch the exception on run, because FutureTask#run does not throw
it. We could have wrapped the callable, but I prefer leaving the original
callable unchanged and overrideing FutureTask behavior instead
- cancel to deal with task cancellations: the original behaviour would be
for future.get to throw a cancellation exception when called on a cancelled
task, while still leaving the task running. We instead want to wait for all
tasks to be completed before returning.
I introduced the private TaskGroup abstraction to group all tasks and
primarily to make the `cancelAll` method available against a final list. This
is to address the dependency between the creation of the FutureTask, which
needs to cancel all tasks on exception, yet the tasks list is populated only
once all tasks have been created.
--
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: issues-unsubscr...@lucene.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org
For additional commands, e-mail: issues-h...@lucene.apache.org
