I have submitted a review board request for this change, comments and feedback are welcome: https://reviews.apache.org/r/58393/ <https://reviews.apache.org/r/58393/>
I don’t believe it breaks the current expected behavior, and I don’t think it will pose any additional barriers to classloader isolation in the future (the fast-classpath-scanner library is agnostic about the class loader implementation you give to it). Anthony - I was never able to find any substantial documentation for Plexus Classworlds. This makes it both hard to evaluate and potentially scary to use. - Jared > On Apr 12, 2017, at 8:55 AM, Michael Stolz <mst...@pivotal.io> wrote: > > +1 for merging the current fix. > > It at least makes it easier to deploy a jar in the happy day scenario. > I'm not sure how much we should be trying to emulate an application server > anyway. > If people really want to combine the cache with the app server they can > embed Geode into an app server. > There may be some API sugar that would make that more useful, but it seems > it's a lot less work than all this classloader stuff we're discussing, and > would likely yield much better sandboxing. > > -- > Mike Stolz > Principal Engineer, GemFire Product Manager > Mobile: +1-631-835-4771 > > On Tue, Apr 11, 2017 at 7:46 PM, Jinmei Liao <jil...@pivotal.io> wrote: > >> 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 >>
