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_
