fskorgen opened a new issue, #8227:
URL: https://github.com/apache/hop/issues/8227
### 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:
```java
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:
```java
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:
```java
wCancel.addListener(SWT.Selection, e -> { isCancelled = true; ...
display.wake(); });
```
```java
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()`:
```java
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()`:
```java
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
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
