I believe with the current fix, we at least do not have the "eager loading" problem anymore. The classloader isolation was a problem before and would remain as a problem after this for further larger scale changes. +1 for merging the fix.
On Tue, Apr 11, 2017 at 4:04 PM, Swapnil Bawaskar <sbawas...@pivotal.io> wrote: > +1 for merging Jared's fix. it gets us closer to fixing classloading in a > later release. > > On Tue, Apr 11, 2017 at 1:41 PM Jacob Barrett <jbarr...@pivotal.io> wrote: > > > I can support the idea of taking the small step of fixing the eager class > > loading if it doesn't: > > 1) break the current expected behavior. > > 2) dig is deeper into a hole with class loading isolation in the future. > > > > If it breaks the current expected behavior, that in itself is ok until > you > > consider that adding class loader isolation later will most definitely > > break the expected behavior, can we afford do break it twice? > > > > -Jake > > > > On Tue, Apr 11, 2017 at 10:06 AM Jared Stewart <jstew...@pivotal.io> > > wrote: > > > > > I would like to distinguish between the following two objectives: > > > 1) Do not eagerly load every class contained in a deployed jar > > > 2) Establish robust classloader isolation for deployed jars > > > > > > The aim of my current line of work is to fix 1) (GEODE-2290). This is > > not > > > to say that 2) is not a valuable pursuit, but I think getting 2) done > > > correctly is a larger task than fixing 1) in isolation. > > > > > > To get into the specifics of the libraries you mentioned, > > > > > > JCL: > > > - JCL has no support for removing/undeploying jars. In this respect, > I > > > don't see anything that JCL would add over simply using a > URLClassLoader. > > > We would have to rebuild the JCL class loader in exactly the same set > of > > > circumstances that we would need to rebuild a URLClassLoader. > > > - I have seen a fair number of warnings from people that JCL is buggy > > and > > > incomplete, e.g. ( > > > > > http://stackoverflow.com/questions/60764/how-should-i- > load-jars-dynamically-at-runtime#comment43066872_1450837 > > ) > > > This would seem to be supported by a quick look at the github issues > page > > > for JCL, which includes things like a bug report of a deadlock from Jun > > > 2016 to which the developers have never responded. > > > > > > JBoss Modules: > > > - JBoss Modules (or Java 9/Jigsaw Modules) seem like a good strategic > > > direction towards which to strive. Yet the fact that they require an > > > external module descriptor (XML) would again seem to make this a much > > > larger task than 1) alone. (If the idea here is to have a user provide > > us > > > with a JBoss Module rather than a plain jar file, this would be a large > > > breaking change for existing users. On the other hand, if the idea is > > for > > > us to autogenerate the necessary metainformation to transform the > user's > > > jar file into a JBoss Module, this would seem to be a large task which > is > > > again independent from the goals of 1). > > > > > > - Jared > > > > > > > On Apr 10, 2017, at 9:41 PM, Jacob Barrett <jbarr...@pivotal.io> > > wrote: > > > > > > > > There are certainly many projects in the OS community that have > solved > > > this > > > > same problem. Perhaps we can find a class loader from a project that > > > would > > > > suite this need. > > > > > > > > Quick search of standalone frameworks comes up with a few popular > hits: > > > > https://github.com/kamranzafar/JCL > > > > https://github.com/jboss-modules/jboss-modules > > > > > > > > -Jake > > > > > > > > > > > > > > > > > > > > On Mon, Apr 10, 2017 at 1:40 PM Kirk Lund <kl...@apache.org> wrote: > > > > > > > >> I think Jared started down this path because we had a custom > > classloader > > > >> implementation that he was trying to get rid of -- that impl was > > pretty > > > >> limited and forced loading of all classes up-front. > > > >> > > > >> Now the code uses fast classpath scanner and that old custom > > classloader > > > >> can't be used. The old classloader is so simplistic and limited that > > > trying > > > >> to modify it looks like more work than writing a new one from > scratch. > > > >> Implementing a new custom classloader or switching our codebase to > > use a > > > >> new classloader container (using an existing solution) are both > > possible > > > >> directions. Until that's completed the "deploy jar" command will > > > continue > > > >> to force ALL classes to be loaded up-front. > > > >> > > > >> One major concern is that implementing a new custom classloader is > > > >> non-trivial and could also introduce some new classloader bugs -- of > > > >> particular interest to "deploy jar" is the fact that Java remembers > > TWO > > > >> classloaders for each class [1] -- the CL that *loaded* the class > AND > > > the > > > >> CL that *initiated* the request to load the class -- dropping the > > > >> *initiating* CL at runtime can result in failures to load additional > > > >> classes from the CL that directly loaded the class even though that > CL > > > is > > > >> intact and available. > > > >> > > > >> [1] states: "When one class loader delegates to another class > loader, > > > the > > > >> loader that initiates the loading is not necessarily the same loader > > > that > > > >> completes the loading and defines the class. If L creates C, either > by > > > >> defining it directly or by delegation, we say that L initiates > loading > > > of C > > > >> or, equivalently, that L is an *initiating* loader of C." > > > >> > > > >> For better or worse, this is one of the reasons why that old custom > CL > > > was > > > >> naively force loading all classes up-front -- ie, to avoid runtime > > > >> classloading failures if the initiating CL was dropped or replaced > and > > > >> ultimately GC'ed. Java won't let you drop the *loading* CL but it > will > > > >> allow you to drop the *initiating* CL (or it did historically -- the > > > >> reference seems to be down in native code, not in java.lang.Class). > > > You'd > > > >> have to find some way to force all initiating requests up to the > > parent > > > >> application CL (or somehow prevent code in deployed jars from > > initiating > > > >> requests from other CLs) and maybe that's what this old custom > > > classloader > > > >> was missing all along. > > > >> > > > >> The tradeoff mentioned by Jared is only necessary if we want a > release > > > >> (soon) that does NOT eagerly class load all deployed classes > up-front. > > > >> Otherwise, this is a feature request that users might have to wait a > > > little > > > >> longer for (and maybe that's ok!). > > > >> > > > >> [1] > > > >> > > https://docs.oracle.com/javase/specs/jvms/se7/html/jvms-5.html#jvms-5.3 > > > >> > > > >> On Mon, Apr 10, 2017 at 10:30 AM, Anthony Baker <aba...@pivotal.io> > > > wrote: > > > >> > > > >>> What about this: > > > >>> > > > >>> 1) Each jar is deployed into it’s own classloader. > > > >>> 2) If the classloader for jar1 is unable to load a class, it > > delegates > > > to > > > >>> its parent which can load classes from siblings to jar1. > > > >>> > > > >>> The classloader hierarchy would be: > > > >>> > > > >>> bootstrap << system << application << (deploy jar)* > > > >>> > > > >>> where the application classloader manages the delegation to all > > > deployed > > > >>> jars. > > > >>> > > > >>> Anthony > > > >>> > > > >>> > > > >>>> On Apr 10, 2017, at 10:20 AM, Jared Stewart <jstew...@pivotal.io> > > > >> wrote: > > > >>>> > > > >>>> There is last one implementation decision for GEODE-2290 that I'm > > torn > > > >>> about, namely having one classloader for all deployed jars vs > having > > > >>> separate classloaders for each deployed jar. > > > >>>> > > > >>>> If we have one class loader, e.g. new UrlClassLoader(jar1, jar2), > > > then: > > > >>>> > > > >>>> - Jar1 will be able to load classes/resources from jar2 (this was > > the > > > >>> previous behavior of deployed jars with our custom class loader) > > > >>>> - But if we redeploy jar 1, jar 2 will also get its class loader > > > >>> rebuilt, which may raise the likelihood of weird classloader > problems > > > >>> arising > > > >>>> > > > >>>> Does anyone else have thoughts about this tradeoff? > > > >>>> > > > >>>> Thanks, > > > >>>> Jared > > > >>> > > > >>> > > > >> > > > > > > > > > -- Cheers Jinmei
