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