RFR: 8167063: spurious message "A JNI error has occurred" if start-class cannot be initialized

[Kumar Srinivasan]

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.patil at oracle.com>; core-libs-dev at 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