Hmmmm, ok. I guess I start to understand what the problem is. If I'm right,
we already have a buggy feature. Bear with me.
None of the parents of LazyStopWebappClassLoader implements "hashCode",
therefore our current "LazyStopWebappClassLoader#equals" implementation (at
the least the one just before my last commit) is based on the result of
"Object.hashCode()". According the the "java api", "Object#hashCode" is
typically implemented by converting the internal address of the object into
an integer.
Furthermore, the general contract of hashCode is (also java api):
* Whenever it is invoked on the same object more than once during an
execution of a Java application, the hashCode method must consistently
return the same integer, provided no information used in equals comparisons
on the object is modified. This integer need not remain consistent from one
execution of an application to another execution of the same application.
* If two objects are equal according to the equals(Object) method, then
calling the hashCode method on each of the two objects must produce the
same integer result.
* It is not required that if two objects are unequal according to the
equals(java.lang.Object) method, then calling the hashCode method on each
of the two objects must produce distinct integer results. However, the
programmer should be aware that producing distinct integer results for
unequal objects may improve the performance of hash tables.
So, in our case...
LazyStopWebappClassLoader.java
******************************************************
public boolean equals(final Object other) {
return other != null && ClassLoader.class.isInstance(other) &&
hashCode() == other.hashCode();
}
public int hashCode() {
return super.hashCode();
}
******************************************************
If this.hashCode() is equal to other.hashCode(), that means that "this ==
other". In other words, "this" is the same instance (same memory address)
represented by "other". In this case we have the same result as we would
have by using the "Object" implementation of both methods.
Object.class
******************************************************
public boolean equals(Object obj) {
return (this == obj);
}
public native int hashCode();
******************************************************
Why I think we have a bug? In both implementations, we would never have two
equal classloader objects if both aren't exactly the same instance because
in both implementations we use "Object.hashCode" to compare "this" and
"other". Am I right?
>>we and libs like deltaspike use classloader as marker
I didn't get what you mean by "marker". How does it work?
Anyway, the build was successful and all my own applications still run
perfectly. Can you give me some direction on how to create a unit test that
validates it?
[]s,
Thiago.
On Tue, Dec 3, 2013 at 6:48 PM, Romain Manni-Bucau <[email protected]>wrote:
> The more obvious issue comes from the fact we and libs like deltaspike use
> classloader as marker so if both are not equals when needed it doesnt work
> (we dont find cdi context ror instance)
> Le 3 déc. 2013 23:34, "Thiago Veronezi" <[email protected]> a écrit :
>
> > >>If you have time to add tests it would be great
> > I will test it a little bit further.
> >
> > But I didn't quite get the problem yet... sorry! :)
> >
> >
> > This is our implementation (without the cache)...
> >
> >
> *****************************************************************************************************
> > public boolean equals(final Object other) {
> > return other != null && ClassLoader.class.isInstance(other) &&
> > hashCode() == other.hashCode();
> > }
> >
> > public int hashCode() {
> > return super.hashCode();
> > }
> >
> >
> *****************************************************************************************************
> >
> > This is the default implementation...
> >
> >
> *****************************************************************************************************
> > public boolean equals(Object obj) {
> > return (this == obj);
> > }
> >
> > public native int hashCode();
> >
> >
> *****************************************************************************************************
> >
> > The default method will return the same integer if "this == other"; and
> we
> > are basically comparing the result of both hashCode() calls. Isn't it the
> > same thing?
> >
> > []s,
> > Thiago.
> >
> >
> >
> >
> >
> > On Tue, Dec 3, 2013 at 5:11 PM, Romain Manni-Bucau <
> [email protected]
> > >wrote:
> >
> > > well, the issue is without doing it the classloader used for cxf
> > > (
> > >
> >
> http://svn.apache.org/repos/asf/tomee/tomee/trunk/server/openejb-cxf-transport/src/main/java/org/apache/openejb/server/cxf/transport/util/CxfContainerClassLoader.java
> > > )
> > > and the webapp loader are different. I'm 100% sure that's a regression
> > > even if build passes and we can't keep it.
> > >
> > > This makes equals not symmetric so using JVM equals/hashCode is the
> > > best we can do.
> > >
> > > If you have time to add tests it would be great, the issue can come
> > > from client/server and jaxws/jaxrs. It can cause linkagerror,
> > > classcast or even prevent deployment.
> > > Romain Manni-Bucau
> > > Twitter: @rmannibucau
> > > Blog: http://rmannibucau.wordpress.com/
> > > LinkedIn: http://fr.linkedin.com/in/rmannibucau
> > > Github: https://github.com/rmannibucau
> > >
> > >
> > >
> > > 2013/12/3 Thiago Veronezi <[email protected]>:
> > > > It didn't locally. It takes ~2 hrs to have an exception or not.
> > > > It should happen in the next 1:15 hrs. If anything happens we can
> > always
> > > > revert it back. :)
> > > >
> > > >
> > > >
> > > >
> > > >
> > > > On Tue, Dec 3, 2013 at 4:19 PM, Romain Manni-Bucau <
> > > [email protected]>wrote:
> > > >
> > > >> it does for sure, not sure we have tests btw
> > > >> Romain Manni-Bucau
> > > >> Twitter: @rmannibucau
> > > >> Blog: http://rmannibucau.wordpress.com/
> > > >> LinkedIn: http://fr.linkedin.com/in/rmannibucau
> > > >> Github: https://github.com/rmannibucau
> > > >>
> > > >>
> > > >>
> > > >> 2013/12/3 Thiago Veronezi <[email protected]>:
> > > >> > I tested it locally without the methods.
> > > >> > Committed the code to see if the build server complains.
> > > >> >
> > > >> > []s,
> > > >> > Thiago.
> > > >> >
> > > >> >
> > > >> >
> > > >> > On Tue, Dec 3, 2013 at 1:11 PM, Romain Manni-Bucau <
> > > >> [email protected]>wrote:
> > > >> >
> > > >> >> hashcode is just a cached impl, equals is mandatory because of
> the
> > > >> >> hack we have to isolate a bit cxf from apps.
> > > >> >>
> > > >> >> If you fix isolation you can remove it ;).
> > > >> >> Romain Manni-Bucau
> > > >> >> Twitter: @rmannibucau
> > > >> >> Blog: http://rmannibucau.wordpress.com/
> > > >> >> LinkedIn: http://fr.linkedin.com/in/rmannibucau
> > > >> >> Github: https://github.com/rmannibucau
> > > >> >>
> > > >> >>
> > > >> >>
> > > >> >> 2013/12/3 Thiago Veronezi <[email protected]>:
> > > >> >> > The equals and hashCode look weird.
> > > >> >> >
> > > >> >> > ****************************************
> > > >> >> > @Override
> > > >> >> > public boolean equals(final Object other) {
> > > >> >> > return other != null &&
> > > ClassLoader.class.isInstance(other) &&
> > > >> >> > hashCode() == other.hashCode();
> > > >> >> > }
> > > >> >> >
> > > >> >> > @Override
> > > >> >> > public int hashCode() {
> > > >> >> > return hashCode;
> > > >> >> > }
> > > >> >> > ****************************************
> > > >> >> >
> > > >> >> > I wonder if we need these methods at all. Usually equal
> hashCodes
> > > does
> > > >> >> not
> > > >> >> > mean that "equals" is true, right?
> > > >> >> >
> > > >> >> > []s,
> > > >> >> > Thiago.
> > > >> >> >
> > > >> >> >
> > > >> >> >
> > > >> >> >
> > > >> >> > On Tue, Dec 3, 2013 at 12:57 PM, <[email protected]>
> wrote:
> > > >> >> >
> > > >> >> >> Author: rmannibucau
> > > >> >> >> Date: Tue Dec 3 17:57:19 2013
> > > >> >> >> New Revision: 1547499
> > > >> >> >>
> > > >> >> >> URL: http://svn.apache.org/r1547499
> > > >> >> >> Log:
> > > >> >> >> correct hashcode in lazywebappclassloader
> > > >> >> >>
> > > >> >> >> Modified:
> > > >> >> >>
> > > >> >> >>
> > > >> >>
> > > >>
> > >
> >
> tomee/tomee/trunk/tomee/tomee-catalina/src/main/java/org/apache/tomee/catalina/LazyStopWebappClassLoader.java
> > > >> >> >>
> > > >> >> >> Modified:
> > > >> >> >>
> > > >> >>
> > > >>
> > >
> >
> tomee/tomee/trunk/tomee/tomee-catalina/src/main/java/org/apache/tomee/catalina/LazyStopWebappClassLoader.java
> > > >> >> >> URL:
> > > >> >> >>
> > > >> >>
> > > >>
> > >
> >
> http://svn.apache.org/viewvc/tomee/tomee/trunk/tomee/tomee-catalina/src/main/java/org/apache/tomee/catalina/LazyStopWebappClassLoader.java?rev=1547499&r1=1547498&r2=1547499&view=diff
> > > >> >> >>
> > > >> >> >>
> > > >> >>
> > > >>
> > >
> >
> ==============================================================================
> > > >> >> >> ---
> > > >> >> >>
> > > >> >>
> > > >>
> > >
> >
> tomee/tomee/trunk/tomee/tomee-catalina/src/main/java/org/apache/tomee/catalina/LazyStopWebappClassLoader.java
> > > >> >> >> (original)
> > > >> >> >> +++
> > > >> >> >>
> > > >> >>
> > > >>
> > >
> >
> tomee/tomee/trunk/tomee/tomee-catalina/src/main/java/org/apache/tomee/catalina/LazyStopWebappClassLoader.java
> > > >> >> >> Tue Dec 3 17:57:19 2013
> > > >> >> >> @@ -41,19 +41,21 @@ public class LazyStopWebappClassLoader e
> > > >> >> >> private boolean restarting = false;
> > > >> >> >> private boolean forceStopPhase =
> > > >> >> >>
> > > >> >>
> > > >>
> > >
> >
> Boolean.parseBoolean(SystemInstance.get().getProperty("tomee.webappclassloader.force-stop-phase",
> > > >> >> >> "false"));
> > > >> >> >> private ClassLoaderConfigurer configurer = null;
> > > >> >> >> + private final int hashCode;
> > > >> >> >>
> > > >> >> >> public LazyStopWebappClassLoader() {
> > > >> >> >> - construct();
> > > >> >> >> + hashCode = construct();
> > > >> >> >> }
> > > >> >> >>
> > > >> >> >> public LazyStopWebappClassLoader(final ClassLoader
> parent)
> > {
> > > >> >> >> super(parent);
> > > >> >> >> - construct();
> > > >> >> >> + hashCode = construct();
> > > >> >> >> }
> > > >> >> >>
> > > >> >> >> - private void construct() {
> > > >> >> >> + private int construct() {
> > > >> >> >> setDelegate(isDelegate());
> > > >> >> >> configurer = INIT_CONFIGURER.get();
> > > >> >> >> + return super.hashCode();
> > > >> >> >> }
> > > >> >> >>
> > > >> >> >> @Override
> > > >> >> >> @@ -211,12 +213,8 @@ public class LazyStopWebappClassLoader e
> > > >> >> >> }
> > > >> >> >>
> > > >> >> >> @Override
> > > >> >> >> - public int hashCode() { // could be improved a bit adding
> > the
> > > >> host
> > > >> >> >> and ensuring contextName != null, an alternative is getURLs()
> > but
> > > it
> > > >> is
> > > >> >> >> longer
> > > >> >> >> - final String name = getContextName();
> > > >> >> >> - if (name != null) {
> > > >> >> >> - return name.hashCode();
> > > >> >> >> - }
> > > >> >> >> - return super.hashCode();
> > > >> >> >> + public int hashCode() {
> > > >> >> >> + return hashCode;
> > > >> >> >> }
> > > >> >> >>
> > > >> >> >> @Override
> > > >> >> >>
> > > >> >> >>
> > > >> >> >>
> > > >> >>
> > > >>
> > >
> >
>
