Thank you Alan and Kumar for your review.
Here is the updated webrev: http://cr.openjdk.java.net/~rpatil/8167063/webrev.03
Changes done:
1. LinkageError in method loadMainClass is handled separately and with a new
message in launcher.properties
2. Test case updated to print the error message(even when test passes) and to
print the final test result message as suggested by Kumar.
Regards,
Ramanand.
-----Original Message-----
From: Kumar Srinivasan
Sent: Wednesday, January 25, 2017 9:30 PM
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
116 if (result.isOK()) {
117 throw new Exception("Test Passed Unexpectedly!");
118 } else if (result.contains("JNI error")) {
119 result.testOutput.forEach(System.err::println);
120 throw new Exception("Test Failed with JNI error!");
121 }
+ 122 System.out.println("Test passes, failed with expected error message");
As a sanity I would add the above. This will make it clear to someone trying to
triage an error.
Kumar
On 1/25/2017 2:10 AM, Ramanand Patil wrote:
> 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
