CommandService.run(...).get() crashes when used in UI-Thread

[maarzt]

Many new ImageJ2 / FIJI developers run into this bug. So we should fix it soon:

CommandService.run(...).get() blocks, when used in the EDT (UI-Thread):

final Future< CommandModule > future = command.run( AnyCommand.class, false );
Object result = future.get().getOutput("result"); // blocks forever

[maarzt]

I invested the cause of this problem:

1. CommendService.run(...) tries to execute the Command as task in another thread. Lets name this thread "thread B".
2. Before the Command is executed, an event is publish by calling EventService.publishNow(new ModuleStartEvent(...)) from "thread B".
3. EventService.publish(...) calls EventQueue.invokeAndWait(...) to publish the event in the event dispatch thread.
4. But the event dispatch thread can not publish the event. Because the event dispatch thread, by executing future.get() waits for "thread B" and "thread B" by executing EventQueue.invokeAndWait() waits for the event dispatch thread.

Long story short (It's a deadlock between two threads):

• The event dispatch thread, waits for the thread that's supposed to execute the command. (future.get())
• Thread that is supposed to execute the command, waits for the event dispatch thread. (By calling EventService.publishNow(new ModuleStartEvent(...)).

[ctrueden]

Thanks for investigating. However, I do not have a good idea how to fix it. We have explored other ways to deliver events in the past, but anything other than forcing all events through the EDT caused subtle and nasty threading-related bugs to occur.

[maarzt]

I have two solution ideas:

1. The events ModuleStartEvent, ModuleCanceledEvent etc. seem to only be used in the LegacyService, so we can change publishNow to publishLater and see if the LegacyService still works properly.
2. Introduce a method CommandService.runAndWait(...) which doesn't start an extra thread.

The EventService behaves strangly, 'publishNow' uses EDT, 'publishLater` uses any other thread. This should be fixed too.

[ctrueden]

The EventService behaves strangly, 'publishNow' uses EDT, 'publishLater` uses any other thread. This should be fixed too.

I agree that it is unintuitive and wrong for publishLater not to use the EDT. It originally did so, until 6b878da41160da695f6552ee86f537cff2443faf. Actually, I thought I had reverted 6b878da41160da695f6552ee86f537cff2443faf a while back, since this "performance optimization" introduces other bugs, but apparently not.

Of your ideas above, (1) is a very deep change, which I am quite reluctant to employ. (2) might work, but I think it's bad programming to execute a command and then wait for it on the EDT. You are not supposed to block operations like that on the EDT, because it freezes the GUI. Maybe we should instead throw an exception if someone does that, with advice on what should be done instead?

[maarzt]

(2) might work, but I think it's bad programming to execute a command and then wait for it on the EDT. You are not supposed to block operations like that on the EDT, because it freezes the GUI.

You imply the Commands are blocking operations. But that's not true. Many Commands run quickly, executing them on the EDT is fine, and we need to make this possible. We otherwise force people to employ multi-threading where it's unnecessary. And this will result in bad code.

[ctrueden]

@maarzt Fair enough.

I didn't test it yet, but maybe a patch like the following would work to inform developers about the deadlock when calling get() on a Future from the EDT in this scenario?

Fail when calling Future.get() from the EDT

diff --git src/main/java/org/scijava/module/DefaultModuleService.java src/main/java/org/scijava/module/DefaultModuleService.java
index 4cd4da23..11b054a8 100644
--- src/main/java/org/scijava/module/DefaultModuleService.java
+++ src/main/java/org/scijava/module/DefaultModuleService.java
@@ -42,6 +42,8 @@ import java.util.Set;
 import java.util.concurrent.Callable;
 import java.util.concurrent.ExecutionException;
 import java.util.concurrent.Future;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.TimeoutException;
 
 import org.scijava.MenuPath;
 import org.scijava.Priority;
@@ -245,7 +247,43 @@ public class DefaultModuleService extends AbstractService implements
 		@SuppressWarnings("unchecked")
 		final Callable<M> callable = (Callable<M>) runner;
 		final Future<M> future = threadService.run(callable);
-		return future;
+		return new Future<M>() {
+
+			@Override
+			public boolean cancel(boolean mayInterruptIfRunning) {
+				return future.cancel(mayInterruptIfRunning);
+			}
+
+			@Override
+			public boolean isCancelled() {
+				return future.isCancelled();
+			}
+
+			@Override
+			public boolean isDone() {
+				return future.isDone();
+			}
+
+			@Override
+			public M get() throws InterruptedException, ExecutionException {
+				failIfDispatchThread();
+				return future.get();
+			}
+
+			@Override
+			public M get(long timeout, TimeUnit unit) throws InterruptedException,
+				ExecutionException, TimeoutException
+			{
+				failIfDispatchThread();
+				return future.get(timeout, unit);
+			}
+
+			private void failIfDispatchThread() {
+				if (!threadService.isDispatchThread()) return;
+				throw new RuntimeException(
+					"Please use runAndWait(...) instead of run(...).get() to avoid EDT deadlock");
+			}
+		};
 	}
 
 	@Override

And then together with a new runAndWait method of ModuleService (and CommandService and ScriptService), we should have a solution?

[maarzt]

Yes, I agree, that would solve it. :star2:

[NicoKiaru]

Ahah, I'm glad I found this issue. This deadlock caused me a lot of issues, and I tried too many failing things. Having a runAndWait method would be really great! Just to be sure : so far there's no way to 'emulate' this runAndWait method, right ?

[imagejan]

Interesting. In what circumstances is CommandService.run() called from the EDT? I've never seen this in my commands, and just wondered why the issue doesn't occur more often then. If you just call CommandService.run() within another command, it works fine, right?

[NicoKiaru]

I think it's called from the EDT when I'm running a Command from a Swing Component. For instance in bigdataviewer playground I generate a PopupMenu on right click with some actions that the user can trigger. Then the action is calling commandservice.run(...) but I can't put commandservice.run(...).get() because it stalls everything. I have such an issue here for instance : https://github.com/bigdataviewer/bigdataviewer-playground/blob/e371fcc4e47ec5d7ed05938b2948a735e2ada258/src/main/java/sc/fiji/bdvpg/scijava/services/SourceAndConverterService.java#L465

[NicoKiaru]

But maybe I don't need the get() in this circumstance ? Run will just process the command fully if it's run from the EDT ?

[imagejan]

If you don't need to programmatically access the outputs, but just trigger running the command, you won't need get(), right.