RFR: 8081474: SwingWorker calls 'done' before the 'doInBackground' is finished [v3]

[Alexey Ivanov]

On Fri, 13 Jan 2023 04:03:55 GMT, Prasanta Sadhukhan <psadhukhan at openjdk.org> 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