Hi Ramanand,
test/tools/launcher/LauncherMessageTest.java
1)
116 String[] commands = {"java", "--module-path", modules.getPath(),
117 "-m", "mod.b/pkgB.ClassB"};
The execution PATH may or may not contain the JAVA_HOME_UNDER_TEST/bin,
so the right "java" may not be picked up, suggest using the TestHelper's
javaCmd field that will be set correctly.
2)
122 if (!result.isOK() && result.contains("JNI error")) {
123 result.testOutput.forEach(System.err::println);
124 throw new RuntimeException("Test Failed with JNI error!");
125 } else {
126 System.out.println("Test Passed...");
127 }
The problem with 122 if it the test returns back with non-zero code, it will
still fail with "Test Failed with JNI error", I prefer it to be broken
up and return
back the right message ie. if the test returns with non-zero code say that.
3)
Also usage of RuntimeException is over the top, you can simply throw an
Exception and have the main throws Exception, jtreg will do the needful.
4)
102 FileUtils.deleteFileWithRetry(Paths.get(modules.getPath() + File.separator +
"mod.a.jar"));
there are redundant use of File.separator, Paths.get should create
the FS correctly on the target platform
ex: Paths.get(modules.getPath(), "mod.a.jar");
Personally I stay away from using raw File.separator, instead
have the APIs do the work for me.
Kumar
Hi Alan,
Thank you for the review.
My comments are inline and Webrev is updated here:
http://cr.openjdk.java.net/~rpatil/8167063/webrev.01/
Change Summary:
- Removed SecurityException handling
- Updated the error message in launcher.properties
- Removed loadModuleMainClass0 method and moved the code back into original
loadModuleMainClass
- NoClassDefFoundError is replaced by its parent class LinkageError in method
loadMainClass
Regards,
Ramanand.
-----Original Message-----
From: Alan Bateman
Sent: Friday, January 20, 2017 8:03 PM
To: Ramanand Patil <ramanand.pa...@oracle.com>; core-libs-dev@openjdk.java.net
Subject: Re: RFR: 8167063: spurious message "A JNI error has occurred" if
start-class cannot be initialized
On 20/01/2017 13:21, Ramanand Patil wrote:
Hi all,
Please review the following bug fix:
Bug: https://bugs.openjdk.java.net/browse/JDK-8167063
Webrev: http://cr.openjdk.java.net/~rpatil/8167063/webrev.00/
Handled the SecurityException and LinkageError which can be thrown from
Class.forName(...) method used in LauncherHelper.java and added corresponding
error messages to launcher.properties.
Though the reported bug is because of the LinkageError, Security exception is
also handled to be safe from calling Class.forName method.
I see this changes loadMainClass to abort with resources that are keyed on
java.launcher.cls.error6 and java.launcher.cls.error7 but they don't appear in
the launcher.properties file. Does this work? I would assume
MissingResourceException is thrown.
Thanks for pointing this. I missed to add it to launcher.properties. But now I
have changed error6 to point to java.launcher.cls.error1 and removed security
exception handling.
For java.launcher.module.error3 and java.launcher.module.error4 then "link" is likely to
confuse people. Sure, there may be linkage errors but there are many linkage errors and I think
would be a lot clearer if the message was "Unable to load main class {0} from module
{1}\n\{2}".
Updated the message as suggested.
It's not clear to me that having a different message for security exceptions
make sense, esp. when the exception is printed. So I think I would drop that.
Yes, I agree to drop out the Security exceptions handling.
Also loadModuleMainClass0 is unusual method name, we've mostly (not
always) used the 0 suffix on native methods. In any case, it doesn't look like
it is needed, the code was okay in loadModuleMainClass as it was.
The method was added just to look the exception handling easier. I have removed
the extra method and placed the code back into loadModuleMainClass.
One final point is that is the nesting catching of LinkageError and
SecurityException in loadMainClass, I assume you don't need the inner catch.
I think both the catch blocks were needed, because the first(inner) catch was
already inside catch.(This was same as NoClassDefFoundError catching twice in
the same method). Now since NoClassDefFoundError is subclass of LinkageError, I
have replaced it with LinkageError. And I think still the same abort message
holds good. Please let me know if this is ok.
-Alan
