Hi Kumar,
Thank you for the review and suggestions for the test case.
Here is the updated Webrev:
http://cr.openjdk.java.net/~rpatil/8167063/webrev.02/
Regards,
Ramanand.
-----Original Message-----
From: Kumar Srinivasan
Sent: Tuesday, January 24, 2017 2:50 AM
To: Ramanand Patil <ramanand.pa...@oracle.com>
Cc: Alan Bateman <alan.bate...@oracle.com>; core-libs-dev
<core-libs-dev@openjdk.java.net>
Subject: Re: RFR: 8167063: spurious message "A JNI error has occurred" if
start-class cannot be initialized
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.
Used TestHelper.javaCmd for instead of "java"
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.
Changed the way test results were checked to include the check for unexpected
failure when result is not non-zero.(i.e. when isOK() returns true).
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.
Changed. Used an Exception instead of RuntimeException
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.
-Removed all instances of File.separator
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
