On Fri, 3 Feb 2023 14:13:34 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: > > Spec wording modified The changes to the spec look go to me. Since a CSR is required, I suggest updating the spec for `isDone` method so that it returns `true` only after `doInBackground` exits. Now `isDone` returns `true` after `cancel` completes but the `DONE` state isn't reached yet. I think this change will make the implementation more consistent is easier to use. As for the test, it does not ensure the `STARTED` state is notified before `doInBackground` started running, nor does it ensure `DONE` state is notified before `done` method is called. Isn't it what we want to test? If I run the test with jdk19 and comment out the `throw` statement in `done`, the test passes. Yet I expect it to fail since the order of notification about `DONE` state and `done` call is reversed in the fix. Or do you both think it's too much for the test? test/jdk/javax/swing/SwingWorker/TestDoneBeforeDoInBackground.java line 35: > 33: import java.util.concurrent.CountDownLatch; > 34: import java.util.concurrent.TimeUnit; > 35: import java.util.concurrent.atomic.AtomicBoolean; `AtomicBoolean` isn't currently used. test/jdk/javax/swing/SwingWorker/TestDoneBeforeDoInBackground.java line 39: > 37: public class TestDoneBeforeDoInBackground { > 38: > 39: private static boolean doInBackground = false; Suggestion: private static volatile boolean doInBackground = false; It still needs to be `volatile`, it's consistently accessed from three different threads. test/jdk/javax/swing/SwingWorker/TestDoneBeforeDoInBackground.java line 70: > 68: protected void done() { > 69: if (!doInBackground) { > 70: throw new RuntimeException("done called before > doInBackground"); Suggestion: throw new RuntimeException("done called before doInBackground finished"); Otherwise, it looks very confusing. test/jdk/javax/swing/SwingWorker/TestDoneBeforeDoInBackground.java line 92: > 90: } > 91: // After the doInBackground method is finished > 92: // SwingWorker} notifies PropertyChangeListeners Suggestion: // SwingWorker notifies PropertyChangeListeners test/jdk/javax/swing/SwingWorker/TestDoneBeforeDoInBackground.java line 97: > 95: worker.getState() != > SwingWorker.StateValue.DONE) { > 96: throw new RuntimeException( > 97: "listener called after doInBackground is > finised" + Suggestion: "PropertyChangeListeners called after doInBackground is finished" + ------------- Changes requested by aivanov (Reviewer). PR: https://git.openjdk.org/jdk/pull/11940
