On Tue, 31 Jan 2023 09:26:26 GMT, Alexey Ivanov <aiva...@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
