A compromise approach on module isolation

Fri May 12 20:38:39 UTC 2017

David, Remi, and all,

I have a few questions (and a concern),

- This seems like a straightforward change and I’m sure it must have been
proposed in the past. And I guess it was rejected for specific reasons.
What are the gotchas of this approach? I.e. Will it break some code? Will
it require some code to be changed? And will it bake-in some behavior that
we might not want in the long term?

- (With my JVM/GC implementer hat on) I really don’t think launching a
class loader per module is a great idea. There are overheads in the JVM for
each class loader you introduce (performance - the GC has to do some extra
work per class loader, footprint - there are data structures maintained per
class loader and space could be wasted due to fragmentation in the meta
space of each class loader). FWIW, at Twitter, most of our services do not
use custom class loaders and we get a lot of benefit out of that. It’d be a
step backwards for us if somehow our JVMs got bloated with potentially
1,000s of class loaders.

- Using class loaders is one way to isolate modules. Has anyone considered
alternatives? For example, doing automatic shading of non-exported packages
in the JVM (lots of hand-waving here)?

Tony

—————
Tony Printezis | @TonyPrintezis | tprintezis at twitter.com

On May 12, 2017 at 2:56:40 PM, David M. Lloyd (david.lloyd at redhat.com)
wrote:

It seems to work in my tests. Can you explain what you mean?

$ jlink --module-path $JAVA_HOME/jmods:out/production --add-modules
org.module.two,org.module.one --output new-test
-J-Djdk.module.detect-cycles=false -J-Djdk.module.isolated=true
--launcher one=org.module.one/org.module.one.Main --launcher
two=org.module.two/org.module.two.Main
$ new-test/bin/two
This is module two, my
cl=jdk.internal.loader.ClassLoaders$AppModuleClassLoader at 2b05039f

On 05/12/2017 01:54 PM, Remi Forax wrote:
> It doesn't work because application modules becomes system modules,
> so even if you propagate the flags, an application modules becomes a
> system module hence is not deployed in a class loader anymore.
>
> Rémi
>
>
> On May 12, 2017 6:45:18 PM GMT+02:00, "David M. Lloyd"
> <david.lloyd at redhat.com> wrote:
>
> 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
> <http://ClassLoaders.java>
> b/jdk/src/java.base/share/classes/jdk/internal/loader/ClassLoaders.java
> <http://ClassLoaders.java>
> index d976aab..f06adfb 100644
> ---
> a/jdk/src/java.base/share/classes/jdk/internal/loader/ClassLoaders.java
> <http://ClassLoaders.java>
> +++
> b/jdk/src/java.base/share/classes/jdk/internal/loader/ClassLoaders.java
> <http://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
> <http://ModuleLoaderMap.java>
>
>
b/jdk/src/java.base/share/classes/jdk/internal/module/ModuleLoaderMap.java
> <http://ModuleLoaderMap.java>
>
> index b0f465d..cc2bdb6 100644
> ---
>
a/jdk/src/java.base/share/classes/jdk/internal/module/ModuleLoaderMap.java
> <http://ModuleLoaderMap.java>
>
> +++
>
b/jdk/src/java.base/share/classes/jdk/internal/module/ModuleLoaderMap.java
> <http://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
>
>
>
>
>
> --
> Sent from my Android device with K-9 Mail. Please excuse my brevity.

-- 
- DML