RFR: 8210839 Improve interaction between source launcher and classpath

[Jonathan Gibbons]

On 09/22/2018 12:58 AM, Alan Bateman wrote:
> On 22/09/2018 07:31, Alan Bateman wrote:
>> On 21/09/2018 23:44, mandy chung wrote:
>>> On 9/21/18 10:53 AM, Jonathan Gibbons wrote:
>>>> Updated webrev, incorporating review feedback.
>>>>
>>>> Webrev:_http://cr.openjdk.java.net/~jjg/8210839/webrev.01/index.html _
>>>
>>> I agree that finding class locally first before delegating to the 
>>> parent is a reasonable solution in particular with the simple use of 
>>> this feature.
>>>
>>> This change looks good (changing to super.getResource(s) to 
>>> getParent().getResource(s) is a good one).
>> I can't see the change just as cr.openjdk.java.net isn't back yet. 
>> However just to say that loadClass will need to do the same to avoid 
>> a delegation loop and stack overflow when a class is not found.
> cr.ojn is back so I see the proposed patch. The new loadClass looks 
> much better but it calls super.loadClass when it should be 
> parent.loadClass. I also see that it delegates before checking if the 
> class loader was recorded as an initiating loader. I think it needs to 
> change to this:
>
>             synchronized (getClassLoadingLock(name)) {
>                 Class<?> c = findLoadedClass(name);
>                 if (c == null) {
>                     if (map.containsKey(name)) {
>                         c = findClass(name);
>                     } else {
>                         c = parent.loadClass(name);
>                     }
>                 }
>                 if (resolve) {
>                     resolveClass(c);
>                 }
>                 return c;
>             }
>
> One suggestion for a test is Class.forName("MIA") to ensure that CNFE 
> is reported. I think that provide better confident that you don't have 
> a stack overflow.
>
> -Alan

Alan, Mandy,

Thanks for the additional feedback.

New webrev:

* addresses issues with loadClass
* renames "map" to "sourceFileClasses" for added clarity
* fixes/improves comments on MemoryClassLoader and contents
* adds new test case as suggested

Webrev: http://cr.openjdk.java.net/~jjg/8210839/webrev.02/index.html

-- Jon
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/compiler-dev/attachments/20180924/b5f2ea54/attachment-0001.html>