On Tue, 31 Jan 2023 08:54:39 GMT, Prasanta Sadhukhan <psadhuk...@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

Reply via email to