GP-1505 - Task Dialog blocking fix

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

@@ -18,8 +18,7 @@ package ghidra.util.task;
 import java.beans.PropertyChangeEvent;
 import java.beans.PropertyChangeListener;
 import java.util.List;
-import java.util.concurrent.CopyOnWriteArrayList;
+import java.util.concurrent.*;
-import java.util.concurrent.ExecutionException;
 
 import javax.swing.SwingUtilities;
 import javax.swing.SwingWorker;
@@ -62,7 +61,7 @@ public abstract class CachingSwingWorker<T> implements CachingLoader<T> {
 	private int taskDialogDelay = 500;
 	private boolean hasProgress = true;
 	private T cachedValue;
-	private SwingWorker<T, Object> worker;
+	private SwingWorkerImpl worker;
 	private WorkerTaskMonitor taskMonitor = new WorkerTaskMonitor();
 
 	/**
@@ -86,7 +85,7 @@ public abstract class CachingSwingWorker<T> implements CachingLoader<T> {
 	/**
 	 * Subclasses must implement this method to create the object being managed/cached.
 	 * @param monitor A task monitor that can be used to provide progress information to
-	 * a progress dialog is it is being shown.  Implementers should also check the monitor 
+	 * a progress dialog is it is being shown.  Implementers should also check the monitor
 	 * periodically to check for cancelled. If cancelled, this method should not throw a
 	 * cancelled exception but instead either return null or a partial result. (For example,
 	 * if the object is a list being generated by a search, then it might make sense to return
@@ -97,7 +96,7 @@ public abstract class CachingSwingWorker<T> implements CachingLoader<T> {
 
 	/**
 	 * Returns the object that this class is managing/caching.  It will return the object if it is
-	 * already created or it will block until the object can be created.  If called from the Swing 
+	 * already created or it will block until the object can be created.  If called from the Swing
 	 * thread, it will also launch a modal progress dialog while waiting for the object to be
 	 * created.
 	 * 
@@ -113,7 +112,7 @@ public abstract class CachingSwingWorker<T> implements CachingLoader<T> {
 		}
 
 		addMonitor(monitor);
-		SwingWorker<T, Object> swingWorker = getWorker();
+		SwingWorkerImpl swingWorker = getWorker();
 		if (SwingUtilities.isEventDispatchThread()) {
 			blockSwingWithProgressDialog(swingWorker);
 		}
@@ -175,7 +174,7 @@ public abstract class CachingSwingWorker<T> implements CachingLoader<T> {
 		}
 	}
 
-	private void blockSwingWithProgressDialog(final SwingWorker<T, Object> localWorker) {
+	private void blockSwingWithProgressDialog(SwingWorkerImpl localWorker) {
 		if (!localWorker.isDone()) {
 
 			TaskDialog dialog = new SwingWorkerTaskDialog(name, hasProgress, localWorker);
@@ -185,7 +184,7 @@ public abstract class CachingSwingWorker<T> implements CachingLoader<T> {
 		}
 	}
 
-	private synchronized SwingWorker<T, Object> getWorker() {
+	private synchronized SwingWorkerImpl getWorker() {
 		if (worker == null) {
 			worker = new SwingWorkerImpl();
 			worker.execute();
@@ -220,19 +219,19 @@ public abstract class CachingSwingWorker<T> implements CachingLoader<T> {
 
 //==================================================================================================
 // Inner Classes
-//==================================================================================================	
+//==================================================================================================
 
 	private class SwingWorkerTaskDialog extends TaskDialog {
 
-		private SwingWorker<T, Object> taskWorker;
+		private SwingWorkerImpl taskWorker;
 
-		SwingWorkerTaskDialog(String title, boolean showProgress, SwingWorker<T, Object> worker) {
+		SwingWorkerTaskDialog(String title, boolean showProgress, SwingWorkerImpl worker) {
-			super(title, true, true, showProgress);
+			super(title, true, true, showProgress, worker.getFinishedLatch());
 			this.taskWorker = worker;
 		}
 
 		@Override
-		// Overridden to allow us to check for the worker's completion.  If we don't do 
+		// Overridden to allow us to check for the worker's completion.  If we don't do
 		// this, then there is no way for the task dialog to know when the work is finished.
 		// The result of this is that the task dialog gets shown every time, regardless of
 		// whether the worker finished before the 'grace' period.
@@ -242,9 +241,20 @@ public abstract class CachingSwingWorker<T> implements CachingLoader<T> {
 	}
 
 	private class SwingWorkerImpl extends SwingWorker<T, Object> {
+
+		// We update this latch *in the background thread* to notify clients in other threads that
+		// the work has been finished.  This prevents the Swing thread from blocking while waiting
+		// for the callbacks that happen on the Swing thread, such as done().
+		private CountDownLatch finished = new CountDownLatch(1);
+
+		CountDownLatch getFinishedLatch() {
+			return finished;
+		}
+
 		@Override
 		protected T doInBackground() throws Exception {
 			T result = runInBackground(taskMonitor);
+			finished.countDown();
 			return result;
 		}
 
@@ -263,7 +273,7 @@ public abstract class CachingSwingWorker<T> implements CachingLoader<T> {
 	}
 
 	private class WorkerTaskMonitor extends TaskMonitorAdapter {
-		private List<TaskMonitor> monitors = new CopyOnWriteArrayList<TaskMonitor>();
+		private List<TaskMonitor> monitors = new CopyOnWriteArrayList<>();
 		private int min = 0;
 		private int max = 0;
 		private int progress = 0;

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

@@ -35,7 +35,9 @@ import ghidra.util.timer.GTimerMonitor;
 
 /**
  * Dialog that is displayed to show activity for a Task that is running outside of the
- * Swing Thread.
+ * Swing Thread.   This dialog uses a delay before showing in order to give the background task
+ * thread a chance to finish.  This prevents a flashing dialog for tasks that finish before the
+ * delay time period.
  *
  * <p>Implementation note:
  * if this class is constructed with a {@code hasProgress} value of {@code false},
@@ -47,6 +49,16 @@ import ghidra.util.timer.GTimerMonitor;
  *
  * <p>This dialog can be toggled between indeterminate mode and progress mode via calls to
  * {@link #setIndeterminate(boolean)}.
+ * 
+ * <p><b>API Usage Note: </b>If this dialog is used outside of the task API, then the client must
+ * be sure to call {@link #taskProcessed()}<b> from the background thread performing the work</b>.
+ * Otherwise, this dialog will always wait for the {@code delay} amount of time for the background
+ * thread to finish.  This happens since the default completed notification mechanism is performed
+ * on the Swing thread.   If a client has triggered blocking on the Swing thread, then the
+ * notification on the Swing thread must wait, causing the full delay to take place.   Calling
+ * {@link #taskProcessed()} from the background thread allows the dialog to get notified before the
+ * {@code delay} period has expired.  The blocking issue only exists with a non-0 {@code delay}
+ * value.
  */
 public class TaskDialog extends DialogComponentProvider implements TaskMonitor {
 
@@ -66,23 +78,23 @@ public class TaskDialog extends DialogComponentProvider implements TaskMonitor {
 	 *  -calls iternalCancel()
 	 *  -triggers taskProcessed()
 	 *  -calls closeDialog runnable
-	 *  
+	 * 
 	 * Cancel Button Pressed:
 	 * 	-(same as Dialog Close Button Pressed)
 	 * 
 	 * Task Monitor Stop Button Pressed:
 	 *  -triggers taskProcessed()
 	 *  -calls closeDialog runnable
-	 *  
+	 * 
 	 * Public API dispose() is Called:
 	 *  -calls iternalCancel()
 	 *  -triggers taskProcessed()
 	 *  -calls closeDialog runnable
-	 *  
+	 * 
 	 * Task Monitor Cancelled by API:
 	 *  -triggers taskProcessed()
 	 *  -calls closeDialog runnable
-	 *  
+	 * 
 	 * Task Finishes Normally:
 	 *  -triggers taskProcessed()
 	 *  -calls closeDialog runnable
@@ -100,7 +112,7 @@ public class TaskDialog extends DialogComponentProvider implements TaskMonitor {
 	};
 
 	private GTimerMonitor showTimer = GTimerMonitor.DUMMY;
-	private CountDownLatch finished = new CountDownLatch(1);
+	private CountDownLatch finished;
 	private boolean supportsProgress;
 
 	private JPanel mainPanel;
@@ -121,12 +133,11 @@ public class TaskDialog extends DialogComponentProvider implements TaskMonitor {
 	 * @param centerOnComp component to be centered over when shown, otherwise center over parent.
 	 * If both centerOnComp and parent are null, dialog will be centered on screen.
 	 * @param task the Task that this dialog will be associated with
-	 * @param finished overrides the latch used by this dialog to know when the task is finished
+	 * @param finished the finished latch used by the background thread to notify of completion
 	 */
 	TaskDialog(Component centerOnComp, Task task, CountDownLatch finished) {
 		this(centerOnComp, task.getTaskTitle(), task.isModal(), task.canCancel(),
-			task.hasProgress());
+			task.hasProgress(), finished);
-		this.finished = finished;
 	}
 
 	/**
@@ -147,7 +158,21 @@ public class TaskDialog extends DialogComponentProvider implements TaskMonitor {
 	 * @param hasProgress true if the dialog should show a progress bar
 	 */
 	public TaskDialog(String title, boolean canCancel, boolean isModal, boolean hasProgress) {
-		this(null, title, isModal, canCancel, hasProgress);
+		this(title, isModal, canCancel, hasProgress, new CountDownLatch(1));
+	}
+
+	/**
+	 * Constructor
+	 *
+	 * @param title title for the dialog
+	 * @param canCancel true if the task can be canceled
+	 * @param isModal true if the dialog should be modal
+	 * @param hasProgress true if the dialog should show a progress bar
+	 * @param finished the finished latch used by the background thread to notify of completion
+	 */
+	public TaskDialog(String title, boolean canCancel, boolean isModal, boolean hasProgress,
+			CountDownLatch finished) {
+		this(null, title, isModal, canCancel, hasProgress, finished);
 	}
 
 	/**
@@ -159,12 +184,14 @@ public class TaskDialog extends DialogComponentProvider implements TaskMonitor {
 	 * @param isModal true if the dialog should be modal
 	 * @param canCancel true if the task can be canceled
 	 * @param hasProgress true if the dialog should show a progress bar
+	 * @param finished the finished latch used by the background thread to notify of completion
 	 */
 	private TaskDialog(Component centerOnComp, String title, boolean isModal, boolean canCancel,
-			boolean hasProgress) {
+			boolean hasProgress, CountDownLatch finished) {
 		super(title, isModal, true, canCancel, true);
 		this.centerOnComponent = centerOnComp;
 		this.supportsProgress = hasProgress;
+		this.finished = finished;
 		setup(canCancel);
 	}
 
@@ -362,6 +389,7 @@ public class TaskDialog extends DialogComponentProvider implements TaskMonitor {
 	/**
 	 * Cancels the task and closes this dialog
 	 */
+	@Override
 	public void dispose() {
 		internalCancel();
 	}
@@ -409,7 +437,7 @@ public class TaskDialog extends DialogComponentProvider implements TaskMonitor {
 			setIndeterminate(false);
 		}
 
-		// Note: it is not clear why we only wish to show progress if the monitor is not 
+		// Note: it is not clear why we only wish to show progress if the monitor is not
 		// visible.  This seems wrong.   If someone knows, please update this code.
 		//if (!monitorComponent.isShowing()) {
 		installProgressMonitor();
@@ -431,7 +459,7 @@ public class TaskDialog extends DialogComponentProvider implements TaskMonitor {
 		supportsProgress = !indeterminate;
 		monitorComponent.setIndeterminate(indeterminate);
 
-		// Assumption: if the client calls this method to show progress, then we should honor 
+		// Assumption: if the client calls this method to show progress, then we should honor
 		// that request.  If we find that nested monitor usage causes dialogs to incorrectly
 		// toggle monitors, then we need to update those clients to use a wrapping style
 		// monitor that prevents the behavior.