[Bug]: ProgressMonitorDialog never closes unless the runnable calls monitor.done()

[fskorgen]

Apache Hop version?

2.19

Java version?

21

Operating system

Windows

What happened?

ProgressMonitorDialog.run starts the work on a background thread and then pumps the event loop
until its shell is disposed:

private void pumpDisplayUntilShellDisposed()
    throws InvocationTargetException, InterruptedException {
  while (!shell.isDisposed()) {
    if (interruptedException != null) { dispose(); throw interruptedException; }
    if (targetException != null) { dispose(); throw targetException; }
    if (!display.readAndDispatch()) { display.sleep(); }
  }
}

Three things end that loop: the shell being disposed, an InterruptedException, or an
InvocationTargetException. Normal completion of the runnable is not one of them.
runMonitoredWork runs the runnable, records those two exception types and returns — it never
disposes the shell:

private void runMonitoredWork(IRunnableWithProgress runnable) {
  try {
    runnable.run(progressMonitor);
  } catch (InvocationTargetException e) {
    targetException = e;
  } catch (InterruptedException e) {
    interruptedException = e;
    Thread.currentThread().interrupt();
  }
}

The only path that disposes the shell on success is ProgressMonitor.done(), which the runnable
has to call. A runnable that finishes its work and returns cleanly leaves the dialog on screen
forever, with the event loop still being pumped.

Cancel cannot recover it

Neither can the user escape. The Cancel button and the shell's close handler only raise a flag:

wCancel.addListener(SWT.Selection, e -> { isCancelled = true; ... display.wake(); });

shell.addListener(SWT.Close, e -> { e.doit = false; isCancelled = true; ... });

isCancelled is what monitor.isCanceled() reports, so cancelling depends on the runnable
noticing the flag and then calling done(). For a runnable that never calls done() at all,
Cancel does nothing and SWT.Close is explicitly refused (e.doit = false). The only way out is
killing the process.

A caller that hits it today

GetQueryFieldsProgressDialog.open():

IRunnableWithProgress op =
    monitor -> {
      db = new Database(HopGui.getInstance().getLoggingObject(), variables, databaseMeta);
      try {
        db.connect();
        result = db.getQueryFields(sql, false);
        if (monitor.isCanceled()) { ... }
      } catch (Exception e) {
        throw new InvocationTargetException(e, ...);
      } finally {
        db.disconnect();          // no monitor.done()
      }
    };

On success the dialog never closes. On failure it does — via the InvocationTargetException path —
so the bug only shows when the query works, which is the common case and probably why it went
unnoticed.

The neighbouring dialogs get away with it by accident rather than design:

Caller	Why it closes
GetTableSizeProgressDialog	calls monitor.done() in its finally
GetDatabaseInfoProgressDialog	DatabaseMetaInformation.getData calls monitor.done() in its finally
GetPreviewTableProgressDialog	Database.getRows(ResultSet, limit, monitor) calls monitor.done()
GetQueryFieldsProgressDialog	nothing does — hangs

So three of four depend on someone further down the stack happening to call done(). That is a
fragile contract for a class with 15 callers in ui, 21 across the tree.

Steps to reproduce

1. Call GetQueryFieldsProgressDialog on a working connection with valid SQL — for example
   new GetQueryFieldsProgressDialog(shell, variables, databaseMeta, "SELECT * FROM <table>").open().
2. Let it succeed.

Expected: the dialog closes and open() returns the row metadata.
Actual: the progress dialog stays up indefinitely. Cancel has no effect, the window's close box
is refused, and HopGui has to be killed.

Suggested fix

Dispose when the runnable finishes, in runMonitoredWork, so the dialog's lifetime no longer
depends on every caller remembering done():

private void runMonitoredWork(IRunnableWithProgress runnable) {
  try {
    runnable.run(progressMonitor);
  } catch (InvocationTargetException e) {
    targetException = e;
  } catch (InterruptedException e) {
    interruptedException = e;
    Thread.currentThread().interrupt();
  } finally {
    dispose();
  }
}

dispose() is already idempotent (it returns early when the shell is null or disposed), so callers
that do call done() are unaffected. Adding the missing monitor.done() to
GetQueryFieldsProgressDialog alone would fix that one caller, but leaves the same trap for the
next one.

Cancel is worth a second look regardless: with the fix above a cancelled-but-unresponsive runnable
still holds the dialog until it returns, which is defensible, but the refused SWT.Close
(e.doit = false) then gives the user no feedback at all.

Issue Priority

Priority: 1

Issue Component

Component: Hop Gui