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

[Alexey Ivanov]

On Fri, 3 Feb 2023 14:13:34 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:
> 
>   Spec wording modified

The changes to the spec look go to me.

Since a CSR is required, I suggest updating the spec for `isDone` method so that it returns `true` only after `doInBackground` exits. Now `isDone` returns `true` after `cancel` completes but the `DONE` state isn't reached yet.

I think this change will make the implementation more consistent is easier to use.

As for the test, it does not ensure the `STARTED` state is notified before `doInBackground` started running, nor does it ensure `DONE` state is notified before `done` method is called.

Isn't it what we want to test?

If I run the test with jdk19 and comment out the `throw` statement in `done`, the test passes. Yet I expect it to fail since the order of notification about `DONE` state and `done` call is reversed in the fix.

Or do you both think it's too much for the test?

test/jdk/javax/swing/SwingWorker/TestDoneBeforeDoInBackground.java line 35:

> 33: import java.util.concurrent.CountDownLatch;
> 34: import java.util.concurrent.TimeUnit;
> 35: import java.util.concurrent.atomic.AtomicBoolean;

`AtomicBoolean` isn't currently used.

test/jdk/javax/swing/SwingWorker/TestDoneBeforeDoInBackground.java line 39:

> 37: public class TestDoneBeforeDoInBackground {
> 38: 
> 39:     private static boolean doInBackground = false;

Suggestion:

    private static volatile boolean doInBackground = false;

It still needs to be `volatile`, it's consistently accessed from three different threads.

test/jdk/javax/swing/SwingWorker/TestDoneBeforeDoInBackground.java line 70:

> 68:             protected void done() {
> 69:                 if (!doInBackground) {
> 70:                     throw new RuntimeException("done called before doInBackground");

Suggestion:

                    throw new RuntimeException("done called before doInBackground finished");

Otherwise, it looks very confusing.

test/jdk/javax/swing/SwingWorker/TestDoneBeforeDoInBackground.java line 92:

> 90:                     }
> 91:                     // After the doInBackground method is finished
> 92:                     // SwingWorker} notifies PropertyChangeListeners

Suggestion:

                    // SwingWorker notifies PropertyChangeListeners

test/jdk/javax/swing/SwingWorker/TestDoneBeforeDoInBackground.java line 97:

> 95:                             worker.getState() != SwingWorker.StateValue.DONE) {
> 96:                         throw new RuntimeException(
> 97:                             "listener called after doInBackground is finised" +

Suggestion:

                            "PropertyChangeListeners called after doInBackground is finished" +

-------------

Changes requested by aivanov (Reviewer).

PR: https://git.openjdk.org/jdk/pull/11940