Fix NullPointerException on a parallel run #35

jankratochvil · 2022-08-17T13:12:28Z

Error: Unexpected exception occurred! java.lang.NullPointerException
java.lang.NullPointerException
	at com.sun.javatest.regtest.config.TestProperties$Cache.getEntryInternal(TestProperties.java:515)
	at com.sun.javatest.regtest.config.TestProperties$Cache.getEntryInternal(TestProperties.java:515)
	at com.sun.javatest.regtest.config.TestProperties$Cache.getEntry(TestProperties.java:502)
	at com.sun.javatest.regtest.config.TestProperties.getEntry(TestProperties.java:170)
	at com.sun.javatest.regtest.config.TestProperties.getTestNGRoot(TestProperties.java:123)
	at com.sun.javatest.regtest.config.RegressionTestFinder.scanFile(RegressionTestFinder.java:143)
	at com.sun.javatest.finder.TagTestFinder.scan(TagTestFinder.java:115)
	at com.sun.javatest.TestFinder.read(TestFinder.java:433)
	at com.sun.javatest.TRT_TreeNode.processFile(TRT_TreeNode.java:1259)
	at com.sun.javatest.TRT_TreeNode.scanIfNeeded(TRT_TreeNode.java:807)
	at com.sun.javatest.TRT_TreeNode.getTreeNode(TRT_TreeNode.java:616)
	at com.sun.javatest.TestResultTable.findNode(TestResultTable.java:400)
	at com.sun.javatest.TestResultTable.lookupNode(TestResultTable.java:533)
	at com.sun.javatest.TestResultTable.lookupInitURL(TestResultTable.java:571)
	at com.sun.javatest.TestResultTable.validatePath(TestResultTable.java:1000)
	at com.sun.javatest.regtest.config.TestManager.validatePath(TestManager.java:299)
	at com.sun.javatest.regtest.config.TestManager.getTests(TestManager.java:271)
	at com.sun.javatest.regtest.tool.Tool.createParameters(Tool.java:1659)
	at com.sun.javatest.regtest.tool.Tool.run(Tool.java:1293)
	at com.sun.javatest.regtest.tool.Tool.run(Tool.java:1082)
	at com.sun.javatest.regtest.tool.Tool.main(Tool.java:155)
	at com.sun.javatest.regtest.Main.main(Main.java:46)

Some debug output showing the problem:

	scanIfNeeded getRootRelativePath= separator=/ filesToScan[i]=resultdir
	processFile file=/resultdir isAbsolute=true
	TestFinder.java read file=/resultdir isAbsolute=true
	scanFile caller file=/resultdir
	scanFile File=/resultdir
	TestProperties.java:dir=/ rootDir=/home/azul/azul/zulu11-git/test/hotspot/jtreg file2=/resultdir file2.getParentFile()=/
	TestProperties.java:dir=null rootDir=/home/azul/azul/zulu11-git/test/hotspot/jtreg file2=/resultdir file2.getParentFile()=/

This happens with jtreg using -w:resultdir . getRootRelativePath() returns empty "" path. Other option would be to return "." but in such case it broke some other code.

This problem does not happen during a single run. It happens only when jtreg is being run in parallel, in my case:

	seq 1 100000|xargs -n1 -P64 ./runtest
	#! /bin/bash
	dir=result-test$$
	rm -rf $dir
	mkdir $dir
	set -o pipefail
	(JAVA_HOME=$HOME/azul/zulu11-git/build/linux-x86_64-normal-server-release/images/jdk/; $JAVA_HOME/bin/java -classpath $HOME/azul/jtreg/build/images/jtreg/lib/jtreg.jar:$HOME/azul/jtreg/build/images/jtreg/lib/javatest.jar:$HOME/azul/jtreg/build/images/jtreg/lib/asmtools.jar com.sun.javatest.regtest.Main -testjdk:$JAVA_HOME -othervm -verbose -ignore:quiet -retain:all -a -conc:1 -timeout:10 -vmoptions:-XX:+UnlockExperimentalVMOptions -w:$dir -noreport -dir:$JAVA_HOME/../../../../test/jdk/ -nativepath:$JAVA_HOME/../test/hotspot/jtreg/native ../hotspot/jtreg/vmTestbase/nsk/jdi/stress/serial/heapwalking001/TestDescription.java |& tee $dir/err) || exit 255
	rm -rf $dir

Unforunately I am not yet Author so I do not yet have a JBS account so I cannot file it to JBS (Java Bug System).

Progress

Change must not contain extraneous whitespace

Reviewers

Dmitry Bessonov (@dbessono - Committer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 35

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jtharness/pull/35.diff

bridgekeeper · 2022-08-17T13:12:51Z

👋 Welcome back jankratochvil! 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 · 2022-08-17T13:13:31Z

@jankratochvil This change now passes all automated pre-integration checks.

After integration, the commit message for the final commit will be:

Fix NullPointerException on a parallel run

Reviewed-by: dbessono

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been no new commits pushed to the master branch. If another commit should be pushed before you perform the /integrate command, your PR will be automatically rebased. If you prefer to avoid any potential automatic rebasing, please check the documentation for the /integrate command for further details.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@dbessono) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

mlbridge · 2022-08-17T13:18:11Z

Webrevs

dbessono · 2022-08-17T14:08:11Z

could it be a dup of https://bugs.openjdk.org/browse/CODETOOLS-7903229 ?

jankratochvil · 2022-08-17T14:38:33Z

could it be a dup of https://bugs.openjdk.org/browse/CODETOOLS-7903229 ?

No, I have tried now that patch but it has no effect in this case.

dbessono · 2022-08-24T13:56:51Z

src/com/sun/javatest/TRT_TreeNode.java

-                    processFile(new File(TestResultTable.getRootRelativePath(this) +
-                            File.separator + filesToScan[i]));
+                    String path = TestResultTable.getRootRelativePath(this);
+                    if (path != "")
+                        path += File.separator;
+                    processFile(new File(path + filesToScan[i]));


Thanks for the suggested patch. I'd like suggest a bit more compact and direct, e.g.:

String path = TestResultTable.getRootRelativePath(this); processFile(new File( "".equals(path) // Zero length string if the node is a root ? filesToScan[i] : (path + File.separator + filesToScan[i]) ));

jonathan-gibbons · 2022-09-01T00:42:10Z

src/com/sun/javatest/TRT_TreeNode.java

+                    processFile(new File( "".equals(path) // Zero length string if the node is a root
+                                          ? filesToScan[i] : (path + File.separator + filesToScan[i]) ));


Pretty ugly code, containing a complex expression inside the new File, and even path string computation instead of using a better File constructor.

I suggest something like:

processFile(path.isEmpty() // Zero length string if the node is a root ? new File(filesToScan[i]) : new File(path, filesToScan[i]));

What if path is null ?

Also, it might be better not to pull in an additional constructor that was not used before, but rather maximally stick with the existing code and APi calls it does

If null is a possibility, check for it.

?? I don't understand the concern about referencing a different constructor that has been around since the beginning of Java time, and why it is better to do string arithmetic to construct a file path.

Implicitly checking for null here below.
If we could keep the existing approaches and API calls in the legacy code base, let's not pull anything new without necessity. I guess we don't have necessity to call an API that is new to this context.

processFile("".equals(path) // Zero length string if the node is a root ? new File(filesToScan[i]) : new File(path + File.separator + filesToScan[i]));

If needed, refactoring like File constructors call optimizations deserves to be better done with a separate commit.

jankratochvil · 2022-09-01T08:05:07Z

getRootRelativePath() can never return null:

jtharness/src/com/sun/javatest/TestResultTable.java

Line 338 in b9f4a2c

/**

The NullPointerException happens later in the code because the wrongly computed filename does not exist. (That could be an unrelated bug whether it should not be handled better, for example in the case someone deletes such file underneath.)

dbessono · 2022-09-01T10:23:16Z

@jankratochvil thanks for the update. I guess even if some method now doesn't return null, we'd be better prepared if it starts in future due to some refactoring.

Can see that the recent change failed jcheck

Check failure on line 808 in src/com/sun/javatest/TRT_TreeNode.java
openjdk / jcheck

Whitespace error

Column 50: trailing whitespace

Once jcheck is OK, I guess we might be good to go to resolve the issues you are facing with a parallel run

Error: Unexpected exception occurred! java.lang.NullPointerException java.lang.NullPointerException at com.sun.javatest.regtest.config.TestProperties$Cache.getEntryInternal(TestProperties.java:515) at com.sun.javatest.regtest.config.TestProperties$Cache.getEntryInternal(TestProperties.java:515) at com.sun.javatest.regtest.config.TestProperties$Cache.getEntry(TestProperties.java:502) at com.sun.javatest.regtest.config.TestProperties.getEntry(TestProperties.java:170) at com.sun.javatest.regtest.config.TestProperties.getTestNGRoot(TestProperties.java:123) at com.sun.javatest.regtest.config.RegressionTestFinder.scanFile(RegressionTestFinder.java:143) at com.sun.javatest.finder.TagTestFinder.scan(TagTestFinder.java:115) at com.sun.javatest.TestFinder.read(TestFinder.java:433) at com.sun.javatest.TRT_TreeNode.processFile(TRT_TreeNode.java:1259) at com.sun.javatest.TRT_TreeNode.scanIfNeeded(TRT_TreeNode.java:807) at com.sun.javatest.TRT_TreeNode.getTreeNode(TRT_TreeNode.java:616) at com.sun.javatest.TestResultTable.findNode(TestResultTable.java:400) at com.sun.javatest.TestResultTable.lookupNode(TestResultTable.java:533) at com.sun.javatest.TestResultTable.lookupInitURL(TestResultTable.java:571) at com.sun.javatest.TestResultTable.validatePath(TestResultTable.java:1000) at com.sun.javatest.regtest.config.TestManager.validatePath(TestManager.java:299) at com.sun.javatest.regtest.config.TestManager.getTests(TestManager.java:271) at com.sun.javatest.regtest.tool.Tool.createParameters(Tool.java:1659) at com.sun.javatest.regtest.tool.Tool.run(Tool.java:1293) at com.sun.javatest.regtest.tool.Tool.run(Tool.java:1082) at com.sun.javatest.regtest.tool.Tool.main(Tool.java:155) at com.sun.javatest.regtest.Main.main(Main.java:46) Some debug output showing the problem: scanIfNeeded getRootRelativePath= separator=/ filesToScan[i]=resultdir processFile file=/resultdir isAbsolute=true TestFinder.java read file=/resultdir isAbsolute=true scanFile caller file=/resultdir scanFile File=/resultdir TestProperties.java:dir=/ rootDir=/home/azul/azul/zulu11-git/test/hotspot/jtreg file2=/resultdir file2.getParentFile()=/ TestProperties.java:dir=null rootDir=/home/azul/azul/zulu11-git/test/hotspot/jtreg file2=/resultdir file2.getParentFile()=/ This happens with jtreg using -w:resultdir . getRootRelativePath() returns empty "" path. Other option would be to return "." but in such case it broke some other code. This problem does not happen during a single run. It happens only when jtreg is being run in parallel, in my case: seq 1 100000|xargs -n1 -P64 ./runtest #! /bin/bash dir=result-test$$ rm -rf $dir mkdir $dir set -o pipefail (JAVA_HOME=$HOME/azul/zulu11-git/build/linux-x86_64-normal-server-release/images/jdk/; $JAVA_HOME/bin/java -classpath $HOME/azul/jtreg/build/images/jtreg/lib/jtreg.jar:$HOME/azul/jtreg/build/images/jtreg/lib/javatest.jar:$HOME/azul/jtreg/build/images/jtreg/lib/asmtools.jar com.sun.javatest.regtest.Main -testjdk:$JAVA_HOME -othervm -verbose -ignore:quiet -retain:all -a -conc:1 -timeout:10 -vmoptions:-XX:+UnlockExperimentalVMOptions -w:$dir -noreport -dir:$JAVA_HOME/../../../../test/jdk/ -nativepath:$JAVA_HOME/../test/hotspot/jtreg/native ../hotspot/jtreg/vmTestbase/nsk/jdi/stress/serial/heapwalking001/TestDescription.java |& tee $dir/err) || exit 255 rm -rf $dir

jankratochvil · 2022-09-01T11:40:48Z

@dbessono I am not sure - is this patch OK or should I check whether getRootRelativePath() has returned null?
Otherwise I have fixed the whitespace, sorry for not noticing the jcheck error, it sends no mails.

dbessono · 2022-09-01T11:59:40Z

The current patch takes possible null value into account and won't generate NPE, as per https://docs.oracle.com/javase/8/docs/api/java/lang/String.html#equals-java.lang.Object-

dbessono · 2022-09-01T12:11:58Z

/integrate

jankratochvil · 2022-09-01T12:12:12Z

It won't generate NPE but it will produce String "null/filename". Which will produce NPE later in the code like the original problem this patch tries to fix. But then we do not know what return value null from getRootRelativePath() should mean. So we do not know what to do with such return value anyway.
But I prefer it this way, thanks for the approval.

openjdk · 2022-09-01T12:12:28Z

@dbessono Only the author (@jankratochvil) is allowed to issue the integrate command.

jankratochvil · 2022-09-01T12:12:46Z

/integrate

openjdk · 2022-09-01T12:13:32Z

@jankratochvil
Your change (at version c52f73a) is now ready to be sponsored by a Committer.

dbessono · 2022-09-01T12:17:21Z

It won't generate NPE but it will produce String "null/filename". Which will produce NPE later in the code like the original problem this patch tries to fix. But then we do not know what return value null from getRootRelativePath() should mean. So we do not know what to do with such return value anyway. But I prefer it this way, thanks for the approval.

you are right, as a follow up I'd update the code to

                    processFile(path == null || path.isEmpty() // Zero length string if the node is a root

dbessono · 2022-09-01T12:24:58Z

/sponsor

openjdk · 2022-09-01T12:25:49Z

Going to push as commit 2b3e2b6.

openjdk · 2022-09-01T12:26:06Z

@dbessono @jankratochvil Pushed as commit 2b3e2b6.

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

…parallel run #35`

jankratochvil · 2022-09-01T13:05:22Z

I see you have already done the path == null || part so we are done, thanks!

dbessono · 2022-09-01T13:08:32Z

I see you have already done the path == null || part so we are done, thanks!

yep, could you please check that the current state of the main JTH branch allows you to unblock the testing in the configuration you are using.

jankratochvil · 2022-09-01T16:27:08Z

I have verified:
https://github.com/jankratochvil/jtharness/tree/badnpe (=before this commit) does throw NPE
https://github.com/jankratochvil/jtharness/tree/master does not throw NPE

dbessono · 2022-09-01T17:26:18Z

I have verified: https://github.com/jankratochvil/jtharness/tree/badnpe (=before this commit) does throw NPE https://github.com/jankratochvil/jtharness/tree/master does not throw NPE

Thank you! Looks we could add a tag for b24

openjdk bot added ready Pull request is ready to be integrated rfr Pull request is ready for review labels Aug 17, 2022

jankratochvil mentioned this pull request Aug 18, 2022

Fix jtreg build with /usr/bin/git available #34

Closed

1 task

jankratochvil force-pushed the npe branch from 560fe3c to 1eeeb96 Compare August 18, 2022 07:52

dbessono requested a review from jonathan-gibbons August 24, 2022 13:46

dbessono reviewed Aug 24, 2022

View reviewed changes

jankratochvil force-pushed the npe branch from 1eeeb96 to 92f964d Compare August 24, 2022 14:04

jonathan-gibbons reviewed Sep 1, 2022

View reviewed changes

jankratochvil force-pushed the npe branch from 92f964d to 9053252 Compare September 1, 2022 08:07

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Sep 1, 2022

jankratochvil force-pushed the npe branch from 9053252 to c52f73a Compare September 1, 2022 11:36

openjdk bot added ready Pull request is ready to be integrated rfr Pull request is ready for review labels Sep 1, 2022

dbessono self-requested a review September 1, 2022 12:11

dbessono approved these changes Sep 1, 2022

View reviewed changes

openjdk bot added the sponsor Pull request requires a sponsor label Sep 1, 2022

openjdk bot added the integrated Pull request has been integrated label Sep 1, 2022

openjdk bot closed this Sep 1, 2022

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review sponsor Pull request requires a sponsor labels Sep 1, 2022

dbessono added a commit that referenced this pull request Sep 1, 2022

(follow up) additional improvement to `Fix NullPointerException on a …

579fd52

…parallel run #35`

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix NullPointerException on a parallel run #35

Fix NullPointerException on a parallel run #35

jankratochvil commented Aug 17, 2022 •

edited by openjdk bot

bridgekeeper bot commented Aug 17, 2022

openjdk bot commented Aug 17, 2022 •

edited

mlbridge bot commented Aug 17, 2022 •

edited

dbessono commented Aug 17, 2022

jankratochvil commented Aug 17, 2022

dbessono Aug 24, 2022

jonathan-gibbons Sep 1, 2022

dbessono Sep 1, 2022

dbessono Sep 1, 2022

jonathan-gibbons Sep 1, 2022

dbessono Sep 1, 2022

dbessono Sep 1, 2022

jankratochvil commented Sep 1, 2022

dbessono commented Sep 1, 2022

jankratochvil commented Sep 1, 2022

dbessono commented Sep 1, 2022

dbessono commented Sep 1, 2022

jankratochvil commented Sep 1, 2022

openjdk bot commented Sep 1, 2022

jankratochvil commented Sep 1, 2022

openjdk bot commented Sep 1, 2022

dbessono commented Sep 1, 2022

dbessono commented Sep 1, 2022

openjdk bot commented Sep 1, 2022

openjdk bot commented Sep 1, 2022

jankratochvil commented Sep 1, 2022

dbessono commented Sep 1, 2022 •

edited

jankratochvil commented Sep 1, 2022

dbessono commented Sep 1, 2022

		processFile(new File( "".equals(path) // Zero length string if the node is a root
		? filesToScan[i] : (path + File.separator + filesToScan[i]) ));

Fix NullPointerException on a parallel run #35

Fix NullPointerException on a parallel run #35

Conversation

jankratochvil commented Aug 17, 2022 • edited by openjdk bot

Progress

Reviewers

Reviewing

bridgekeeper bot commented Aug 17, 2022

openjdk bot commented Aug 17, 2022 • edited

mlbridge bot commented Aug 17, 2022 • edited

Webrevs

dbessono commented Aug 17, 2022

jankratochvil commented Aug 17, 2022

dbessono Aug 24, 2022

Choose a reason for hiding this comment

jonathan-gibbons Sep 1, 2022

Choose a reason for hiding this comment

dbessono Sep 1, 2022

Choose a reason for hiding this comment

dbessono Sep 1, 2022

Choose a reason for hiding this comment

jonathan-gibbons Sep 1, 2022

Choose a reason for hiding this comment

dbessono Sep 1, 2022

Choose a reason for hiding this comment

dbessono Sep 1, 2022

Choose a reason for hiding this comment

jankratochvil commented Sep 1, 2022

dbessono commented Sep 1, 2022

jankratochvil commented Sep 1, 2022

dbessono commented Sep 1, 2022

dbessono commented Sep 1, 2022

jankratochvil commented Sep 1, 2022

openjdk bot commented Sep 1, 2022

jankratochvil commented Sep 1, 2022

openjdk bot commented Sep 1, 2022

dbessono commented Sep 1, 2022

dbessono commented Sep 1, 2022

openjdk bot commented Sep 1, 2022

openjdk bot commented Sep 1, 2022

jankratochvil commented Sep 1, 2022

dbessono commented Sep 1, 2022 • edited

jankratochvil commented Sep 1, 2022

dbessono commented Sep 1, 2022

jankratochvil commented Aug 17, 2022 •

edited by openjdk bot

openjdk bot commented Aug 17, 2022 •

edited

mlbridge bot commented Aug 17, 2022 •

edited

dbessono commented Sep 1, 2022 •

edited