8081474: SwingWorker calls 'done' before the 'doInBackground' is finished by prsadhuk · Pull Request #11940 · openjdk/jdk

prsadhuk · 2023-01-11T11:01:18Z

SwingWorker done() method spec 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
also says the state should be DONE after doInBackground() returns which is also not done.

Modified the code to honour the specification.

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed (2 reviews required, with at least 1 Reviewer, 1 Author)

Issues

JDK-8081474: SwingWorker calls 'done' before the 'doInBackground' is finished
JDK-8301940: SwingWorker calls 'done' before the 'doInBackground' is finished (CSR) (Withdrawn)

Reviewers

Alexey Ivanov (@aivanov-jdk - Reviewer)
Sergey Bylokhov (@mrserb - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/11940/head:pull/11940
$ git checkout pull/11940

Update a local copy of the PR:
$ git checkout pull/11940
$ git pull https://git.openjdk.org/jdk pull/11940/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 11940

View PR using the GUI difftool:
$ git pr show -t 11940

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/11940.diff

…shed

bridgekeeper · 2023-01-11T11:02:38Z

👋 Welcome back psadhukhan! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2023-01-11T11:06:25Z

@prsadhuk The following label will be automatically applied to this pull request:

client

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

mlbridge · 2023-01-11T11:24:19Z

Webrevs

src/java.desktop/share/classes/javax/swing/SwingWorker.java

prsadhuk · 2023-01-27T05:05:37Z

Any more comments on this?
Should we close the JBS as "Not an issue"?
or
Should we just update the spec citing it's not necessary to wait for doInBackground() to finish before done() is called?
or
any other suggestion for the fix?

aivanov-jdk

Blocking EDT with cancel is unacceptable.

I propose the following fix (diff to your PR):

diff --git a/src/java.desktop/share/classes/javax/swing/SwingWorker.java b/src/java.desktop/share/classes/javax/swing/SwingWorker.java
index 6acca76d859..9da72fe2f0a 100644
--- a/src/java.desktop/share/classes/javax/swing/SwingWorker.java
+++ b/src/java.desktop/share/classes/javax/swing/SwingWorker.java
@@ -301,18 +301,16 @@ public abstract class SwingWorker<T, V> implements RunnableFuture<T> {
                 new Callable<T>() {
                     public T call() throws Exception {
                         setState(StateValue.STARTED);
-                        T ret = doInBackground();
-                        setState(StateValue.DONE);
-                        return ret;
+                        try {
+                            return doInBackground();
+                        } finally {
+                            setState(StateValue.DONE);
+                            doneEDT();
+                        }
                     }
                 };
 
-        future = new FutureTask<T>(callable) {
-                       @Override
-                       protected void done() {
-                           doneEDT();
-                       }
-                   };
+       future = new FutureTask<T>(callable);
        state = StateValue.PENDING;
        propertyChangeSupport = new SwingWorkerPropertyChangeSupport(this);
        doProcess = null;
@@ -566,7 +564,7 @@ public abstract class SwingWorker<T, V> implements RunnableFuture<T> {
      * {@inheritDoc}
      */
     public final boolean isDone() {
-        return future.isDone();
+        return future.isDone() && state == StateValue.DONE;
     }
 
     /**
@@ -753,13 +751,6 @@ public abstract class SwingWorker<T, V> implements RunnableFuture<T> {
         if (SwingUtilities.isEventDispatchThread()) {
             doDone.run();
         } else {
-            if (state != StateValue.DONE) {
-                do {
-                    try {
-                        Thread.sleep(100);
-                    } catch (InterruptedException e) {}
-                } while (state != StateValue.DONE);
-            }
             doSubmit.add(doDone);
         }
     }

or diff to master:

diff --git a/src/java.desktop/share/classes/javax/swing/SwingWorker.java b/src/java.desktop/share/classes/javax/swing/SwingWorker.java
index 0d86075be3f..9da72fe2f0a 100644
--- a/src/java.desktop/share/classes/javax/swing/SwingWorker.java
+++ b/src/java.desktop/share/classes/javax/swing/SwingWorker.java
@@ -301,18 +301,16 @@ public abstract class SwingWorker<T, V> implements RunnableFuture<T> {
                 new Callable<T>() {
                     public T call() throws Exception {
                         setState(StateValue.STARTED);
-                        return doInBackground();
+                        try {
+                            return doInBackground();
+                        } finally {
+                            setState(StateValue.DONE);
+                            doneEDT();
+                        }
                     }
                 };
 
-        future = new FutureTask<T>(callable) {
-                       @Override
-                       protected void done() {
-                           doneEDT();
-                           setState(StateValue.DONE);
-                       }
-                   };
-
+       future = new FutureTask<T>(callable);
        state = StateValue.PENDING;
        propertyChangeSupport = new SwingWorkerPropertyChangeSupport(this);
        doProcess = null;
@@ -566,7 +564,7 @@ public abstract class SwingWorker<T, V> implements RunnableFuture<T> {
      * {@inheritDoc}
      */
     public final boolean isDone() {
-        return future.isDone();
+        return future.isDone() && state == StateValue.DONE;
     }
 
     /**

This approach ensures cancel completes quickly and done is called only after doInBackground exits.

I also suggest adding a condition to the test which verifies cancel doesn't take too long as well as ensuring done was called before exiting from main.

The isDone method has the same problem: it returns true before the background task has completed, that's why I modified it so that it returns true only when the state is DONE too.

I believe the time of sleep to simulate work can be reduced for automatic testing, however, I agree a few seconds' delay is better for debugging. That's why the time could be configurable in constants.

aivanov-jdk · 2023-01-29T14:28:56Z

src/java.desktop/share/classes/javax/swing/SwingWorker.java

                        setState(StateValue.STARTED);
-                        return doInBackground();
+                        T ret = doInBackground();
+                        setState(StateValue.DONE);


What if doInBackground thrown an exception? DONE state will never be reached.

aivanov-jdk · 2023-01-29T14:32:11Z

src/java.desktop/share/classes/javax/swing/SwingWorker.java

+            if (state != StateValue.DONE) {
+                do {
+                    try {
+                        Thread.sleep(100);
+                    } catch (InterruptedException e) {}
+                } while (state != StateValue.DONE);
+            }


In the previous iteration, using sleep for waiting was the concern, you're still using sleep.

This is not going to work because it makes cancel wait until DONE state is reached, which is not what one would expect, especially taking into account that cancel is likely called from EDT to cancel the background operation and blocking EDT is not acceptable.

I removed sleep from EDT case and not blocking EDT and guess sleep is now being done for non-EDT case...
If it is to be called from EDT then it should go to "if" block and not to "else" which is what I based my fix on..

Anyway, I appreciate your fix and will see to it..

Yet having sleep is an indication of busy wait. You could've used CountDownLatch in conjunction with setState to avoid sleep at all.

I removed sleep from EDT case and not blocking EDT and guess sleep is now being done for non-EDT case...

Ah, right. But then if cancel is called on EDT, done is invoked before doInBackground completes, isn't it?

If cancel is called from another thread, the main thread as in the test, it doesn't return until doInBackground completes. With your test case and fix, cancel blocks for 5 seconds.

Either way, the behaviour does not look right.

It seems the isDone method modification is causing a JCK test to fail and there's no spec for it to challenge JCK test, although logically it seems right that STATE should be DONE but for now, I have modified the fix which satisfies regression test and JCK both..

isDone() spec mention "Completion may be due to normal termination, an exception, or cancellation -- in all of these cases, this method will return true"
so for cancel state also, the state needs to be in DONE state, in which case we can reinstate the DONE state check in isDone()

isDone() spec is inherited from Future.isDone() and it is true for how FutureTask is implemented.

jdk/src/java.desktop/share/classes/javax/swing/SwingWorker.java

Lines 565 to 570 in 810c8a2

/**

* {@inheritDoc}

*/

public final boolean isDone() {

return future.isDone();

}

Can't we change it?

Given what is said in the spec for SwingWorker before, I assume isDone should return true when the status of SwingWorker moves to DONE and it should occur when doInBackground returns as this is defined in the spec:

jdk/src/java.desktop/share/classes/javax/swing/SwingWorker.java

Lines 288 to 293 in 810c8a2

/**

* {@code SwingWorker} is {@code DONE}

* after {@code doInBackground} method

* is finished.

*/

DONE

test/jdk/javax/swing/SwingWorker/TestSwingWorker.java

aivanov-jdk · 2023-01-29T15:14:44Z

Should we just update the spec citing it's not necessary to wait for doInBackground() to finish before done() is called?

It is also an option.

All the suggested approaches set the state to DONE before done is called now. Yet the Workflow states the following: “After the doInBackground method is finished the done method is executed. Then SwingWorker notifies any PropertyChangeListeners about the state property change to StateValue.DONE.”

However, the last sentence may not have been guaranteed before either but it was likely satisfied as doneEDT was called before setState(StateValue.DONE). So, done had likely been executed before PropertyChangeListener was notified.

Hence, a small update to the spec may still be required whatever option we choose.

aivanov-jdk · 2023-01-31T09:26:26Z

src/java.desktop/share/classes/javax/swing/SwingWorker.java

+        setState(StateValue.DONE);
        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...

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...

Should we update the spec for isDone()? I think it's the right way because it makes the behaviour consistent. Doing so requires a CSR which will be the grounds for updating the JCK test.

I am not sure if it is entirely wrong..I will keep it as it is for now..I dont want to go in CSR route for now..
The present iteration takes care of reproducer and also satisifies JCK..
If in future it is needed, we can take a look..

Let's see what others have to say.

src/java.desktop/share/classes/javax/swing/SwingWorker.java

test/jdk/javax/swing/SwingWorker/TestDoneBeforeDoInBackground.java

mrserb · 2023-01-31T23:53:18Z

How the updated code is supposed to work if the worker will have some state updated by the "doInBackground" and used by the "done" method. I guess if the "doInBackground" will be canceled the "done" method may throw an exception? Do we have such usage in our codebase?

prsadhuk · 2023-02-01T05:47:19Z

The scenario is covered by JCK test and present PR iteration does not show any issue

aivanov-jdk · 2023-02-01T17:15:31Z

How the updated code is supposed to work if the worker will have some state updated by the "doInBackground" and used by the "done" method.

What do you mean?

~~Not much has changed to the way done is called.~~ Previously doneEDT was called from FutureTask implementation, its overridden done method. Now, doneEDT is called right from doInBackground.

Thus, if cancel was called on EDT, doneEDT and consequently SwingWorker.done were called on EDT (the latter was called synchronously). That is doneEDT was called on the thread which called cancel.

Now, doneEDT is always called on the background thread whether it's cancelled or not.

Yet no one should have depended on that because it was an implementation detail rather than a contract.

I guess if the "doInBackground" will be canceled the "done" method may throw an exception?

Why is it? done cannot throw exceptions.

This entire scenario is where doInBackground is cancelled but done gets called before doInBackground exits.

Yet get method throws CancellationException if the work for doInBackground is cancelled. It's specified in the Future interface.

Do we have such usage in our codebase?

I found the CheckCancellationException.java test that you wrote. It verifies that calling get throws CancellationException.

SwingWorker is used in JEditorPane as well as in jconsole packages. Yet I didn't look into the details how it's used.

mrserb · 2023-02-01T19:42:30Z

SwingWorker is used in JEditorPane as well as in jconsole packages. Yet I didn't look into the details how it's used.

I think all of that usages by jconsole will start to throw the "CancellationException" in the "done" method since now we will start to call it. Not sure can it cause some issue or not.

aivanov-jdk · 2023-02-01T22:40:14Z

SwingWorker is used in JEditorPane as well as in jconsole packages. Yet I didn't look into the details how it's used.

I think all of that usages by jconsole will start to throw the "CancellationException" in the "done" method since now we will start to call it. Not sure can it cause some issue or not.

Why do you think so?

The done method hasn't thrown CancellationException, and it will not. If it had thrown the exception, Prasanta's test would've caught it.

Additionally, the done method must be called. It was called before, it is called now.

I cannot understand your concern.

prsadhuk · 2023-02-02T04:16:18Z

Implementation of done method is not changed to cause/invoke CancellationException .
As per JDK-6608234 "CancellationException" will be called for get method if cancel is invoked, which is still valid/working and as mentioned, we have not seen any exception either in jtreg or in jck with updated fix.

test/jdk/javax/swing/SwingWorker/TestDoneBeforeDoInBackground.java

openjdk · 2023-02-10T14:50:24Z

@prsadhuk The CSR requirement cannot be removed as there is already a CSR associated with the issue JDK-8081474. Please withdraw the CSR JDK-8301940 and then use the command /csr unneeded again.

prsadhuk · 2023-02-10T14:52:05Z

/csr unneeded

openjdk · 2023-02-10T14:53:00Z

@prsadhuk determined that a CSR request is not needed for this pull request.

test/jdk/javax/swing/SwingWorker/TestDoneBeforeDoInBackground.java

prsadhuk · 2023-02-10T15:26:33Z

I have tried alignment to the extent possible..I am not sure if it's required to be so finicky about it :-)

aivanov-jdk · 2023-02-10T16:19:59Z

I have tried alignment to the extent possible..

Not to the extent possible if it's different from the commonly used style.

I am not sure if it's required to be so finicky about it :-)

It's probably not. However, the code which uses consistent style in the entire project is easier to read, it looks as if it was written by a single person even though it was actually written by a large team of different people. Otherwise, it's like a text with typos, you're still able to comprehend it but you stumble on each occurrence of one.

aivanov-jdk · 2023-02-11T16:06:54Z

I have tried alignment to the extent possible..

Not to the extent possible if it's different from the commonly used style.

I am not sure if it's required to be so finicky about it :-)

It's probably not.

In addition to that, in all the cases I have provided suggestions to fix the indentation which can be applied with a single click.

prsadhuk · 2023-02-13T09:21:26Z

I guess current iteration of the PR takes care of the alignment being suggested..

aivanov-jdk

Should evt.getNewValue() be used to identify the new value of the state property?

The test does not fail if I change the order of the lines in the finally block that reverses the order in which the done method is called and listeners are notified. I expected it to fail. Isn't it the reason why PropertyChangeListener was added?

test/jdk/javax/swing/SwingWorker/TestDoneBeforeDoInBackground.java

aivanov-jdk

I would appreciate if you update the copyright year in SwingWorker.

src/java.desktop/share/classes/javax/swing/SwingWorker.java

prsadhuk · 2023-02-27T09:17:02Z

/integrate

openjdk · 2023-02-27T09:20:17Z

Going to push as commit dbb5581.
Since your change was applied there have been 864 commits pushed to the master branch:

306134d: 8300792: Refactor examples in java.net.http to use @snippet
a2c5a4a: 8302732: sun/net/www/http/HttpClient/MultiThreadTest.java still failing intermittently
db217c9: 8292583: Comment for ciArrayKlass::make is wrong
2613b94: 8302149: Speed up compiler/jsr292/methodHandleExceptions/TestAMEnotNPE.java
d2660a6: 8303045: Remove RegionNode::LoopStatus::NeverIrreducibleEntry assert with wrong assumption
1794f49: 8302681: [IR Framework] Only allow cpuFeatures from a verified list
a43931b: 8303102: jcmd: ManagementAgent.status truncates the text longer than O_BUFLEN
2fb1e3b: 8302268: Prefer ArrayList to LinkedList in XEmbeddedFramePeer
3a9f491: 8301964: Expensive fillInStackTrace operation in HttpURLConnection.getLastModified when no last-modified in response
1dbd18a: 8302120: Prefer ArrayList to LinkedList in AggregatePainter
... and 854 more: https://git.openjdk.org/jdk/compare/ce6395a1356a3d1334aeffc70ac8e4f86dd81a4c...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-02-27T09:20:36Z

@prsadhuk Pushed as commit dbb5581.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

8081474: SwingWorker calls 'done' before the 'doInBackground' is fini…

7e9c466

…shed

openjdk bot added the client client-libs-dev@openjdk.org label Jan 11, 2023

prsadhuk added 2 commits January 11, 2023 11:10

Fix

4fe095b

Fix

ae0af86

openjdk bot added the rfr Pull request is ready for review label Jan 11, 2023

Fix

70e4a8c

mrserb reviewed Jan 12, 2023

View reviewed changes

src/java.desktop/share/classes/javax/swing/SwingWorker.java Outdated Show resolved Hide resolved

Remove blocking thread sleep for EDT

4f59bfa

aivanov-jdk suggested changes Jan 29, 2023

View reviewed changes

prsadhuk added 2 commits January 31, 2023 05:52

Fix and test updated

c34d55b

Set DONE state for cancel

5d723a7

aivanov-jdk suggested changes Jan 31, 2023

View reviewed changes

turbanoff reviewed Jan 31, 2023

View reviewed changes

test/jdk/javax/swing/SwingWorker/TestDoneBeforeDoInBackground.java Outdated Show resolved Hide resolved

prsadhuk added 2 commits January 31, 2023 11:10

Fix and test updated

4e93f1c

Test update

ff5d18a

aivanov-jdk suggested changes Jan 31, 2023

View reviewed changes

test/jdk/javax/swing/SwingWorker/TestDoneBeforeDoInBackground.java Outdated Show resolved Hide resolved

test/jdk/javax/swing/SwingWorker/TestDoneBeforeDoInBackground.java Outdated Show resolved Hide resolved

Test update

f3b59bd

aivanov-jdk suggested changes Feb 2, 2023

View reviewed changes

Test update

09d43ae

openjdk bot removed the csr Pull request needs approved CSR before integration label Feb 10, 2023

Space rectify

6776db1

aivanov-jdk suggested changes Feb 10, 2023

View reviewed changes

Test alignment

b2a1302

Test alignment

d1c9541

aivanov-jdk suggested changes Feb 14, 2023

View reviewed changes

test/jdk/javax/swing/SwingWorker/TestDoneBeforeDoInBackground.java Show resolved Hide resolved

Test update

ca7f54f

aivanov-jdk approved these changes Feb 15, 2023

View reviewed changes

test/jdk/javax/swing/SwingWorker/TestDoneBeforeDoInBackground.java Outdated Show resolved Hide resolved

Test fix

22ef8f5

aivanov-jdk approved these changes Feb 16, 2023

View reviewed changes

mrserb reviewed Feb 16, 2023

View reviewed changes

src/java.desktop/share/classes/javax/swing/SwingWorker.java Show resolved Hide resolved

Copyright year updated

9d12744

aivanov-jdk approved these changes Feb 17, 2023

View reviewed changes

mrserb approved these changes Feb 17, 2023

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Feb 17, 2023

openjdk bot added the integrated Pull request has been integrated label Feb 27, 2023

openjdk bot closed this Feb 27, 2023

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Feb 27, 2023

prsadhuk deleted the JDK-8081474 branch February 27, 2023 09:23

	/**
	* {@inheritDoc}
	*/
	public final boolean isDone() {
	return future.isDone();
	}

	/**
	* {@code SwingWorker} is {@code DONE}
	* after {@code doInBackground} method
	* is finished.
	*/
	DONE

		setState(StateValue.DONE);
		return future.cancel(mayInterruptIfRunning);

Conversation

prsadhuk commented Jan 11, 2023 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issues

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented Jan 11, 2023

Uh oh!

openjdk bot commented Jan 11, 2023

Uh oh!

mlbridge bot commented Jan 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

Uh oh!

prsadhuk commented Jan 27, 2023

Uh oh!

aivanov-jdk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

aivanov-jdk commented Jan 29, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mrserb commented Jan 31, 2023

Uh oh!

prsadhuk commented Feb 1, 2023

Uh oh!

aivanov-jdk commented Feb 1, 2023

Uh oh!

mrserb commented Feb 1, 2023

Uh oh!

aivanov-jdk commented Feb 1, 2023

Uh oh!

prsadhuk commented Feb 2, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

openjdk bot commented Feb 10, 2023

Uh oh!

prsadhuk commented Feb 10, 2023

Uh oh!

openjdk bot commented Feb 10, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

prsadhuk commented Jan 11, 2023 •

edited by openjdk bot

Loading

mlbridge bot commented Jan 11, 2023 •

edited

Loading

prsadhuk commented Feb 10, 2023 •

edited

Loading

aivanov-jdk commented Feb 11, 2023 •

edited

Loading

aivanov-jdk left a comment •

edited

Loading