Java 14: track interrupt state when thread is dead #9341

theresa-m · 2020-04-23T15:20:35Z

Fixes: #8916

A new private boolean is added to java.lang.Thread to track the interrupt state since a J9VMThread only exists when the thread is actually running. When a thread is started the interrupt state will be transferred from the deadInterrupt boolean to the J9VMThread and tracked in the vm. When the thread is cleaned up, the interrupt state will be transferred back to be tracked by the deadInterrupt boolean.
Update formatting for Java14AndUp test playlist

Signed-off-by: Theresa Mammarella Theresa.T.Mammarella@ibm.com

jcl/src/java.base/share/classes/java/lang/Thread.java

test/functional/Java14andUp/playlist.xml

test/functional/Java14andUp/src/org/openj9/test/java/lang/Test_Thread.java

pshipton · 2020-04-24T20:35:59Z

Once the test suite is finalized, pls confirm it passes on Hotspot.

pshipton · 2020-05-05T17:34:24Z

lgtm. Do the tests pass on Hotspot?

pshipton · 2020-05-05T17:37:07Z

test/functional/Java14andUp/playlist.xml

+            <variation>NoOptions</variation>
+        </variations>
+        <command>$(JAVA_COMMAND) $(JVM_OPTIONS) \
+            --enable-preview \


Is --enable-preview needed for this test?

Without --enable-preview here I am getting java.lang.UnsupportedClassVersionError: Preview features are not enabled for org/openj9/test/java/lang/Test_Class when I try to run ThreadInterruptImplTest.

Is there a preferable way to set this up @llxia? Note Test_Class is part of Jep359Tests and is not related to ThreadInterruptImplTest.

Will we remove --enable-preview option in the future? If so, you can move it in variation <variation>--enable-preview</variation>, so it is more visible.

So now I am curious why org/openj9/test/java/lang/Test_Class is involved when we're running tests from org/openj9/test/java/lang/Test_Thread. Is it just some side affect of testng? Do you have the stack trace?

Exception in thread "main" java.lang.UnsupportedClassVersionError: Preview features are not enabled for org/openj9/test/java/lang/Test_Class (class file version 58.65535). Try running with '--enable-preview' at java.base/java.lang.ClassLoader.defineClass1(Native Method) at java.base/java.lang.ClassLoader.defineClass(ClassLoader.java:1017) at java.base/java.security.SecureClassLoader.defineClass(SecureClassLoader.java:151) at java.base/jdk.internal.loader.BuiltinClassLoader.defineClass(BuiltinClassLoader.java:821) at java.base/jdk.internal.loader.BuiltinClassLoader.findClassOnClassPathOrNull(BuiltinClassLoader.java:719) at java.base/jdk.internal.loader.BuiltinClassLoader.loadClassOrNull(BuiltinClassLoader.java:642) at java.base/jdk.internal.loader.BuiltinClassLoader.loadClass(BuiltinClassLoader.java:600) at java.base/jdk.internal.loader.ClassLoaders$AppClassLoader.loadClass(ClassLoaders.java:178) at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:522) at org.testng.internal.ClassHelper.forName(ClassHelper.java:117) at org.testng.xml.XmlClass.loadClass(XmlClass.java:74) at org.testng.xml.XmlClass.init(XmlClass.java:69) at org.testng.xml.XmlClass.<init>(XmlClass.java:55) at org.testng.xml.TestNGContentHandler.startElement(TestNGContentHandler.java:575) at java.xml/com.sun.org.apache.xerces.internal.parsers.AbstractSAXParser.startElement(AbstractSAXParser.java:518) at java.xml/com.sun.org.apache.xerces.internal.parsers.AbstractXMLDocumentParser.emptyElement(AbstractXMLDocumentParser.java:183) at java.xml/com.sun.org.apache.xerces.internal.impl.dtd.XMLDTDValidator.emptyElement(XMLDTDValidator.java:752) at java.xml/com.sun.org.apache.xerces.internal.impl.XMLNSDocumentScannerImpl.scanStartElement(XMLNSDocumentScannerImpl.java:351) at java.xml/com.sun.org.apache.xerces.internal.impl.XMLDocumentFragmentScannerImpl$FragmentContentDriver.next(XMLDocumentFragmentScannerImpl.java:2725) at java.xml/com.sun.org.apache.xerces.internal.impl.XMLDocumentScannerImpl.next(XMLDocumentScannerImpl.java:605) at java.xml/com.sun.org.apache.xerces.internal.impl.XMLNSDocumentScannerImpl.next(XMLNSDocumentScannerImpl.java:112) at java.xml/com.sun.org.apache.xerces.internal.impl.XMLDocumentFragmentScannerImpl.scanDocument(XMLDocumentFragmentScannerImpl.java:541) at java.xml/com.sun.org.apache.xerces.internal.parsers.XML11Configuration.parse(XML11Configuration.java:888) at java.xml/com.sun.org.apache.xerces.internal.parsers.XML11Configuration.parse(XML11Configuration.java:824) at java.xml/com.sun.org.apache.xerces.internal.parsers.XMLParser.parse(XMLParser.java:141) at java.xml/com.sun.org.apache.xerces.internal.parsers.AbstractSAXParser.parse(AbstractSAXParser.java:1224) at java.xml/com.sun.org.apache.xerces.internal.jaxp.SAXParserImpl$JAXPSAXParser.parse(SAXParserImpl.java:635) at java.xml/com.sun.org.apache.xerces.internal.jaxp.SAXParserImpl.parse(SAXParserImpl.java:324) at java.xml/javax.xml.parsers.SAXParser.parse(SAXParser.java:197) at org.testng.xml.XMLParser.parse(XMLParser.java:38) at org.testng.xml.SuiteXmlParser.parse(SuiteXmlParser.java:16) at org.testng.xml.SuiteXmlParser.parse(SuiteXmlParser.java:9) at org.testng.xml.Parser.parse(Parser.java:152) at org.testng.xml.Parser.parse(Parser.java:233) at org.testng.TestNG.parseSuite(TestNG.java:295) at org.testng.TestNG.initializeSuitesAndJarFile(TestNG.java:348) at org.testng.TestNG.initializeEverything(TestNG.java:995) at org.testng.TestNG.run(TestNG.java:1009) at org.testng.TestNG.privateMain(TestNG.java:1354) at org.testng.TestNG.main(TestNG.java:1323)

I think the issue is that TestNG includes all tests from the suite on the classpath. I tried out a java program locally and added an unrelated record on the classpath and the same error happened.

theresa-m · 2020-05-05T18:17:00Z

Confirmed the tests pass with hotspot.

jcl/src/java.base/share/classes/java/lang/Thread.java

runtime/vm/vmthread.c

DanHeidinga · 2020-05-06T04:08:14Z

runtime/vm/vmthread.c

+
+	#if (JAVA_SPEC_VERSION >= 14)
+		/* Refresh java.lang.Thread interrupt value so it is acessible when vm thread is cleaned up. */
+		J9VMJAVALANGTHREAD_SET_DEADINTERRUPT(vmThread, vmThread->threadObject, omrthread_interrupted(vmThread->osThread));


Can this be moved into the existing java "cleanUpAttachedThread" call?

The call path, starting below in cleanUpAttachedThread is:
https://github.com/eclipse/openj9/blob/ed6c69f07441718bb9ab8cbf65c4d755f9ca56c3/runtime/vm/callin.cpp#L459

https://github.com/eclipse/openj9/blob/906d4009533c1c313e9e31a9e5db895e9ff0dcdf/jcl/src/java.base/share/classes/java/lang/J9VMInternals.java#L318

https://github.com/eclipse/openj9/blob/5a2386c150d3277d326c05224b206ef041ffbc73/jcl/src/java.base/share/classes/java/lang/Thread.java#L1527

Could this cleanup method call isInterrupted() and handle this final set in java? Using the java native to do the set would also clear the native thread's interrupted state which would be a plus, given we recycle the native threads at the vm level

If we can't do it in java, then I'd like to see this moved as late as possible - so it's done in the same acquire / release block as the cleanUpAttachedThread call

test/functional/Java14andUp/src/org/openj9/test/java/lang/Test_Thread.java

DanHeidinga · 2020-05-06T19:34:37Z

jcl/src/java.base/share/classes/java/lang/Thread.java

@@ -1525,6 +1551,11 @@ void uncaughtException(Throwable e) {
 * @see J9VMInternals#threadCleanup()
 */
 void cleanup() {
+/*[IF Java14]*/
+	/* Refresh deadInterrupt value so it is accurate when vm thread is cleaned up. */	
+	deadInterrupt = interrupted();


I like this approach. Can you confirm that the thread is "isAlive" at this point? Otherwise the interrupted() call will return the previous setting of deadInterrupt.

The thread will still be considered alive at this point. isAlive will start to return false when threadRef is cleared: https://github.com/eclipse/openj9/blob/5a2386c150d3277d326c05224b206ef041ffbc73/jcl/src/java.base/share/classes/java/lang/Thread.java#L1545

- A new private boolean is added to java.lang.Thread to track the interrupt state since a J9VMThread only exists when the thread is actually running. When a thread is started the interrupt state will be transferred from the deadInterrupt boolean to the J9VMThread and tracked in the vm. When the thread is cleaned up, the interrupt state will be transferred back to be tracked by the deadInterrupt boolean. - Update formatting for Java14AndUp test playlist Signed-off-by: Theresa Mammarella <Theresa.T.Mammarella@ibm.com>

DanHeidinga

lgtm

DanHeidinga · 2020-05-07T20:24:26Z

Jenkins test sanity plinux,win,osx jdk14

DanHeidinga · 2020-05-07T20:24:37Z

Jenkins test sanity zlinux jdk8

llxia

The test change LGTM

DanHeidinga · 2020-05-08T13:19:52Z

@pshipton Any remaining comments on this? Otherwise, it's good to merge

pshipton · 2020-05-08T14:57:34Z

@DanHeidinga lgtm

theresa-m force-pushed the thread_impl branch from 82bd4c0 to d0f53a8 Compare April 23, 2020 15:26

pshipton added comp:vm jdk14 labels Apr 23, 2020

pshipton added this to the Release 0.21 (Java 8, 11, 14) July refresh milestone Apr 23, 2020

pshipton reviewed Apr 23, 2020

View reviewed changes

jcl/src/java.base/share/classes/java/lang/Thread.java Show resolved Hide resolved

pshipton reviewed Apr 23, 2020

View reviewed changes

jcl/src/java.base/share/classes/java/lang/Thread.java Show resolved Hide resolved

theresa-m force-pushed the thread_impl branch from 02bf934 to c1d3d5f Compare April 24, 2020 18:48

theresa-m requested a review from pshipton April 24, 2020 18:48

pshipton reviewed Apr 24, 2020

View reviewed changes

theresa-m force-pushed the thread_impl branch 6 times, most recently from 7db4f6e to ba295f2 Compare April 29, 2020 20:55

theresa-m requested a review from pshipton April 29, 2020 20:56

pshipton requested a review from DanHeidinga May 5, 2020 17:34

pshipton reviewed May 5, 2020

View reviewed changes

pshipton requested review from pshipton and llxia May 5, 2020 17:37

DanHeidinga reviewed May 5, 2020

View reviewed changes

jcl/src/java.base/share/classes/java/lang/Thread.java Outdated Show resolved Hide resolved

jcl/src/java.base/share/classes/java/lang/Thread.java Outdated Show resolved Hide resolved

runtime/vm/vmthread.c Outdated Show resolved Hide resolved

runtime/vm/vmthread.c Show resolved Hide resolved

theresa-m force-pushed the thread_impl branch from cf868ec to 9e2f0d8 Compare May 5, 2020 22:33

theresa-m requested a review from DanHeidinga May 5, 2020 22:34

DanHeidinga reviewed May 6, 2020

View reviewed changes

theresa-m force-pushed the thread_impl branch from f926cc1 to d43bd50 Compare May 6, 2020 19:36

theresa-m force-pushed the thread_impl branch from bc0ff87 to 64f97ef Compare May 6, 2020 20:04

DanHeidinga approved these changes May 7, 2020

View reviewed changes

DanHeidinga self-assigned this May 7, 2020

llxia approved these changes May 7, 2020

View reviewed changes

fjeremic added the comp:test label May 7, 2020

pshipton approved these changes May 8, 2020

View reviewed changes

DanHeidinga merged commit 29f42b1 into eclipse-openj9:master May 8, 2020

llxia mentioned this pull request Jul 6, 2020

(v0.20.0) Upgrade ASM version to 8.0.1 #10092

Merged

pshipton mentioned this pull request Aug 27, 2020

Rework the code with Thread.interrupt() #10464

Merged

theresa-m deleted the thread_impl branch July 20, 2023 18:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Java 14: track interrupt state when thread is dead #9341

Java 14: track interrupt state when thread is dead #9341

theresa-m commented Apr 23, 2020 •

edited

Loading

pshipton commented Apr 24, 2020

pshipton commented May 5, 2020

pshipton May 5, 2020

theresa-m May 5, 2020

llxia May 5, 2020

pshipton May 5, 2020

theresa-m May 5, 2020

theresa-m May 5, 2020

theresa-m commented May 5, 2020

DanHeidinga May 6, 2020

DanHeidinga May 6, 2020

DanHeidinga May 6, 2020

theresa-m May 6, 2020

DanHeidinga left a comment

DanHeidinga commented May 7, 2020

DanHeidinga commented May 7, 2020

llxia left a comment

DanHeidinga commented May 8, 2020

pshipton commented May 8, 2020

Java 14: track interrupt state when thread is dead #9341

Java 14: track interrupt state when thread is dead #9341

Conversation

theresa-m commented Apr 23, 2020 • edited Loading

pshipton commented Apr 24, 2020

pshipton commented May 5, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theresa-m commented May 5, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DanHeidinga left a comment

Choose a reason for hiding this comment

DanHeidinga commented May 7, 2020

DanHeidinga commented May 7, 2020

llxia left a comment

Choose a reason for hiding this comment

DanHeidinga commented May 8, 2020

pshipton commented May 8, 2020

theresa-m commented Apr 23, 2020 •

edited

Loading