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

[Alexey Ivanov]

On Tue, 31 Jan 2023 08:54:39 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:
> 
>   Set DONE state for cancel

Changes requested by aivanov (Reviewer).

src/java.desktop/share/classes/javax/swing/SwingWorker.java line 554:

> 552:     public final boolean cancel(boolean mayInterruptIfRunning) {
> 553:         setState(StateValue.DONE);
> 554:         return future.cancel(mayInterruptIfRunning);

Setting the state to `DONE` unconditionally doesn't seem right either. What if `doInBackground` isn't started? What if `doInBackground` starts after you compare the state is `PENDING`?

If `cancel` changes the state, it must do it after `future.cancel` returns. But then, the state moves to `DONE` before `doInBackground` exited which contradicts the spec for `DONE`.

src/java.desktop/share/classes/javax/swing/SwingWorker.java line 568:

> 566:      */
> 567:     public final boolean isDone() {
> 568:         return future.isDone() && state == StateValue.DONE;

Suggestion:

        return state == StateValue.DONE;

I guess we can remove the call to `future.isDone()` altogether. Either both conditions are true, or both are false.

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

> 49:                 try {
> 50:                     System.out.println("Cleaning up");
> 51:                     Thread.sleep(5000);

I still think that you should define constants for *all the delays*, and using 1 or 2 seconds for cleaning should be enough.

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

> 70:         Thread.sleep(5000);
> 71: 
> 72:         worker.cancel(true);

I still suggest ensuring `cancel` does not take too long.

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

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