FarmersWrap opened a new pull request, #54: URL: https://github.com/apache/openwebbeans/pull/54
# Background I found the flaky behavior in org.apache.webbeans.test.specalization.observer.prot.ProtectedObserverTest#testObserverMethodsInParentOfSpecializedBeans with an open-source research tool [NonDex](https://github.com/TestingResearchIllinois/NonDex), which will shuffle implementation-dependent operations. The test is as below. ``` @Test public void testEquals2AnnotationsUnorderedParam() { BeanCacheKey a = new BeanCacheKey(true, String.class, null, it -> null, a12); BeanCacheKey b = new BeanCacheKey(true, String.class, null, it -> null, a21); Assert.assertEquals(a, b); Assert.assertEquals(a.hashCode(), b.hashCode()); } ``` In BeanCacheKey(), it sorts the annotation array using AnnotationComparator ``` public final class BeanCacheKey { private static final Comparator<Annotation> ANNOTATION_COMPARATOR = new AnnotationComparator(); private final boolean isDelegate; private final Type type; private final String path; private final Annotation qualifier; private final Annotation[] qualifiers; private final int hashCode; private volatile LazyAnnotatedTypes lazyAnnotatedTypes; // only needed for the "main" key private final Function<Class<?>, AnnotatedType<?>> lazyAtLoader; public BeanCacheKey(boolean isDelegate, Type type, String path, Function<Class<?>, AnnotatedType<?>> lazyAtLoader, Annotation... qualifiers) { this.isDelegate = isDelegate; this.type = type; this.path = path; this.lazyAtLoader = lazyAtLoader; int length = qualifiers != null ? qualifiers.length : 0; if (length == 0) { qualifier = null; this.qualifiers = null; } else if (length == 1) { qualifier = qualifiers[0]; this.qualifiers = null; } else { qualifier = null; // to save array creations, we only create an array, if we have more than one annotation this.qualifiers = new Annotation[length]; System.arraycopy(qualifiers, 0, this.qualifiers, 0, length); Arrays.sort(this.qualifiers, ANNOTATION_COMPARATOR); } // this class is directly used in ConcurrentHashMap.get() so simply init the hasCode here hashCode = computeHashCode(); } ``` AnnotationComparator uses getDeclaredMethods() to get a Method[], and the comparator will compare the Method[] using its elements. We want member1 and member2 to compare the same type of element during the comparison. We do not want to use a "name" to compare with a "number". So, we sorted the array to get it fixed. The Method[]. will only be used in this comparator. ``` private static class AnnotationComparator implements Comparator<Annotation> { // Notice: Sorting is a bit costly, but the use of this code is very rar. @Override public int compare(Annotation annotation1, Annotation annotation2) { Class<? extends Annotation> type1 = annotation1.annotationType(); Class<? extends Annotation> type2 = annotation2.annotationType(); int temp = type1.getName().compareTo(type2.getName()); if (temp != 0) { return temp; } if (annotation1 instanceof EmptyAnnotationLiteral || annotation2 instanceof EmptyAnnotationLiteral) { // if any of those 2 annotations are known to have no members // then we can immediately return as we know the 2 annotations mean the same return 0; } Method[] member1 = type1.getDeclaredMethods(); Arrays.sort(member1, (m1, m2) -> m1.getName().compareTo(m2.getName())); Method[] member2 = type2.getDeclaredMethods(); Arrays.sort(member2, (m1, m2) -> m1.getName().compareTo(m2.getName())); // TBD: the order of the list of members seems to be deterministic int i = 0; int j = 0; int length1 = member1.length; int length2 = member2.length; // find next nonbinding for (;; i++, j++) { while (i < length1 && member1[i].isAnnotationPresent(Nonbinding.class)) { i++; } while (j < length2 && member2[j].isAnnotationPresent(Nonbinding.class)) { j++; } if (i >= length1 && j >= length2) { // both ended return 0; } else if (i >= length1) { // #1 ended return 1; } else if (j >= length2) { // #2 ended return -1; } else { // not ended int c = member1[i].getName().compareTo(member2[j].getName()); if (c != 0) { return c; } Object value1 = callMethod(annotation1, member1[i]); Object value2 = callMethod(annotation2, member2[j]); assert value1.getClass().equals(value2.getClass()); if (value1 instanceof Comparable) { c = ((Comparable)value1).compareTo(value2); if (c != 0) { return c; } } else if (value1.getClass().isArray()) { c = value1.getClass().getComponentType().getName() .compareTo(value2.getClass().getComponentType().getName()); if (c != 0) { return c; } int length = Array.getLength(value1); c = length - Array.getLength(value2); if (c != 0) { return c; } for (int k = 0; k < length; k++) { c = ((Comparable)Array.get(value1, k)).compareTo(Array.get(value2, k)); if (c != 0) { return c; } } } else if (value1 instanceof Class) { c = ((Class)value1).getName().compareTo(((Class) value2).getName()); if (c != 0) { return c; } } else { // valid types for members are only Comparable, Arrays, or Class assert false; } } } } } ``` The modification can be found in the commit. The normal run has passed but it fails with nondex. After adding sorting algorithm, no failure was reported after 500 runs # Does this PR introduce any user-facing change? No # Is the change a dependency upgrade? No # How was this patch tested? Test Environment: ``` openjdk version "11.0.20.1" Apache Maven 3.6.3 Ubuntu 20.04.6 LTS Linux version: 5.4.0-163-generic ``` -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: dev-unsubscr...@openwebbeans.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
