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

[Alexey Ivanov]

On Fri, 10 Feb 2023 11:46:22 GMT, Prasanta Sadhukhan <psadhukhan at 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 two additional commits since the last revision:
> 
>  - Fix and test updated
>  - Fix and test updated

The changes look good except for the minor comments.

The updated fix doesn't require the CSR.

Please update the copyright year.

src/java.desktop/share/classes/javax/swing/SwingWorker.java line 313:

> 311:                 };
> 312: 
> 313:         future = new FutureTask<T>(callable);

Suggestion:

       future = new FutureTask<T>(callable);

An extra space breaks the correct indentation.

test/jdk/javax/swing/SwingWorker/TestDoneBeforeDoInBackground.java line 43:

> 41:     private static final int WAIT_TIME = 200;
> 42:     private static final long CLEANUP_TIME = 1000;
> 43:     private static final CountDownLatch doneLatch = new CountDownLatch(1);

Suggestion:

    private static final int WAIT_TIME = 200;
    private static final long CLEANUP_TIME = 1000;

    private static final AtomicBoolean doInBackgroundStarted = new AtomicBoolean(false);
    private static final AtomicBoolean doInBackgroundFinished = new AtomicBoolean(false);
    private static final CountDownLatch doneLatch = new CountDownLatch(1);


Separate the real constants from the objects which track the state. The flags should also be marked `final`.

test/jdk/javax/swing/SwingWorker/TestDoneBeforeDoInBackground.java line 57:

> 55:                     System.out.println("Got interrupted!");
> 56:                 }
> 57:                 try {

Maybe add a blank line between the two try-catch blocks?

And before `return`-statement?

test/jdk/javax/swing/SwingWorker/TestDoneBeforeDoInBackground.java line 73:

> 71:                 if (!doInBackgroundFinished.get()) {
> 72:                     throw new RuntimeException("done called before " +
> 73:                                              " doInBackground is finished");

Suggestion:

                    throw new RuntimeException("done called before " +
                                               "doInBackground is finished");


Add two spaces before the quote and to align it to the one on the line above; remove one space after the quote so that the error message doesn't have two consecutive spaces.

test/jdk/javax/swing/SwingWorker/TestDoneBeforeDoInBackground.java line 99:

> 97:                                "PropertyChangeListeners called with " +
> 98:                                "state STARTED before doInBackground is finished");
> 99:                     }

I think one condition is enough. I propose moving the state to be the first condition.


                    if (worker.getState() == SwingWorker.StateValue.STARTED
                        && (doInBackgroundStarted.get()
                            || doInBackgroundFinished.get())) {

We start with *the state* and ensure both `doInBackgroundStarted` and `doInBackgroundFinished` are `false`.

Yes, the error message can't be very specific in this case.

---

Java Code Style suggests binary operators should be wrapped to the next line rather than left on the previous line. I do prefer this style because it highlights *it is a continuation line* rather than a new statement.

I know it isn't followed consistently though. Nor do we have an updated code style guide in the clientlibs that we adhere when writing new code.

test/jdk/javax/swing/SwingWorker/TestDoneBeforeDoInBackground.java line 100:

> 98:                                "state STARTED before doInBackground is finished");
> 99:                     }
> 100:                     // After the doInBackground method is finished

Adding a blank line between the conditions would visually separate them.

test/jdk/javax/swing/SwingWorker/TestDoneBeforeDoInBackground.java line 104:

> 102:                     // property change to StateValue.DONE
> 103:                     if (doInBackgroundFinished.get() &&
> 104:                             worker.getState() != SwingWorker.StateValue.DONE) {

The condition here should take both flags into account. Again, I propose moving the state condition to be the first one.


                    if (worker.getState() != SwingWorker.StateValue.DONE
                        && doInBackgroundFinished.get()) {

We may also check that `doInBackgroundStarted` is `true`.

test/jdk/javax/swing/SwingWorker/TestDoneBeforeDoInBackground.java line 128:

> 126:                            " getState " + worker.getState());
> 127:         if (doInBackgroundFinished.get() &&
> 128:               worker.getState() != SwingWorker.StateValue.DONE) {

Suggestion:

        if (doInBackgroundFinished.get() &&
                worker.getState() != SwingWorker.StateValue.DONE) {

If you follow the style of double indentation on continuation lines, two more spaces should be added.

Consider moving the `&&` operator to the continuation line.

test/jdk/javax/swing/SwingWorker/TestDoneBeforeDoInBackground.java line 130:

> 128:               worker.getState() != SwingWorker.StateValue.DONE) {
> 129:             throw new RuntimeException("doInBackground is finished " +
> 130:                                        " but State is not DONE");

Suggestion:

                                       "but the State is not DONE");

Two consecutive spaces in an error message are redundant, one is enough.

The article seems missing?

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

Changes requested by aivanov (Reviewer).

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