Task Launcher - fixed synchronization issues

Ghidra/Framework/Docking/src/main/java/ghidra/util/task/BasicTaskMonitor.java

@@ -28,8 +28,9 @@ import ghidra.util.exception.CancelledException;
  * We wish for this class to be performant.    Thus, we do not synchronize the methods of this
  * class, nor do we make the values thread visible via <code>volatile</code> or by any of 
  * the Java concurrent structures (e.g., {@link AtomicBoolean}).   In order to keep the values of
- * this class's fields update-to-date, we have chosen to synchronize the <code>getter</code> 
- * methods.   Thus, when the fields are read, the most recent version will be used.
+ * this class's fields update-to-date, we have chosen to synchronize the package-level client of
+ * this class.  <b>If this class is ever made public, then most of the methods herein need to 
+ * be synchronized to prevent race conditions and to provide visibility.
  */
 class BasicTaskMonitor implements TaskMonitor {
 
@@ -98,7 +99,7 @@ class BasicTaskMonitor implements TaskMonitor {
 	}
 
 	@Override
-	public synchronized void setMaximum(long max) {
+	public void setMaximum(long max) {
 		this.maxProgress = max;
 		if (progress > max) {
 			progress = max;
@@ -125,7 +126,7 @@ class BasicTaskMonitor implements TaskMonitor {
 		boolean wasCancelled = isCancelled;
 		isCancelled = true;
 		if (!wasCancelled) {
-			notifyChangeListeners();
+			notifyCancelledListeners();
 		}
 	}
 
@@ -149,7 +150,7 @@ class BasicTaskMonitor implements TaskMonitor {
 		// stub
 	}
 
-	private synchronized void notifyChangeListeners() {
+	private void notifyCancelledListeners() {
 		for (CancelledListener listener : listeners) {
 			listener.cancelled();
 		}

Ghidra/Framework/Docking/src/main/java/ghidra/util/task/TaskDialog.java

@@ -57,7 +57,7 @@ public class TaskDialog extends DialogComponentProvider implements TaskMonitor {
 	/** If not null, then the value of the string has yet to be rendered */
 	private AtomicReference<String> newMessage = new AtomicReference<>();
 	private SwingUpdateManager messageUpdater =
-		new SwingUpdateManager(() -> setStatusText(newMessage.getAndSet(null)));
+		new SwingUpdateManager(100, 250, () -> setStatusText(newMessage.getAndSet(null)));
 
 	/** 
 	 * Constructor

Ghidra/Framework/Docking/src/main/java/ghidra/util/task/TaskLauncher.java

@@ -28,7 +28,6 @@ import ghidra.util.Swing;
  * {@link #TaskLauncher(Task, Component, int, int)}.  Alternatively, for simpler uses,
  * see one of the many static convenience methods.
  * 
- * <a name="modal_usage"></a>
  * <p><b><a name="modal_usage">Modal Usage</a></b><br>
  * Most clients of this class should not be concerned with where 
  * the dialog used by this class will appear.  By default, it will be shown over 
@@ -237,7 +236,7 @@ public class TaskLauncher {
 	 * @throws IllegalStateException if the given thread is the Swing thread
 	 */
 	protected void runInThisBackgroundThread(Task task) {
-		if (Swing.isEventDispatchThread()) {
+		if (Swing.isSwingThread()) {
 			throw new IllegalStateException("Must not call this method from the Swing thread");
 		}
 

Ghidra/Framework/Docking/src/main/java/ghidra/util/task/TaskRunner.java

@@ -16,9 +16,7 @@
 package ghidra.util.task;
 
 import java.awt.Component;
-import java.util.concurrent.CountDownLatch;
-import java.util.concurrent.Executor;
-import java.util.concurrent.atomic.AtomicReference;
+import java.util.concurrent.*;
 
 import generic.concurrent.GThreadPool;
 import generic.util.WindowUtilities;
@@ -35,17 +33,7 @@ class TaskRunner {
 	private int delayMs;
 	private int dialogWidth;
 
-	private Thread taskThread;
-	private CancelledListener monitorChangeListener = () -> {
-		if (task.isInterruptible()) {
-			taskThread.interrupt();
-		}
-		if (task.isForgettable()) {
-			closeDialog(); // close the dialog and forget about the task
-		}
-	};
-
-	private AtomicReference<TaskDialog> taskDialog = new AtomicReference<>();
+	private TaskDialog taskDialog;
 	private CountDownLatch finished = new CountDownLatch(1);
 
 	TaskRunner(Task task, Component parent, int delayMs, int dialogWidth) {
@@ -67,17 +55,18 @@ class TaskRunner {
 	private void showDialogIfSwingOrShowLaterAndWait(WrappingTaskMonitor monitor) {
 
 		Swing.runIfSwingOrRunLater(() -> showTaskDialog(monitor));
-		waitForModalIfNotSwing();
+		waitForModalTask();
 	}
 
-	private void waitForModalIfNotSwing() {
-		if (Swing.isSwingThread() || !task.isModal()) {
-			// if this is the Swing thread, then the work is already done at this point; otherwise,
-			// the task is not modal, so do not block
-			return;
+	private void waitForModalTask() {
+
+		if (!task.isModal()) {
+			return; // we do not wait for non-modal tasks
 		}
 
 		try {
+			// fun note: if this is the Swing thread, then it will not wait, as the Swing thread
+			// was blocked by the modal dialog in the call before this one
 			finished.await();
 		}
 		catch (InterruptedException e) {
@@ -86,39 +75,14 @@ class TaskRunner {
 	}
 
 	// protected to allow for dependency injection
-	protected TaskDialog buildTaskDialog(Component comp) {
+	protected TaskDialog buildTaskDialog(Component comp, TaskMonitor monitor) {
 
-		//
-		// This class may be used by background threads.  Make sure that our GUI creation is
-		// on the Swing thread to prevent exceptions while painting (as seen when using the
-		// Nimbus Look and Feel).
-		//
 		TaskDialog dialog = createTaskDialog(comp);
 		dialog.setMinimumSize(dialogWidth, 0);
-
-		if (task.isInterruptible() || task.isForgettable()) {
-			dialog.addCancelledListener(monitorChangeListener);
-		}
-
 		dialog.setStatusJustification(task.getStatusTextAlignment());
-
 		return dialog;
 	}
 
-	private void showTaskDialog(WrappingTaskMonitor monitor) {
-
-		Swing.assertThisIsTheSwingThread("Must be on the Swing thread build the Task Dialog");
-
-		if (finished.getCount() == 0) {
-			return;
-		}
-
-		TaskDialog dialog = buildTaskDialog(parent);
-		taskDialog.set(dialog);
-		monitor.setDelegate(dialog); // initialize the dialog to the current state of the monitor
-		dialog.show(Math.max(delayMs, 0));
-	}
-
 	private void startBackgroundThread(TaskMonitor monitor) {
 
 		// add the task here, so we can track it before it is actually started by the thread
@@ -139,21 +103,6 @@ class TaskRunner {
 		});
 	}
 
-	private void taskFinished() {
-		finished.countDown();
-		TaskDialog dialog = taskDialog.get();
-		if (dialog != null) {
-			dialog.taskProcessed();
-		}
-	}
-
-	private void closeDialog() {
-		TaskDialog dialog = taskDialog.get();
-		if (dialog != null) {
-			dialog.close();
-		}
-	}
-
 	private TaskDialog createTaskDialog(Component comp) {
 		Component currentParent = comp;
 		if (currentParent != null) {
@@ -165,4 +114,36 @@ class TaskRunner {
 		}
 		return new TaskDialog(comp, task);
 	}
+
+	private void showTaskDialog(WrappingTaskMonitor monitor) {
+
+		Swing.assertThisIsTheSwingThread("Must be on the Swing thread build the Task Dialog");
+
+		taskDialog = buildTaskDialog(parent, monitor);
+		monitor.setDelegate(taskDialog); // initialize the dialog to the current state of the monitor
+		int delay = Math.max(delayMs, 0);
+		try {
+			if (finished.await(delay, TimeUnit.MILLISECONDS)) {
+				return;
+			}
+		}
+		catch (InterruptedException e) {
+			// not sure about this case; proceed to show the dialog
+		}
+
+		taskDialog.show(delay);
+	}
+
+	private void taskFinished() {
+
+		finished.countDown();
+
+		// Do this later on the Swing thread to handle the race condition where the dialog 
+		// did not exist at the time of this call, but was in the process of being created
+		Swing.runLater(() -> {
+			if (taskDialog != null) {
+				taskDialog.taskProcessed();
+			}
+		});
+	}
 }

Ghidra/Framework/Docking/src/test/java/ghidra/util/task/AbstractTaskTest.java

@@ -105,7 +105,7 @@ public class AbstractTaskTest extends AbstractDockingTest {
 
 			return new TaskRunner(task, parent, delay, dialogWidth) {
 				@Override
-				protected TaskDialog buildTaskDialog(Component comp) {
+				protected TaskDialog buildTaskDialog(Component comp, TaskMonitor monitor) {
 					dialogSpy = new TaskDialogSpy(task);
 					return dialogSpy;
 				}

Ghidra/Framework/Docking/src/test/java/ghidra/util/task/TaskDialogTest.java

@@ -111,5 +111,4 @@ public class TaskDialogTest extends AbstractTaskTest {
 
 		assertFalse(dialogSpy.isCancelEnabled());
 	}
-
 }

Ghidra/Framework/Generic/src/main/java/ghidra/util/task/Task.java

@@ -220,28 +220,6 @@ public abstract class Task implements MonitoredRunnable {
 		return isModal;
 	}
 
-	public boolean isInterruptible() {
-		return isInterruptible;
-	}
-
-	public void setInterruptible(boolean interruptible) {
-		this.isInterruptible = interruptible;
-	}
-
-	/**
-	 * Returns true if this task should be left alone to die when cancelled, as opposed to being
-	 * interrupted
-	 * 
-	 * @return true if forgettable
-	 */
-	public boolean isForgettable() {
-		return isForgettable;
-	}
-
-	public void setForgettable(boolean isForgettable) {
-		this.isForgettable = isForgettable;
-	}
-
 	/**
 	 * Sets the task listener on this task.  It is a programming error to call this method more
 	 * than once or to call this method if a listener was passed into the constructor of this class.

Ghidra/Framework/Utility/src/main/java/ghidra/util/Swing.java

@@ -61,7 +61,7 @@ public class Swing {
 	 *
 	 * @return  true if this is the event dispatch thread -OR- is in headless mode.
 	 */
-	public static boolean isEventDispatchThread() {
+	public static boolean isSwingThread() {
 		if (isInHeadlessMode()) {
 			return true;
 		}
@@ -70,14 +70,6 @@ public class Swing {
 		return SwingUtilities.isEventDispatchThread();
 	}
 
-	/**
-	 * A convenience method for {@link #isEventDispatchThread()}
-	 * @return true if this is the Swing thread
-	 */
-	public static boolean isSwingThread() {
-		return isEventDispatchThread();
-	}
-
 	/**
 	 * Wait until AWT event queue (Swing) has been flushed and no more (to a point) events
 	 * are pending.
@@ -102,7 +94,7 @@ public class Swing {
 			return; // squash during production mode
 		}
 
-		if (!isEventDispatchThread()) {
+		if (!isSwingThread()) {
 			Throwable t =
 				ReflectionUtilities.filterJavaThrowable(new AssertException(errorMessage));
 			Msg.error(SystemUtilities.class, errorMessage, t);

Ghidra/Framework/Utility/src/main/java/ghidra/util/SystemUtilities.java

@@ -422,7 +422,7 @@ public class SystemUtilities {
 	 * @return  true if this is the event dispatch thread -OR- is in headless mode.
 	 */
 	public static boolean isEventDispatchThread() {
-		return Swing.isEventDispatchThread();
+		return Swing.isSwingThread();
 	}
 
 	/**