RFR 8029891 : Deadlock detected in java/lang/ClassLoader/deadlock/GetResource.java

[Peter Levart]

Hi Brent,

I think all you need for test is this:


import java.lang.reflect.Method;
import java.lang.reflect.Modifier;
import java.util.*;
import java.util.stream.Collectors;
import java.util.stream.Stream;

public class CheckOverrides {

     public static void main(String[] args) {
         Set<MethodSignature> pMethodSignatures =
             Stream.of(Properties.class.getDeclaredMethods())
                 .filter(CheckOverrides::isMethodOfInterest)
                 .map(MethodSignature::new)
                 .collect(Collectors.toSet());

         Map<MethodSignature, Method> unoverriddenMethods = new HashMap<>();
         for (Class<?> superclass = Properties.class.getSuperclass();
              superclass != Object.class;
              superclass = superclass.getSuperclass()) {
             Stream.of(superclass.getDeclaredMethods())
                 .filter(CheckOverrides::isMethodOfInterest)
                 .forEach(m -> unoverriddenMethods.putIfAbsent(new 
MethodSignature(m), m));
         }
         unoverriddenMethods.keySet().removeAll(pMethodSignatures);

         if (!unoverriddenMethods.isEmpty()) {
             throw new RuntimeException(
                 "The following methods should be overridden by 
Properties class:\n" +
                     unoverriddenMethods.values().stream()
                         .map(Method::toString)
                         .collect(Collectors.joining("\n  ", "  ", "\n"))
             );
         }
     }

     static boolean isMethodOfInterest(Method method) {
         int mods = method.getModifiers();
         return !Modifier.isStatic(mods) &&
             (Modifier.isPublic(mods) || Modifier.isProtected(mods));
     }

     static class MethodSignature {
         final Class<?> returnType;
         final String name;
         final Class<?>[] parameterTypes;

         MethodSignature(Method method) {
             this(method.getReturnType(), method.getName(), 
method.getParameterTypes());
         }

         private MethodSignature(Class<?> returnType, String name, 
Class<?>[] parameterTypes) {
             this.returnType = returnType;
             this.name = name;
             this.parameterTypes = parameterTypes;
         }

         @Override
         public boolean equals(Object o) {
             if (this == o) return true;
             if (o == null || getClass() != o.getClass()) return false;
             MethodSignature that = (MethodSignature) o;
             if (!returnType.equals(that.returnType)) return false;
             if (!name.equals(that.name)) return false;
             return Arrays.equals(parameterTypes, that.parameterTypes);
         }

         @Override
         public int hashCode() {
             int result = returnType.hashCode();
             result = 31 * result + name.hashCode();
             result = 31 * result + Arrays.hashCode(parameterTypes);
             return result;
         }
     }
}



Regards, Peter


On 05/04/2015 08:32 PM, Peter Levart wrote:
>
>
> On 05/04/2015 06:11 PM, Brent Christian wrote:
>> Hi,
>>
>> Please review this fix, courtesy of Peter Levart (thanks!), that I 
>> would like to get in.
>>
>> https://bugs.openjdk.java.net/browse/JDK-8029891
>> http://cr.openjdk.java.net/~bchristi/8029891/webrev.0/
>>
>> There is some discussion of it in the bug report, starting at 
>> 2014-12-31.
>>
>> The problem, as stated by Mandy:
>>
>> "System Properties is a hashtable that synchronizes on itself for any 
>> access. Currently System.getProperties returns the Properties 
>> instance accessed by the system in which any application code might 
>> synchronize on it (that's what the test is doing). The problem 
>> reported JDK-6977738 is about Properties.store method that was fixed 
>> not to synchronize on this instance. System property is a common way 
>> for changing the default setting and so it's impractical to expect 
>> the class loading code path not to call System.getProperty."
>>
>> This fix changes java.util.Properties to store its values in an 
>> internal ConcurrentHashMap, ignoring its Hashtable heritage.  In this 
>> way, Properties can be "de-sychronized": all methods inherited from 
>> Hashtable are overridden, to remove synchronization, and delegate to 
>> the internal CHM.
>>
>> The serialized form is unchanged.
>>
>>
>> An alternative approach considered would be for 
>> System.getProperties() to return a duplicate snapshot of the current 
>> Properties.  This presents a compatibility risk to existing code that 
>> keeps a reference to the return value of System.getProperties() and 
>> expects to either read new properties added afterwards, or set 
>> properties on the cached copy.
>>
>> -Brent
>>
>
> Hi Brent,
>
> The test looks mostly good, but I would refrain from matching the 
> exception types as part of method signature. Why?
>
> - javac already performs all the necessary checks about declared 
> exceptions if some method overrides another method
> - overriding method can declare a subset and/or subtypes of checked 
> exceptions of those than overridden method declares and this is OK.
> - overriding/overridden methods can declare arbitrary unrelated sets 
> of unchecked exceptions and this is OK.
>
> Another question is whether return type should be considered part of 
> the method signature. For JVM it is, but Java, the language, just 
> matches the method's name and parameter types and javac makes sure 
> that overriding method declarest the same return type or a subtype of 
> overridden method's return type. Well, javac generates two methods in 
> the later case - the one declared in the source code with covariant 
> return type and a synthetic bridge method with overridden method's 
> return type that just calls the declared method. 
> Class.getDeclaredMethods() returns both methods in that case. So 
> either "key" shape would work (one that takes returnType as part of 
> key and one that doesn't).
>
> I don't think you need or even want to check that Properties declares 
> all the methods with signatures from all interfaces. Just checking 
> superclass(es) would do, as Properties is not abstract and in order to 
> compile it must implement all abstract methods (either by inheriting 
> from superclass(es) or implementing them itself). Specifically, you 
> would not want to enforce interface default methods to be overridden 
> by Properties if they are not overridden by superclass(es) (as the 
> interface default method can only call other methods in the interface 
> or Object methods and you need not override such method in Properties 
> for Properties to be OK).
>
> Regards, Peter
>