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

[Prasanta Sadhukhan]

On Tue, 31 Jan 2023 09:26:26 GMT, Alexey Ivanov <aivanov at openjdk.org> wrote:

>> Prasanta Sadhukhan has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Set DONE state for cancel
>
> 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`.

OK..I have removed unconditional setting of DONE state..
But then as I mentioned, it would cause JCK failure, so I have reverted isDone() change too...

> 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.

It will cause JCK issue..

> 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.

done

> 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.

Not sure I understand this...please elaborate...

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

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