6963811: Deadlock-prone locking changes in Introspector

Reviewed-by: peterz, rupashka

src/share/classes/com/sun/beans/finder/InstanceFinder.java

 /*
- * Copyright (c) 2009, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2009, 2010, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -39,7 +39,7 @@ class InstanceFinder<T> {
     private final Class<? extends T> type;
     private final boolean allow;
     private final String suffix;
-    private String[] packages;
+    private volatile String[] packages;
 
     InstanceFinder(Class<? extends T> type, boolean allow, String suffix, String... packages) {
         this.type = type;
@@ -49,9 +49,7 @@ class InstanceFinder<T> {
     }
 
     public String[] getPackages() {
-        return (this.packages.length > 0)
-                ? this.packages.clone()
-                : this.packages;
+        return this.packages.clone();
     }
 
     public void setPackages(String... packages) {

src/share/classes/com/sun/beans/finder/PersistenceDelegateFinder.java

 /*
- * Copyright (c) 2009, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2009, 2010, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -47,6 +47,7 @@ public final class PersistenceDelegateFinder
     }
 
     public void register(Class<?> type, PersistenceDelegate delegate) {
+        synchronized (this.registry) {
             if (delegate != null) {
                 this.registry.put(type, delegate);
             }
@@ -54,10 +55,14 @@ public final class PersistenceDelegateFinder
                 this.registry.remove(type);
             }
         }
+    }
 
     @Override
     public PersistenceDelegate find(Class<?> type) {
-        PersistenceDelegate delegate = this.registry.get(type);
+        PersistenceDelegate delegate;
+        synchronized (this.registry) {
+            delegate = this.registry.get(type);
+        }
         return (delegate != null) ? delegate : super.find(type);
     }
 }

src/share/classes/com/sun/beans/finder/PropertyEditorFinder.java

 /*
- * Copyright (c) 2009, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2009, 2010, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -64,12 +64,18 @@ public final class PropertyEditorFinder
     }
 
     public void register(Class<?> type, Class<?> editor) {
+        synchronized (this.registry) {
             this.registry.put(type, editor);
         }
+    }
 
     @Override
     public PropertyEditor find(Class<?> type) {
-        PropertyEditor editor = instantiate(this.registry.get(type), null);
+        Class<?> predefined;
+        synchronized (this.registry) {
+            predefined = this.registry.get(type);
+        }
+        PropertyEditor editor = instantiate(predefined, null);
         if (editor == null) {
             editor = super.find(type);
             if ((editor == null) && (null != type.getEnumConstants())) {

src/share/classes/java/beans/Encoder.java

@@ -194,13 +194,8 @@ public class Encoder {
      * @see java.beans.BeanInfo#getBeanDescriptor
      */
     public PersistenceDelegate getPersistenceDelegate(Class<?> type) {
-        synchronized (this.finder) {
         PersistenceDelegate pd = this.finder.find(type);
-            if (pd != null) {
-                return pd;
-            }
-        }
-        return MetaData.getPersistenceDelegate(type);
+        return (pd != null) ? pd : MetaData.getPersistenceDelegate(type);
     }
 
     /**
@@ -214,10 +209,8 @@ public class Encoder {
      * @see java.beans.BeanInfo#getBeanDescriptor
      */
     public void setPersistenceDelegate(Class<?> type, PersistenceDelegate delegate) {
-        synchronized (this.finder) {
         this.finder.register(type, delegate);
     }
-    }
 
     /**
      * Removes the entry for this instance, returning the old entry.

src/share/classes/java/beans/Introspector.java

@@ -158,21 +158,23 @@ public class Introspector {
         if (!ReflectUtil.isPackageAccessible(beanClass)) {
             return (new Introspector(beanClass, null, USE_ALL_BEANINFO)).getBeanInfo();
         }
+        Map<Class<?>, BeanInfo> beanInfoCache;
+        BeanInfo beanInfo;
         synchronized (BEANINFO_CACHE) {
-            Map<Class<?>, BeanInfo> beanInfoCache =
-                    (Map<Class<?>, BeanInfo>) AppContext.getAppContext().get(BEANINFO_CACHE);
-
+            beanInfoCache = (Map<Class<?>, BeanInfo>) AppContext.getAppContext().get(BEANINFO_CACHE);
             if (beanInfoCache == null) {
                 beanInfoCache = new WeakHashMap<Class<?>, BeanInfo>();
                 AppContext.getAppContext().put(BEANINFO_CACHE, beanInfoCache);
             }
-            BeanInfo beanInfo = beanInfoCache.get(beanClass);
+            beanInfo = beanInfoCache.get(beanClass);
+        }
         if (beanInfo == null) {
-                beanInfo = (new Introspector(beanClass, null, USE_ALL_BEANINFO)).getBeanInfo();
+            beanInfo = new Introspector(beanClass, null, USE_ALL_BEANINFO).getBeanInfo();
+            synchronized (BEANINFO_CACHE) {
                 beanInfoCache.put(beanClass, beanInfo);
             }
-            return beanInfo;
         }
+        return beanInfo;
     }
 
     /**
@@ -302,10 +304,7 @@ public class Introspector {
      */
 
     public static String[] getBeanInfoSearchPath() {
-        BeanInfoFinder finder = getFinder();
-        synchronized (finder) {
-            return finder.getPackages();
-        }
+        return getFinder().getPackages();
     }
 
     /**
@@ -329,10 +328,7 @@ public class Introspector {
         if (sm != null) {
             sm.checkPropertiesAccess();
         }
-        BeanInfoFinder finder = getFinder();
-        synchronized (finder) {
-            finder.setPackages(path);
-        }
+        getFinder().setPackages(path);
     }
 
 
@@ -454,10 +450,7 @@ public class Introspector {
      * @return Instance of an explicit BeanInfo class or null if one isn't found.
      */
     private static BeanInfo findExplicitBeanInfo(Class beanClass) {
-        BeanInfoFinder finder = getFinder();
-        synchronized (finder) {
-            return finder.find(beanClass);
-        }
+        return getFinder().find(beanClass);
     }
 
     /**

src/share/classes/java/beans/PropertyEditorManager.java

 /*
- * Copyright (c) 1996, 2009, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 1996, 2010, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -81,10 +81,7 @@ public class PropertyEditorManager {
         if (sm != null) {
             sm.checkPropertiesAccess();
         }
-        PropertyEditorFinder finder = getFinder();
-        synchronized (finder) {
-            finder.register(targetType, editorClass);
-        }
+        getFinder().register(targetType, editorClass);
     }
 
     /**
@@ -95,10 +92,7 @@ public class PropertyEditorManager {
      * The result is null if no suitable editor can be found.
      */
     public static PropertyEditor findEditor(Class<?> targetType) {
-        PropertyEditorFinder finder = getFinder();
-        synchronized (finder) {
-            return finder.find(targetType);
-        }
+        return getFinder().find(targetType);
     }
 
     /**
@@ -110,10 +104,7 @@ public class PropertyEditorManager {
      *         e.g. Sun implementation initially sets to  {"sun.beans.editors"}.
      */
     public static String[] getEditorSearchPath() {
-        PropertyEditorFinder finder = getFinder();
-        synchronized (finder) {
-            return finder.getPackages();
-        }
+        return getFinder().getPackages();
     }
 
     /**
@@ -134,10 +125,7 @@ public class PropertyEditorManager {
         if (sm != null) {
             sm.checkPropertiesAccess();
         }
-        PropertyEditorFinder finder = getFinder();
-        synchronized (finder) {
-            finder.setPackages(path);
-        }
+        getFinder().setPackages(path);
     }
 
     private static PropertyEditorFinder getFinder() {

test/java/beans/Introspector/Test6963811.java

+/*
+ * Copyright (c) 2010, Oracle and/or its affiliates. All rights reserved.
+ * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.
+ *
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ *
+ * You should have received a copy of the GNU General Public License version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ */
+
+/*
+ * @test
+ * @bug 6963811
+ * @summary Tests deadlock in Introspector
+ * @author Sergey Malenkov
+ */
+
+import java.beans.Introspector;
+import java.beans.SimpleBeanInfo;
+
+public class Test6963811 implements Runnable {
+    private final long time;
+    private final boolean sync;
+
+    public Test6963811(long time, boolean sync) {
+        this.time = time;
+        this.sync = sync;
+    }
+
+    public void run() {
+        try {
+            Thread.sleep(this.time); // increase the chance of the deadlock
+            Introspector.getBeanInfo(
+                    this.sync ? Super.class : Sub.class,
+                    this.sync ? null : Object.class);
+        }
+        catch (Exception exception) {
+            exception.printStackTrace();
+        }
+    }
+
+    public static void main(String[] args) throws Exception {
+        Thread[] threads = new Thread[2];
+        for (int i = 0; i < threads.length; i++) {
+            threads[i] = new Thread(new Test6963811(0L, i > 0));
+            threads[i].start();
+            Thread.sleep(500L); // increase the chance of the deadlock
+        }
+        for (Thread thread : threads) {
+            thread.join();
+        }
+    }
+
+    public static class Super {
+    }
+
+    public static class Sub extends Super {
+    }
+
+    public static class SubBeanInfo extends SimpleBeanInfo {
+        public SubBeanInfo() {
+            new Test6963811(1000L, true).run();
+        }
+    }
+}

test/java/beans/PropertyEditor/Test6963811.java

+/*
+ * Copyright (c) 2010, Oracle and/or its affiliates. All rights reserved.
+ * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.
+ *
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ *
+ * You should have received a copy of the GNU General Public License version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ */
+
+/*
+ * @test
+ * @bug 6963811
+ * @summary Tests deadlock in PropertyEditorManager
+ * @author Sergey Malenkov
+ */
+
+import java.beans.PropertyEditorManager;
+import sun.beans.editors.StringEditor;
+
+public class Test6963811 implements Runnable {
+    private final long time;
+    private final boolean sync;
+
+    public Test6963811(long time, boolean sync) {
+        this.time = time;
+        this.sync = sync;
+    }
+
+    public void run() {
+        try {
+            Thread.sleep(this.time); // increase the chance of the deadlock
+            if (this.sync) {
+                synchronized (Test6963811.class) {
+                    PropertyEditorManager.findEditor(Super.class);
+                }
+            }
+            else {
+                PropertyEditorManager.findEditor(Sub.class);
+            }
+        }
+        catch (Exception exception) {
+            exception.printStackTrace();
+        }
+    }
+
+    public static void main(String[] args) throws Exception {
+        Thread[] threads = new Thread[2];
+        for (int i = 0; i < threads.length; i++) {
+            threads[i] = new Thread(new Test6963811(0L, i > 0));
+            threads[i].start();
+            Thread.sleep(500L); // increase the chance of the deadlock
+        }
+        for (Thread thread : threads) {
+            thread.join();
+        }
+    }
+
+    public static class Super {
+    }
+
+    public static class Sub extends Super {
+    }
+
+    public static class SubEditor extends StringEditor {
+        public SubEditor() {
+            new Test6963811(1000L, true).run();
+        }
+    }
+}

test/java/beans/XMLEncoder/Test6963811.java

+/*
+ * Copyright (c) 2010, Oracle and/or its affiliates. All rights reserved.
+ * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.
+ *
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ *
+ * You should have received a copy of the GNU General Public License version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ */
+
+/*
+ * @test
+ * @bug 6963811
+ * @summary Tests deadlock in Encoder
+ * @author Sergey Malenkov
+ */
+
+import java.beans.Encoder;
+import java.beans.DefaultPersistenceDelegate;
+
+public class Test6963811 implements Runnable {
+    private static final Encoder ENCODER = new Encoder();
+    private final long time;
+    private final boolean sync;
+
+    public Test6963811(long time, boolean sync) {
+        this.time = time;
+        this.sync = sync;
+    }
+
+    public void run() {
+        try {
+            Thread.sleep(this.time); // increase the chance of the deadlock
+            if (this.sync) {
+                synchronized (Test6963811.class) {
+                    ENCODER.getPersistenceDelegate(Super.class);
+                }
+            }
+            else {
+                ENCODER.getPersistenceDelegate(Sub.class);
+            }
+        }
+        catch (Exception exception) {
+            exception.printStackTrace();
+        }
+    }
+
+    public static void main(String[] args) throws Exception {
+        Thread[] threads = new Thread[2];
+        for (int i = 0; i < threads.length; i++) {
+            threads[i] = new Thread(new Test6963811(0L, i > 0));
+            threads[i].start();
+            Thread.sleep(500L); // increase the chance of the deadlock
+        }
+        for (Thread thread : threads) {
+            thread.join();
+        }
+    }
+
+    public static class Super {
+    }
+
+    public static class Sub extends Super {
+    }
+
+    public static class SubPersistenceDelegate extends DefaultPersistenceDelegate {
+        public SubPersistenceDelegate() {
+            new Test6963811(1000L, true).run();
+        }
+    }
+}