On Fri, 13 Jan 2023 04:03:55 GMT, Prasanta Sadhukhan <[email protected]> wrote:
>> SwingWorker done() method [spec >> ](https://github.com/openjdk/jdk/blob/master/src/java.desktop/share/classes/javax/swing/SwingWorker.java#L452) >> says "Executed on the Event Dispatch Thread after the doInBackground method >> is finished" >> but there's no mechanism in place to honor that claim. >> The >> [spec](https://github.com/openjdk/jdk/blob/master/src/java.desktop/share/classes/javax/swing/SwingWorker.java#L289) >> also says the state should be DONE after doInBackground() returns which is >> also not done. >> >> Modified the code to honour the specification. > > Prasanta Sadhukhan has updated the pull request incrementally with one > additional commit since the last revision: > > Remove blocking thread sleep for EDT Blocking EDT with `cancel` is unacceptable. I propose the following fix ([**diff to your PR**](https://github.com/openjdk/jdk/compare/pr/11940...aivanov-jdk:prasanta/8081474-SwingWorker-done)): diff --git a/src/java.desktop/share/classes/javax/swing/SwingWorker.java b/src/java.desktop/share/classes/javax/swing/SwingWorker.java index 6acca76d859..9da72fe2f0a 100644 --- a/src/java.desktop/share/classes/javax/swing/SwingWorker.java +++ b/src/java.desktop/share/classes/javax/swing/SwingWorker.java @@ -301,18 +301,16 @@ public abstract class SwingWorker<T, V> implements RunnableFuture<T> { new Callable<T>() { public T call() throws Exception { setState(StateValue.STARTED); - T ret = doInBackground(); - setState(StateValue.DONE); - return ret; + try { + return doInBackground(); + } finally { + setState(StateValue.DONE); + doneEDT(); + } } }; - future = new FutureTask<T>(callable) { - @Override - protected void done() { - doneEDT(); - } - }; + future = new FutureTask<T>(callable); state = StateValue.PENDING; propertyChangeSupport = new SwingWorkerPropertyChangeSupport(this); doProcess = null; @@ -566,7 +564,7 @@ public abstract class SwingWorker<T, V> implements RunnableFuture<T> { * {@inheritDoc} */ public final boolean isDone() { - return future.isDone(); + return future.isDone() && state == StateValue.DONE; } /** @@ -753,13 +751,6 @@ public abstract class SwingWorker<T, V> implements RunnableFuture<T> { if (SwingUtilities.isEventDispatchThread()) { doDone.run(); } else { - if (state != StateValue.DONE) { - do { - try { - Thread.sleep(100); - } catch (InterruptedException e) {} - } while (state != StateValue.DONE); - } doSubmit.add(doDone); } } or diff to master: diff --git a/src/java.desktop/share/classes/javax/swing/SwingWorker.java b/src/java.desktop/share/classes/javax/swing/SwingWorker.java index 0d86075be3f..9da72fe2f0a 100644 --- a/src/java.desktop/share/classes/javax/swing/SwingWorker.java +++ b/src/java.desktop/share/classes/javax/swing/SwingWorker.java @@ -301,18 +301,16 @@ public abstract class SwingWorker<T, V> implements RunnableFuture<T> { new Callable<T>() { public T call() throws Exception { setState(StateValue.STARTED); - return doInBackground(); + try { + return doInBackground(); + } finally { + setState(StateValue.DONE); + doneEDT(); + } } }; - future = new FutureTask<T>(callable) { - @Override - protected void done() { - doneEDT(); - setState(StateValue.DONE); - } - }; - + future = new FutureTask<T>(callable); state = StateValue.PENDING; propertyChangeSupport = new SwingWorkerPropertyChangeSupport(this); doProcess = null; @@ -566,7 +564,7 @@ public abstract class SwingWorker<T, V> implements RunnableFuture<T> { * {@inheritDoc} */ public final boolean isDone() { - return future.isDone(); + return future.isDone() && state == StateValue.DONE; } /** This approach ensures `cancel` completes quickly and `done` is called only after `doInBackground` exits. I also suggest adding a condition to the test which verifies `cancel` doesn't take too long as well as ensuring `done` was called before exiting from `main`. The `isDone` method has the same problem: it returns `true` before the background task has completed, that's why I modified it so that it returns `true` only when the state is `DONE` too. I believe the time of `sleep` to simulate work can be reduced for automatic testing, however, I agree a few seconds' delay is better for debugging. That's why the time could be configurable in constants. src/java.desktop/share/classes/javax/swing/SwingWorker.java line 305: > 303: setState(StateValue.STARTED); > 304: T ret = doInBackground(); > 305: setState(StateValue.DONE); What if doInBackground thrown an exception? `DONE` state will never be reached. src/java.desktop/share/classes/javax/swing/SwingWorker.java line 762: > 760: } catch (InterruptedException e) {} > 761: } while (state != StateValue.DONE); > 762: } In the previous iteration, using `sleep` for waiting was *the concern*, you're still using `sleep`. This is not going to work because it makes `cancel` wait until `DONE` state is reached, which is not what one would expect, especially taking into account that `cancel` is likely called from EDT to cancel the background operation and blocking EDT is not acceptable. test/jdk/javax/swing/SwingWorker/TestSwingWorker.java line 32: > 30: import javax.swing.SwingWorker; > 31: > 32: public class TestSwingWorker { I believe it was decided to give test classes a meaningful names but `TestSwingWorker` isn't better than `bug8081474`. The latter has an advantage though, it's unique. `DoneBeforeDoInBackground` seems a better name which describes the test purpose, don't you think? test/jdk/javax/swing/SwingWorker/TestSwingWorker.java line 47: > 45: } > 46: } > 47: catch (InterruptedException ex) { Suggestion: } catch (InterruptedException ex) { `catch` should be on the same line as the closing `brace` of the previous block. ------------- Changes requested by aivanov (Reviewer). PR: https://git.openjdk.org/jdk/pull/11940
