A compromise approach on module isolation

[David M. Lloyd]

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