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?
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_
