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

[Peter Levart]

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/4563a4be/attachment-0001.html>