Marker = key in a hasmap or sthg like it. The point is we use kind of fake loader delegating to real loaders. So hashCode is effectively equals but equals shouldnt return true from a strict perspective but if it doesnt do it we are not able to use cdi correctly (excepted luck or particular case) and a lot of ee libs are broken
To try to do a test I think you need to execute both from a webapp and from a jaxws request. But when the bug was reported it was very hard to find and it was in a complicated case so not sure that's trivial to do, we have good fallbacks for simple cases Le 4 déc. 2013 03:03, "Thiago Veronezi" <[email protected]> a écrit : > 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 > > > > >> >> >> > > > > >> >> >> > > > > >> >> >> > > > > >> >> > > > > >> > > > > > > > > > >
