Hi Daniel, Cause there are some commands using a webapp like classloader and it would fail in this mode in general - we have the same hack for tomee embedded i think, just not triggered the same way. Feel free to revisit it and unify it with what you are doing. Likely what was planned: https://github.com/apache/tomee/blob/master/container/openejb-loader/src/main/java/org/apache/openejb/loader/SystemClassPath.java#L33 and https://github.com/apache/tomee/blob/master/container/openejb-loader/src/main/java/org/apache/openejb/loader/SystemClassPath.java#L66. This is why i mentionned you shouldnt modify the classloader by default at the beginning of the review and just reuse the system classpath. The intention was to make this custom loader behave the same as the child first one but guess it was never aligned.
Romain Manni-Bucau @rmannibucau <https://twitter.com/rmannibucau> | Blog <https://rmannibucau.metawerx.net/> | Old Blog <http://rmannibucau.wordpress.com> | Github <https://github.com/rmannibucau> | LinkedIn <https://www.linkedin.com/in/rmannibucau> | Book <https://www.packtpub.com/application-development/java-ee-8-high-performance> Le jeu. 22 nov. 2018 à 14:24, Daniel Cunha <daniels...@gmail.com> a écrit : > Hi Romain, > > I get your point now... > Question: Why do we need to do it[1]? > > if (loader != null) { > Thread.currentThread().setContextClassLoader(loader); > if (loader != ClassLoader.getSystemClassLoader()) { > > System.setProperty("openejb.classloader.first.disallow-system-loading", > "true"); > } > } > > > Do we really need to change it? Why? > I believe which the CLI has you owner classpath and we don't need to change > the default on to execute our commands. Just try understand why we are > doing it. > > [1] > > https://github.com/apache/tomee/blob/master/container/openejb-core/src/main/java/org/apache/openejb/cli/Bootstrap.java#L151-L156 > > Em ter, 20 de nov de 2018 às 17:32, Daniel Cunha <daniels...@gmail.com> > escreveu: > > > Hi Romain, > > > > Ok, I'll create a scenario for it. > > > > Thank you. > > > > Em ter, 20 de nov de 2018 às 17:29, Romain Manni-Bucau < > > rmannibu...@gmail.com> escreveu: > > > >> Hi Daniel > >> > >> Do you think you can assert the classloader in the execution thread > didnt > >> change after the call? Was one concern and i didnt find the matching > >> assert > >> > >> Le mar. 20 nov. 2018 20:00, Daniel Cunha <daniels...@apache.org> a > écrit > >> : > >> > >> > Hi folks, > >> > > >> > I've added a new test with JAXRS, calling a command in my endpoint > from > >> > Bootstrap CLI. > >> > Please review the changes on my PR[1] and let me know what do you > think > >> > about it. > >> > > >> > Thank you. :) > >> > > >> > [1] https://github.com/apache/tomee/pull/176 > >> > > >> > Em seg, 19 de nov de 2018 às 13:57, Daniel Cunha < > daniels...@gmail.com> > >> > escreveu: > >> > > >> > > Hi Jon, > >> > > > >> > > ok. I got it! :) > >> > > > >> > > Em seg, 19 de nov de 2018 às 13:49, Jonathan Gallimore < > >> > > jonathan.gallim...@gmail.com> escreveu: > >> > > > >> > >> On Mon, Nov 19, 2018 at 4:30 PM Daniel Cunha <daniels...@gmail.com > > > >> > >> wrote: > >> > >> > >> > >> > Hi guys, > >> > >> > > >> > >> > I updated the PR with some chages: > >> > >> > 1 - Move BasicURLClassPath.CustomizableURLClassLoader to be > public. > >> > >> > 2 - I'm using BasicURLClassPath.CustomizableURLClassLoader > instead > >> of > >> > >> > DynamicClassLoader (class that I created) > >> > >> > 3- DynamicClassLoader is not needed anymore. Was removed from the > >> PR. > >> > >> > 3 - Added more tests: > >> > >> > - Standalone client that has its own main and delegates to > the > >> CLI > >> > >> main > >> > >> > - As above, but perhaps with the standalone client creating a > >> > >> > classloader > >> > >> > with the CLI jars and then launching it > >> > >> > > >> > >> > Question: > >> > >> > > >> > >> > @Jon, > >> > >> > what do you want me mean with: A webapp calling commands in the > >> CLI? > >> > >> > > >> > >> > >> > >> Hopefully I can answer that one in a straightforward way :). Try > >> > creating > >> > >> a > >> > >> servlet that loads the Bootstrap CLI class, and invokes some > methods. > >> > >> Effectively you'd be calling the TomEE command line commands from a > >> > >> servlet. That use case hadn't occurred to me, but it sounds like > one > >> > worth > >> > >> trying. > >> > >> > >> > >> That would probably need to sit > >> > >> under > >> > arquillian/arquillian-tomee-tests/arquillian-tomee-webprofile-tests. > >> > >> > >> > >> Jon > >> > >> > >> > >> > >> > >> > > >> > >> > Command is a class with main method, if I've it deployed in my > >> library > >> > >> > folder, it should works fine, I just need to create a instance > and > >> > call > >> > >> the > >> > >> > main. > >> > >> > Same will be if I try execute it with Process (that is exactly > >> what we > >> > >> are > >> > >> > testing). > >> > >> > > >> > >> > Em sex, 2 de nov de 2018 às 07:53, Jonathan Gallimore < > >> > >> > jonathan.gallim...@gmail.com> escreveu: > >> > >> > > >> > >> > > I'm following this thread with some interest. What would be > some > >> > good > >> > >> > tests > >> > >> > > to make sure these cases are covered off? Some suggestions in > >> > >> addition to > >> > >> > > the existing test: > >> > >> > > > >> > >> > > * Standalone client that has its own main and delegates to the > >> CLI > >> > >> main > >> > >> > > * As above, but perhaps with the standalone client creating a > >> > >> classloader > >> > >> > > with the CLI jars and then launching it > >> > >> > > * A webapp calling commands in the CLI > >> > >> > > > >> > >> > > Any others? These are interesting use cases that I hadn't > >> > considered - > >> > >> > this > >> > >> > > is interesting. > >> > >> > > > >> > >> > > Jon > >> > >> > > > >> > >> > > > >> > >> > > > >> > >> > > On Wed, Oct 31, 2018 at 2:30 PM Romain Manni-Bucau < > >> > >> > rmannibu...@gmail.com> > >> > >> > > wrote: > >> > >> > > > >> > >> > > > Le mer. 31 oct. 2018 à 14:52, Daniel Cunha < > >> daniels...@gmail.com> > >> > a > >> > >> > > écrit > >> > >> > > > : > >> > >> > > > > >> > >> > > > > Em seg, 29 de out de 2018 às 14:01, Jonathan Gallimore < > >> > >> > > > > jonathan.gallim...@gmail.com> escreveu: > >> > >> > > > > > >> > >> > > > > > We should write a test case for both of these, and agree > on > >> > >> them. > >> > >> > I'd > >> > >> > > > > like > >> > >> > > > > > to be able to merge this in with confidence, and without > >> the > >> > >> tests > >> > >> > I > >> > >> > > > > don't > >> > >> > > > > > see how we can do that. > >> > >> > > > > > > >> > >> > > > > > Jon > >> > >> > > > > > > >> > >> > > > > > On Mon, Oct 29, 2018 at 3:35 PM Romain Manni-Bucau < > >> > >> > > > > rmannibu...@gmail.com> > >> > >> > > > > > wrote: > >> > >> > > > > > > >> > >> > > > > > > Hi Daniel, > >> > >> > > > > > > > >> > >> > > > > > > the point is that this code is reused in several places > >> so > >> > we > >> > >> > must > >> > >> > > be > >> > >> > > > > as > >> > >> > > > > > > clean as we can, here the cases I had in mind: > >> > >> > > > > > > > >> > >> > > > > > > 1. reused main in another main (so an app calls the > >> main): > >> > >> here > >> > >> > the > >> > >> > > > > > pitfall > >> > >> > > > > > > is that the property > >> > >> > openejb.classloader.first.disallow-system-load > >> > >> > > > is > >> > >> > > > > > not > >> > >> > > > > > > resetted and the TCLL neither so after the main you > use a > >> > >> closed > >> > >> > > > > > > classloader (so will likely easily fail) > >> > >> > > > > > > >> > >> > > > > > >> > >> > > > > I really didn't get your point. Side note we are create a > >> > >> classloader > >> > >> > > > only > >> > >> > > > > for tomee cli execution, which keep alive only for a short > >> time > >> > >> > during > >> > >> > > > > command execution. > >> > >> > > > > > >> > >> > > > > That part of code is not reused in another place, if yes, > >> just > >> > >> point > >> > >> > me > >> > >> > > > > where, because I can't find it. > >> > >> > > > > > >> > >> > > > > It will close only after the program finish, since it is > >> inside > >> > a > >> > >> > > > try-catch > >> > >> > > > > block, no? > >> > >> > > > > > >> > >> > > > > >> > >> > > > Assuming the main is launch but this main is sometimes > launched > >> > >> > > > programmatically (like in a webapp) > >> > >> > > > When not you are right > >> > >> > > > > >> > >> > > > > >> > >> > > > > > >> > >> > > > > > >> > >> > > > > > > 2. if a user relies in java 8 on the classloader being > >> the > >> > >> system > >> > >> > > one > >> > >> > > > > > then > >> > >> > > > > > > your new classloader level breaks it so we shouldnt > >> create > >> > >> this > >> > >> > new > >> > >> > > > > > > classloader when not needed and the old ClassPath impl > is > >> > >> working > >> > >> > > > IMO. > >> > >> > > > > We > >> > >> > > > > > > can enrich it with a warn log saying "it will fail in > >> next > >> > >> > version" > >> > >> > > > > (and > >> > >> > > > > > we > >> > >> > > > > > > drop that code after 1 or 2 releases). Idea is to not > >> break > >> > >> > > directly > >> > >> > > > > > users > >> > >> > > > > > > who can not even know some of the libs they added in > the > >> > >> > classpath > >> > >> > > > for > >> > >> > > > > > > logging purposes rely on that classloader setup - > random > >> > >> example > >> > >> > > > indeed > >> > >> > > > > > ;). > >> > >> > > > > > > > >> > >> > > > > > > >> > >> > > > > > >> > >> > > > > Yeah, I probably could use the SystemClassPath, but it is > >> doing > >> > >> much > >> > >> > > more > >> > >> > > > > than I need to CLI. I don't think that I need to change the > >> > >> > > > java.class.path > >> > >> > > > > property to make it works or other property like > >> > >> > > > > openejb.classloader.first.disallow-system-load. > >> > >> > > > > > >> > >> > > > > >> > >> > > > We should likely be cleaner here but not your PR, fair enough > >> > >> > > > > >> > >> > > > > >> > >> > > > > > >> > >> > > > > And to be honest, I didn't understand what is > >> > >> > > > > openejb.classloader.first.disallow-system-load and why do > we > >> > have > >> > >> it. > >> > >> > > :) > >> > >> > > > > > >> > >> > > > > >> > >> > > > It is used in the "webapp" loader to check if we should load > >> some > >> > >> well > >> > >> > > > known classes from the system classloader or not. > >> > >> > > > It is likely the kind of property you rely on when you want > to > >> be > >> > >> > > > embeddable but also standalone. > >> > >> > > > > >> > >> > > > > >> > >> > > > > > >> > >> > > > > > >> > >> > > > > > > Romain Manni-Bucau > >> > >> > > > > > > @rmannibucau <https://twitter.com/rmannibucau> | Blog > >> > >> > > > > > > <https://rmannibucau.metawerx.net/> | Old Blog > >> > >> > > > > > > <http://rmannibucau.wordpress.com> | Github < > >> > >> > > > > > > https://github.com/rmannibucau> | > >> > >> > > > > > > LinkedIn <https://www.linkedin.com/in/rmannibucau> | > >> Book > >> > >> > > > > > > < > >> > >> > > > > > > > >> > >> > > > > > > >> > >> > > > > > >> > >> > > > > >> > >> > > > >> > >> > > >> > >> > >> > > >> > https://www.packtpub.com/application-development/java-ee-8-high-performance > >> > >> > > > > > > > > >> > >> > > > > > > > >> > >> > > > > > > > >> > >> > > > > > > Le lun. 29 oct. 2018 à 16:14, Daniel Cunha < > >> > >> > daniels...@apache.org> > >> > >> > > a > >> > >> > > > > > > écrit : > >> > >> > > > > > > > >> > >> > > > > > > > Hi, > >> > >> > > > > > > > > >> > >> > > > > > > > I updated the PR with tests! > >> > >> > > > > > > > > >> > >> > > > > > > > Em seg, 29 de out de 2018 às 08:27, Jonathan > Gallimore > >> < > >> > >> > > > > > > > jonathan.gallim...@gmail.com> escreveu: > >> > >> > > > > > > > > >> > >> > > > > > > > > I saw your note on the PR regarding Romain's > >> feedback. > >> > >> > > > > > > > > > >> > >> > > > > > > > > For visibility here, Romain is querying whether > this > >> > patch > >> > >> > will > >> > >> > > > > cause > >> > >> > > > > > > an > >> > >> > > > > > > > > issue with folks calling the CLI with a custom > >> classpath > >> > >> > using > >> > >> > > > > `java > >> > >> > > > > > > -cp > >> > >> > > > > > > > > ...` under Java 8. I'd suggest we add a test for > it, > >> > which > >> > >> > > looks > >> > >> > > > to > >> > >> > > > > > be > >> > >> > > > > > > > what > >> > >> > > > > > > > > Daniel is doing. > >> > >> > > > > > > > > > >> > >> > > > > > > > > Thanks guys. > >> > >> > > > > > > > > > >> > >> > > > > > > > > Jon > >> > >> > > > > > > > > > >> > >> > > > > > > > > On Thu, Oct 25, 2018 at 12:35 PM Daniel Cunha < > >> > >> > > > > daniels...@apache.org > >> > >> > > > > > > > >> > >> > > > > > > > > wrote: > >> > >> > > > > > > > > > >> > >> > > > > > > > > > Hi Folks, > >> > >> > > > > > > > > > > >> > >> > > > > > > > > > I sent a PR on TomEE to fix the TOMEE-2253. I > got a > >> > lot > >> > >> of > >> > >> > > > > feedback > >> > >> > > > > > > > from > >> > >> > > > > > > > > > Romain (thank you Romain for all comments and > help) > >> > >> > > > > > > > > > > >> > >> > > > > > > > > > I believe which we are in good shape to merge. > >> > >> > > > > > > > > > The changes cover from Java 8 till Java 11 which > >> not > >> > >> > provide > >> > >> > > > any > >> > >> > > > > > > impact > >> > >> > > > > > > > > for > >> > >> > > > > > > > > > TomEE or application deployed on it. > >> > >> > > > > > > > > > > >> > >> > > > > > > > > > Please, look at the PR and let me know what you > >> think. > >> > >> > > > > > > > > > > >> > >> > > > > > > > > > PR: https://github.com/apache/tomee/pull/176 > >> > >> > > > > > > > > > > >> > >> > > > > > > > > > -- > >> > >> > > > > > > > > > Daniel "soro" Cunha > >> > >> > > > > > > > > > https://twitter.com/dvlc_ > >> > >> > > > > > > > > > > >> > >> > > > > > > > > > >> > >> > > > > > > > > >> > >> > > > > > > > > >> > >> > > > > > > > -- > >> > >> > > > > > > > Daniel "soro" Cunha > >> > >> > > > > > > > https://twitter.com/dvlc_ > >> > >> > > > > > > > > >> > >> > > > > > > > >> > >> > > > > > > >> > >> > > > > > >> > >> > > > > Thank you! :) > >> > >> > > > > -- > >> > >> > > > > Daniel "soro" Cunha > >> > >> > > > > https://twitter.com/dvlc_ > >> > >> > > > > > >> > >> > > > > >> > >> > > > >> > >> > > >> > >> > > >> > >> > -- > >> > >> > Daniel "soro" Cunha > >> > >> > https://twitter.com/dvlc_ > >> > >> > > >> > >> > >> > > > >> > > > >> > > -- > >> > > Daniel "soro" Cunha > >> > > https://twitter.com/dvlc_ > >> > > > >> > > >> > > >> > -- > >> > Daniel "soro" Cunha > >> > https://twitter.com/dvlc_ > >> > > >> > > > > > > -- > > Daniel "soro" Cunha > > https://twitter.com/dvlc_ > > > > > -- > Daniel "soro" Cunha > https://twitter.com/dvlc_ >
