A compromise approach on module isolation

[David M. Lloyd]

Here's a commit which does so (and also covers the circularity case), if 
you want to play with it a bit:

https://github.com/dmlloyd/openjdk-modules/commit/ffbca0555aeb934181ac92ec3a2bf48fae7efe04

On 05/12/2017 08:45 AM, David M. Lloyd wrote:
> Hi Rémi, thanks for the feedback.
> 
> At the moment you can fix this by editing the launcher scripts and 
> adding "-Djdk.module.isolated=true" to the end of the 
> "JLINK_VM_OPTIONS=" line (and similarly for Windows).
> 
> I think the tool can do this automatically with only minimal changes. 
> I'll have a look at it.
> 
> On 05/12/2017 03:28 AM, Remi Forax wrote:
>> Hi David,
>> with this patch, there is an issue if people (like me :) ) are using 
>> jlink.
>>
>> With jlink, application modules becomes system modules so depending if 
>> the application is delivered as a bunch of jars or as a custom jlink 
>> image, so behavior will be different because the application modules 
>> deploy with jlink will not be sandboxed.
>>
>> Maybe it means that jlink has to be changed, i do not know, but i 
>> think i would prefer that the module itself should decide using the 
>> module-info if it should run its own classloader or not, at least 
>> jlink can refuse to work if a module ask for being deployed in its own 
>> classloader
>>
>> And i'm still not sure that this kind of code has to be in the jdk, it 
>> can be easily written outside without putting the burden of having to 
>> maintain it in the JDK.
>>
>> regards,
>> Rémi
>>
>>
>> ----- Mail original -----
>>> De: "David M. Lloyd" <david.lloyd at redhat.com>
>>> À: jpms-spec-experts at openjdk.java.net
>>> Envoyé: Vendredi 12 Mai 2017 01:17:45
>>> Objet: A compromise approach on module isolation
>>
>>> In the wake of the PR vote, one of the most oft-cited unmet expectations
>>> was the lack of package namespace isolation among modules.  I'd like to
>>> propose a patch [1] which would allow this type of module isolation to
>>> be used by the run time on an opt-in or opt-out basis.
>>>
>>> Justification:
>>>
>>> • Large applications are more likely to encounter concealed and
>>> non-concealed package conflicts.
>>> • By making the change opt-in or opt-out, any application which expects
>>> all of its modules to be loaded by a single class loader can still
>>> function, while allowing more complex applications (which already would
>>> necessarily have required at least some separated class loaders) to
>>> continue to work.
>>> • The patch seems to have a small complexity and maintenance footprint.
>>>
>>> Drawbacks:
>>>
>>> None that I am aware of.
>>>
>>> Patch information:
>>>
>>> The approach of this patch simply looks for a system property and,
>>> depending on the value of this property, establishes isolated module
>>> loaders.  I relinquish any copyright claim to the patch in this mail.
>>> Please do not get bogged down by any formatting problems introduced by
>>> the mailing list; the purpose of directly including the patch is to give
>>> a clear, unambiguous subject for discussion.  I can provide a proper
>>> webrev (or whatever other form is requested) if needed.
>>>
>>> Other than the system property (which is just one possible approach), no
>>> API or specification changes should be needed.  This version uses the
>>> property "jdk.module.isolated" defaulting to "false".
>>>
>>> [1]:
>>>
>>> diff --git
>>> a/jdk/src/java.base/share/classes/jdk/internal/loader/ClassLoaders.java
>>> b/jdk/src/java.base/share/classes/jdk/internal/loader/ClassLoaders.java
>>> index d976aab..f06adfb 100644
>>> --- 
>>> a/jdk/src/java.base/share/classes/jdk/internal/loader/ClassLoaders.java
>>> +++ 
>>> b/jdk/src/java.base/share/classes/jdk/internal/loader/ClassLoaders.java
>>> @@ -112,6 +112,15 @@ public static ClassLoader appClassLoader() {
>>>       }
>>>
>>>       /**
>>> +     * Returns a new isolated application module class loader.
>>> +     *
>>> +     * @return the new application module class loader (not {@code 
>>> null})
>>> +     */
>>> +    public static ClassLoader appModuleClassLoader() {
>>> +        return new AppModuleClassLoader(APP_LOADER);
>>> +    }
>>> +
>>> +    /**
>>>        * The class loader that is used to find resources in modules
>>> defined to
>>>        * the boot class loader. It is not used for class loading.
>>>        */
>>> @@ -220,6 +229,21 @@ protected Package defineOrCheckPackage(String pn,
>>> Manifest man, URL url) {
>>>       }
>>>
>>>       /**
>>> +     * An isolated application module class loader that is a {@code
>>> BuiltinClassLoader} with
>>> +     * the application class loader as parent.
>>> +     */
>>> +    private static class AppModuleClassLoader extends 
>>> BuiltinClassLoader {
>>> +        static {
>>> +            if (!ClassLoader.registerAsParallelCapable())
>>> +                throw new InternalError();
>>> +        }
>>> +
>>> +        AppModuleClassLoader(AppClassLoader parent) {
>>> +            super("app", parent, null);
>>> +        }
>>> +    }
>>> +
>>> +    /**
>>>        * Returns a {@code URLClassPath} of file URLs to each of the
>>> elements in
>>>        * the given class path.
>>>        */
>>> diff --git
>>> a/jdk/src/java.base/share/classes/jdk/internal/module/ModuleLoaderMap.java 
>>>
>>> b/jdk/src/java.base/share/classes/jdk/internal/module/ModuleLoaderMap.java 
>>>
>>> index b0f465d..cc2bdb6 100644
>>> ---
>>> a/jdk/src/java.base/share/classes/jdk/internal/module/ModuleLoaderMap.java 
>>>
>>> +++
>>> b/jdk/src/java.base/share/classes/jdk/internal/module/ModuleLoaderMap.java 
>>>
>>> @@ -85,6 +85,8 @@ public ClassLoader apply(String name) {
>>>           return platformModules;
>>>       }
>>>
>>> +    private static final boolean ISOLATED_MODULE_PATH =
>>> Boolean.parseBoolean(System.getProperty("jdk.module.isolated", 
>>> "false"));
>>> +
>>>       /**
>>>        * Returns the function to map modules in the given configuration
>>> to the
>>>        * built-in class loaders.
>>> @@ -102,6 +104,8 @@ public ClassLoader apply(String name) {
>>>               if (!bootModules.contains(mn)) {
>>>                   if (platformModules.contains(mn)) {
>>>                       map.put(mn, platformClassLoader);
>>> +                } else if (ISOLATED_MODULE_PATH) {
>>> +                    map.put(mn, ClassLoaders.appModuleClassLoader());
>>>                   } else {
>>>                       map.put(mn, appClassLoader);
>>>                   }
>>>
>>>
>>> -- 
>>> - DML
> 
> 


-- 
- DML