RFR: JDK-8210009: Source Launcher classloader should support getResource and getResourceAsStream

[Jonathan Gibbons]

Updated webrev, addressing comments from Peter.

1. Fixed protocol string, and updated test to verify that URLs are OK
2. Changed init of the stream handler

Webrev: http://cr.openjdk.java.net/~jjg/8210009/webrev.02/

-- Jon

On 09/05/2018 01:43 AM, Peter Levart wrote:
> Hi Jon,
>
> A few comments at the end...
>
> On 09/04/2018 10:52 PM, Jonathan Gibbons wrote:
>> Please review a change to add support for the getResource* group of 
>> methods
>> to the internal class loader used by the Source Launcher, introduced 
>> in JEP 330.
>>
>> The overall issue has been discussed on OpenJDK lists, [1,2]
>>
>> The implementation consists of providing findResource and findResources,
>> along with a couple of minor additional classes to implement 
>> URLStreamHandler
>> and URLConnection.
>>
>> There is one implementation detail worthy of note. The "new URL" should
>> be inside a doPrivileged block, but overall, the Source Launcher does 
>> not
>> work when a security manager in installed. This is being tracked as 
>> JDK-8210274,
>> and the appropriate doPrivileged call will be added as part of that fix.
>>
>> -- Jon
>>
>> JBS: https://bugs.openjdk.java.net/browse/JDK-8210009
>> Webrev: http://cr.openjdk.java.net/~jjg/8210009/webrev.01/
>
> In the protocol (schema) name:
>
>  618         private final String PROTOCOL = "sourcelauncher_" + 
> getClass().getSimpleName() + hashCode();
>
> ...which looks like "sourcelauncher_MemoryClassLoader-12345", the 
> underscore is not an allowed character. The syntax of schema name is:
>
> alpha *( alpha | digit | "+" | "-" | "." )
>
> URL does not check for that, but when you perform say url.toURI() you 
> get URISyntaxException.
>
> If the intent was to append hashCode() to make the schema name unique 
> per MemoryClassLoader instance (is there a reason for that?), then 
> hashCode() does not guarantee that. Although the probability of a 
> clash is very low.
>
> Was the intent of randomizing protocol name preventing things like 
> making url3 below succeed in returning "wrong" resource?
>
> MemoryClassLoader cl1 = ...
> MemoryClassLoader cl2 = ...
> URL url1 = cl1.getResource(path1);
> URL url2 = cl2.getResource(path2);
>
> URL url3 = new URL(url1, url2.toString());
>
>
> findResource() may be called concurrently from multiple threads, so 
> lazy initialization of 'handler' field should be performed carefully 
> using local variables:
>
>             URLStreamHandler handler = this.handler;
>             if (handler == null) {
>                 this.handler = handler = new MemoryURLStreamHandler();
>             }
>
> ...then use local var 'hander' for constructing URL, or you risk 
> reorderings which might pass null as handler parameter to URL 
> constructor. This is still publication via data race, but should be 
> safe as the only MemoryURLStreamHandler's state is a synthetic final 
> field referencing outer MemoryClassLoader instance.
>
>
> I haven't tried to run the test, but how does this work:
>
>   56         String[] names = {
>   57             "p/q/CLTest.class",
>   58             "p/q/CLTest$Inner.class",
>   59             "p/q/CLTest2.class",
>   60             "java/lang/Object.class",
>   61             "UNKNOWN.class",
>   62             "UNKNOWN"
>   63         };
>
>
> ...and then:
>
>   80             URL u = cl.getResource(name);
>   81             if (u == null) {
>   82                 if (!name.contains("UNKNOWN")) {
>   83                     error("resource not found: " + name);
>   84                 }
>
> Shouldn't resource name "p/q/CLTest2.class" raise an error?
>
>
>
> Regards, Peter
>
>

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