6713352: Deadlock in JFileChooser with synchronized custom FileSystemView
Reviewed-by: malenkov, peterz
--- a/jdk/src/share/classes/javax/swing/plaf/basic/BasicDirectoryModel.java Fri May 15 12:06:22 2009 +0400
+++ b/jdk/src/share/classes/javax/swing/plaf/basic/BasicDirectoryModel.java Fri May 15 17:26:45 2009 +0400
@@ -230,51 +230,46 @@
}
public void run0() {
- DoChangeContents doChangeContents = ShellFolder.getInvoker().invoke(new Callable<DoChangeContents>() {
- public DoChangeContents call() throws Exception {
- FileSystemView fileSystem = filechooser.getFileSystemView();
+ FileSystemView fileSystem = filechooser.getFileSystemView();
+
+ File[] list = fileSystem.getFiles(currentDirectory, filechooser.isFileHidingEnabled());
- File[] list = fileSystem.getFiles(currentDirectory, filechooser.isFileHidingEnabled());
+ if (isInterrupted()) {
+ return;
+ }
- Vector<File> acceptsList = new Vector<File>();
+ final Vector<File> newFileCache = new Vector<File>();
+ Vector<File> newFiles = new Vector<File>();
- if (isInterrupted()) {
- return null;
- }
+ // run through the file list, add directories and selectable files to fileCache
+ // Note that this block must be OUTSIDE of Invoker thread because of
+ // deadlock possibility with custom synchronized FileSystemView
+ for (File file : list) {
+ if (filechooser.accept(file)) {
+ boolean isTraversable = filechooser.isTraversable(file);
- // run through the file list, add directories and selectable files to fileCache
- for (File file : list) {
- if (filechooser.accept(file)) {
- acceptsList.addElement(file);
- }
+ if (isTraversable) {
+ newFileCache.addElement(file);
+ } else if (filechooser.isFileSelectionEnabled()) {
+ newFiles.addElement(file);
}
if (isInterrupted()) {
- return null;
+ return;
}
-
- // First sort alphabetically by filename
- sort(acceptsList);
+ }
+ }
- Vector<File> newDirectories = new Vector<File>(50);
- Vector<File> newFiles = new Vector<File>();
- // run through list grabbing directories in chunks of ten
- for (int i = 0; i < acceptsList.size(); i++) {
- File f = acceptsList.elementAt(i);
- boolean isTraversable = filechooser.isTraversable(f);
- if (isTraversable) {
- newDirectories.addElement(f);
- } else if (!isTraversable && filechooser.isFileSelectionEnabled()) {
- newFiles.addElement(f);
- }
- if (isInterrupted()) {
- return null;
- }
- }
+ // First sort alphabetically by filename
+ sort(newFileCache);
+ sort(newFiles);
- Vector<File> newFileCache = new Vector<File>(newDirectories);
- newFileCache.addAll(newFiles);
+ newFileCache.addAll(newFiles);
+ // To avoid loads of synchronizations with Invoker and improve performance we
+ // execute the whole block on the COM thread
+ DoChangeContents doChangeContents = ShellFolder.getInvoker().invoke(new Callable<DoChangeContents>() {
+ public DoChangeContents call() throws Exception {
int newSize = newFileCache.size();
int oldSize = fileCache.size();
--- a/jdk/src/share/classes/sun/awt/shell/ShellFolder.java Fri May 15 12:06:22 2009 +0400
+++ b/jdk/src/share/classes/sun/awt/shell/ShellFolder.java Fri May 15 17:26:45 2009 +0400
@@ -274,45 +274,61 @@
// Override File methods
- public static void sort(List<? extends File> files) {
+ public static void sort(final List<? extends File> files) {
if (files == null || files.size() <= 1) {
return;
}
- // Check that we can use the ShellFolder.sortChildren() method:
- // 1. All files have the same non-null parent
- // 2. All files is ShellFolders
- File commonParent = null;
+ // To avoid loads of synchronizations with Invoker and improve performance we
+ // synchronize the whole code of the sort method once
+ getInvoker().invoke(new Callable<Void>() {
+ public Void call() throws Exception {
+ // Check that we can use the ShellFolder.sortChildren() method:
+ // 1. All files have the same non-null parent
+ // 2. All files is ShellFolders
+ File commonParent = null;
- for (File file : files) {
- File parent = file.getParentFile();
+ for (File file : files) {
+ File parent = file.getParentFile();
- if (parent == null || !(file instanceof ShellFolder)) {
- commonParent = null;
+ if (parent == null || !(file instanceof ShellFolder)) {
+ commonParent = null;
- break;
- }
+ break;
+ }
- if (commonParent == null) {
- commonParent = parent;
- } else {
- if (commonParent != parent && !commonParent.equals(parent)) {
- commonParent = null;
+ if (commonParent == null) {
+ commonParent = parent;
+ } else {
+ if (commonParent != parent && !commonParent.equals(parent)) {
+ commonParent = null;
- break;
+ break;
+ }
+ }
}
- }
- }
- if (commonParent instanceof ShellFolder) {
- ((ShellFolder) commonParent).sortChildren(files);
- } else {
- Collections.sort(files, FILE_COMPARATOR);
- }
+ if (commonParent instanceof ShellFolder) {
+ ((ShellFolder) commonParent).sortChildren(files);
+ } else {
+ Collections.sort(files, FILE_COMPARATOR);
+ }
+
+ return null;
+ }
+ });
}
- public void sortChildren(List<? extends File> files) {
- Collections.sort(files, FILE_COMPARATOR);
+ public void sortChildren(final List<? extends File> files) {
+ // To avoid loads of synchronizations with Invoker and improve performance we
+ // synchronize the whole code of the sort method once
+ getInvoker().invoke(new Callable<Void>() {
+ public Void call() throws Exception {
+ Collections.sort(files, FILE_COMPARATOR);
+
+ return null;
+ }
+ });
}
public boolean isAbsolute() {
--- a/jdk/src/windows/classes/sun/awt/shell/Win32ShellFolder2.java Fri May 15 12:06:22 2009 +0400
+++ b/jdk/src/windows/classes/sun/awt/shell/Win32ShellFolder2.java Fri May 15 17:26:45 2009 +0400
@@ -527,7 +527,7 @@
/**
* @return Whether this is a file system shell folder
*/
- public synchronized boolean isFileSystem() {
+ public boolean isFileSystem() {
if (cachedIsFileSystem == null) {
cachedIsFileSystem = hasAttribute(ATTRIB_FILESYSTEM);
}
@@ -543,8 +543,8 @@
public Boolean call() throws Exception {
// Caching at this point doesn't seem to be cost efficient
return (getAttributes0(getParentIShellFolder(),
- getRelativePIDL(), attribute)
- & attribute) != 0;
+ getRelativePIDL(), attribute)
+ & attribute) != 0;
}
});
}
@@ -761,7 +761,7 @@
/**
* @return Whether this shell folder is a link
*/
- public synchronized boolean isLink() {
+ public boolean isLink() {
if (cachedIsLink == null) {
cachedIsLink = hasAttribute(ATTRIB_LINK);
}
@@ -1160,8 +1160,16 @@
private static native int compareIDsByColumn(long pParentIShellFolder, long pidl1, long pidl2, int columnIdx);
- public void sortChildren(List<? extends File> files) {
- Collections.sort(files, new ColumnComparator(getIShellFolder(), 0));
+ public void sortChildren(final List<? extends File> files) {
+ // To avoid loads of synchronizations with Invoker and improve performance we
+ // synchronize the whole code of the sort method once
+ getInvoker().invoke(new Callable<Void>() {
+ public Void call() throws Exception {
+ Collections.sort(files, new ColumnComparator(getIShellFolder(), 0));
+
+ return null;
+ }
+ });
}
private static class ColumnComparator implements Comparator<File> {
--- a/jdk/src/windows/classes/sun/awt/shell/Win32ShellFolderManager2.java Fri May 15 12:06:22 2009 +0400
+++ b/jdk/src/windows/classes/sun/awt/shell/Win32ShellFolderManager2.java Fri May 15 17:26:45 2009 +0400
@@ -478,21 +478,22 @@
public <T> T invoke(Callable<T> task) {
try {
- T result;
if (Thread.currentThread() == comThread) {
// if it's already called from the COM
// thread, we don't need to delegate the task
- result = task.call();
+ return task.call();
} else {
- Future<T> future = submit(task);
- try {
- result = future.get();
- } catch (InterruptedException e) {
- result = null;
- future.cancel(true);
+ while (true) {
+ Future<T> future = submit(task);
+
+ try {
+ return future.get();
+ } catch (InterruptedException e) {
+ // Repeat the attempt
+ future.cancel(true);
+ }
}
}
- return result;
} catch (Exception e) {
Throwable cause = (e instanceof ExecutionException) ? e.getCause() : e;
if (cause instanceof RuntimeException) {
--- /dev/null Thu Jan 01 00:00:00 1970 +0000
+++ b/jdk/test/javax/swing/JFileChooser/6713352/bug6713352.java Fri May 15 17:26:45 2009 +0400
@@ -0,0 +1,99 @@
+/*
+ * Copyright 2009 Sun Microsystems, Inc. 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 Sun Microsystems, Inc., 4150 Network Circle, Santa Clara,
+ * CA 95054 USA or visit www.sun.com if you need additional information or
+ * have any questions.
+ */
+
+/* @test
+ @bug 6713352
+ @summary Deadlock in JFileChooser with synchronized custom FileSystemView
+ @author Pavel Porvatov
+ @run main bug6713352
+*/
+
+import sun.awt.shell.ShellFolder;
+
+import javax.swing.*;
+import javax.swing.filechooser.FileSystemView;
+import java.io.File;
+import java.io.FileNotFoundException;
+import java.io.IOException;
+
+public class bug6713352 {
+ public static void main(String[] args) throws Exception {
+ SwingUtilities.invokeAndWait(new Runnable() {
+ public void run() {
+ MyFileSystemView systemView = new MyFileSystemView();
+
+ synchronized (systemView) { // Get SystemView lock
+ new JFileChooser(systemView);
+
+ // Wait a little bit. BasicDirectoryModel will lock Invoker and stop on
+ // the bug6713352.MyFileSystemView.getFiles() method
+ try {
+ Thread.sleep(5000);
+ } catch (InterruptedException e) {
+ throw new RuntimeException(e);
+ }
+
+ try {
+ System.out.println("Try to get Invokers lock");
+
+ ShellFolder.getShellFolder(new File("c:/")).listFiles(true);
+ } catch (FileNotFoundException e) {
+ throw new RuntimeException(e);
+ }
+ }
+
+ // To avoid RejectedExecutionException in BasicDirectoryModel wait a second
+ try {
+ Thread.sleep(1000);
+ } catch (InterruptedException e) {
+ throw new RuntimeException(e);
+ }
+ }
+ });
+ }
+
+ private static class MyFileSystemView extends FileSystemView {
+
+ public File createNewFolder(File containingDir) throws IOException {
+ return null;
+ }
+
+ public File[] getFiles(File dir, boolean useFileHiding) {
+ System.out.println("getFiles start");
+
+ File[] result;
+
+ synchronized (this) {
+ result = super.getFiles(dir, useFileHiding);
+ }
+
+ System.out.println("getFiles finished");
+
+ return result;
+ }
+
+ public synchronized Boolean isTraversable(File f) {
+ return super.isTraversable(f);
+ }
+ }
+}