Bug 501681 - Deadlock in WorkbenchErrorHandler.handle
Fix a deadlock situation introduced via bug 241244 commit
1904f477320dac4a9f4d45f7be478efba4a0b735.
If a job code wants to handle a status in a "blocking" way, submit an
async task for UI thread to avoid deadlocks.
Change-Id: I3d86408fefd324db41ab87494ed98f62d05131c9
Signed-off-by: Andrey Loskutov <loskutov@gmx.de>
diff --git a/bundles/org.eclipse.ui.workbench/Eclipse UI/org/eclipse/ui/statushandlers/StatusManager.java b/bundles/org.eclipse.ui.workbench/Eclipse UI/org/eclipse/ui/statushandlers/StatusManager.java
index 0e232ef..a84203e 100644
--- a/bundles/org.eclipse.ui.workbench/Eclipse UI/org/eclipse/ui/statushandlers/StatusManager.java
+++ b/bundles/org.eclipse.ui.workbench/Eclipse UI/org/eclipse/ui/statushandlers/StatusManager.java
@@ -54,8 +54,8 @@
* <li>SHOW - a style indicating that handlers should show a problem to an user
* without blocking the calling method while awaiting user response. This is
* generally done using a non modal {@link Dialog}</li>
- * <li>BLOCK - a style indicating that the handling should block the calling
- * method until the user has responded. This is generally done using a modal
+ * <li>BLOCK - a style indicating that the handling should block the UI
+ * until the user has responded. This is generally done using a modal
* window such as a {@link Dialog}</li>
* </ul>
* </p>
@@ -102,7 +102,7 @@
* therefore likely but not required that the <code>StatusHandler</code>
* would achieve this through the use of a modal dialog.
* </p><p>Due to the fact
- * that use of <code>BLOCK</code> will block any thread, care should be
+ * that use of <code>BLOCK</code> will block UI, care should be
* taken in this use of this flag.
* </p>
*/
diff --git a/bundles/org.eclipse.ui.workbench/Eclipse UI/org/eclipse/ui/statushandlers/WorkbenchErrorHandler.java b/bundles/org.eclipse.ui.workbench/Eclipse UI/org/eclipse/ui/statushandlers/WorkbenchErrorHandler.java
index 30a35a2..4fed5d8 100644
--- a/bundles/org.eclipse.ui.workbench/Eclipse UI/org/eclipse/ui/statushandlers/WorkbenchErrorHandler.java
+++ b/bundles/org.eclipse.ui.workbench/Eclipse UI/org/eclipse/ui/statushandlers/WorkbenchErrorHandler.java
@@ -48,22 +48,7 @@
if (Display.getCurrent() != null) {
showStatusAdapter(statusAdapter, block);
} else {
- if (block) {
- Display.getDefault().syncExec(new Runnable() {
- @Override
- public void run() {
- showStatusAdapter(statusAdapter, true);
- }
- });
-
- } else {
- Display.getDefault().asyncExec(new Runnable() {
- @Override
- public void run() {
- showStatusAdapter(statusAdapter, false);
- }
- });
- }
+ Display.getDefault().asyncExec(() -> showStatusAdapter(statusAdapter, block));
}
}
diff --git a/tests/org.eclipse.ui.tests/Eclipse UI Tests/org/eclipse/ui/tests/concurrency/FreezeMonitor.java b/tests/org.eclipse.ui.tests/Eclipse UI Tests/org/eclipse/ui/tests/concurrency/FreezeMonitor.java
new file mode 100644
index 0000000..9bff464
--- /dev/null
+++ b/tests/org.eclipse.ui.tests/Eclipse UI Tests/org/eclipse/ui/tests/concurrency/FreezeMonitor.java
@@ -0,0 +1,64 @@
+/*******************************************************************************
+ * Copyright (c) 2016 Google, Inc and others.
+ * All rights reserved. This program and the accompanying materials
+ * are made available under the terms of the Eclipse Public License v1.0
+ * which accompanies this distribution, and is available at
+ * http://www.eclipse.org/legal/epl-v10.html
+ *
+ * Contributors:
+ * Stefan Xenos (Google) - Initial implementation
+ *******************************************************************************/
+package org.eclipse.ui.tests.concurrency;
+
+import java.lang.management.ManagementFactory;
+import java.lang.management.ThreadInfo;
+import java.lang.management.ThreadMXBean;
+
+import org.eclipse.core.runtime.ICoreRunnable;
+import org.eclipse.core.runtime.IProgressMonitor;
+import org.eclipse.core.runtime.OperationCanceledException;
+import org.eclipse.core.runtime.jobs.Job;
+
+public class FreezeMonitor {
+
+ private static /* @Nullable */ Job monitorJob;
+
+ public static void expectCompletionIn(final long millis) {
+ done();
+ monitorJob = Job.create("FreezeMonitor", new ICoreRunnable() {
+ @Override
+ public void run(IProgressMonitor monitor) {
+ if (monitor.isCanceled()) {
+ throw new OperationCanceledException();
+ }
+ StringBuilder result = new StringBuilder();
+ result.append("Possible frozen test case\n");
+ ThreadMXBean threadStuff = ManagementFactory.getThreadMXBean();
+ ThreadInfo[] allThreads = threadStuff.getThreadInfo(threadStuff.getAllThreadIds(), 200);
+ for (ThreadInfo threadInfo : allThreads) {
+ result.append("\"");
+ result.append(threadInfo.getThreadName());
+ result.append("\": ");
+ result.append(threadInfo.getThreadState());
+ result.append("\n");
+ final StackTraceElement[] elements = threadInfo.getStackTrace();
+ for (StackTraceElement element : elements) {
+ result.append(" ");
+ result.append(element);
+ result.append("\n");
+ }
+ result.append("\n");
+ }
+ System.out.println(result.toString());
+ }
+ });
+ monitorJob.schedule(millis);
+ }
+
+ public static void done() {
+ if (monitorJob != null) {
+ monitorJob.cancel();
+ monitorJob = null;
+ }
+ }
+}
diff --git a/tests/org.eclipse.ui.tests/Eclipse UI Tests/org/eclipse/ui/tests/statushandlers/StatusDialogManagerTest.java b/tests/org.eclipse.ui.tests/Eclipse UI Tests/org/eclipse/ui/tests/statushandlers/StatusDialogManagerTest.java
index 9f133da..26653c8 100644
--- a/tests/org.eclipse.ui.tests/Eclipse UI Tests/org/eclipse/ui/tests/statushandlers/StatusDialogManagerTest.java
+++ b/tests/org.eclipse.ui.tests/Eclipse UI Tests/org/eclipse/ui/tests/statushandlers/StatusDialogManagerTest.java
@@ -12,6 +12,10 @@
package org.eclipse.ui.tests.statushandlers;
+import java.util.concurrent.Semaphore;
+import java.util.concurrent.atomic.AtomicReference;
+import java.util.concurrent.locks.ReentrantLock;
+
import org.eclipse.core.runtime.IProgressMonitor;
import org.eclipse.core.runtime.IStatus;
import org.eclipse.core.runtime.MultiStatus;
@@ -34,6 +38,7 @@
import org.eclipse.swt.widgets.Button;
import org.eclipse.swt.widgets.Composite;
import org.eclipse.swt.widgets.Control;
+import org.eclipse.swt.widgets.Display;
import org.eclipse.swt.widgets.Event;
import org.eclipse.swt.widgets.Label;
import org.eclipse.swt.widgets.Link;
@@ -49,8 +54,10 @@
import org.eclipse.ui.statushandlers.IStatusAdapterConstants;
import org.eclipse.ui.statushandlers.StatusAdapter;
import org.eclipse.ui.statushandlers.StatusManager;
+import org.eclipse.ui.statushandlers.StatusManager.INotificationListener;
import org.eclipse.ui.statushandlers.WorkbenchErrorHandler;
import org.eclipse.ui.statushandlers.WorkbenchStatusDialogManager;
+import org.eclipse.ui.tests.concurrency.FreezeMonitor;
import org.eclipse.ui.tests.harness.util.UITestCase;
import junit.framework.TestCase;
@@ -73,11 +80,12 @@
@Override
protected void setUp() throws Exception {
+ super.setUp();
automatedMode = ErrorDialog.AUTOMATED_MODE;
wsdm = new WorkbenchStatusDialogManager(null, null);
ErrorDialog.AUTOMATED_MODE = false;
wsdm.setProperty(IStatusDialogConstants.ANIMATION, Boolean.FALSE);
- super.setUp();
+ FreezeMonitor.expectCompletionIn(60_000);
}
public void testBlockingAppearance() {
@@ -1049,6 +1057,100 @@
assertTrue("Custom support area provider should be consulted", consulted[0]);
}
+ public void testDeadlockFromBug501681() throws Exception {
+ assertNotNull("Test must run in UI thread", Display.getCurrent());
+
+ final StatusAdapter statusAdapter = createStatusAdapter("Oops");
+ AtomicReference<StatusAdapter[]> reported = new AtomicReference<>();
+ INotificationListener listener = (type, adapters) -> {
+ reported.set(adapters);
+ };
+
+ AtomicReference<ReentrantLock> lock = new AtomicReference<>();
+ lock.set(new ReentrantLock());
+ final Semaphore semaphore = new Semaphore(0);
+
+ // This simulates a thread locked some shared resource
+ // It will release lock only if "wait" is set to "false"
+ Thread t = new Thread(() -> {
+ lock.get().lock();
+ try {
+ semaphore.acquire();
+ } catch (InterruptedException e) {
+ // continue
+ }
+ lock.get().unlock();
+ });
+ t.start();
+
+ // Wait for thread to acquire the lock
+ while (!lock.get().isLocked()) {
+ Thread.sleep(50);
+ }
+ assertTrue(lock.get().isLocked());
+
+ // Verify status handling works (without blocking)
+ StatusManager.getManager().addListener(listener);
+ StatusManager.getManager().handle(statusAdapter, StatusManager.SHOW | StatusManager.LOG);
+ assertSame(statusAdapter, reported.get()[0]);
+ Shell shell = StatusDialogUtil.getStatusShell();
+ if (shell != null) {
+ shell.dispose();
+ }
+ reported.set(null);
+
+ // Verify status handling works with blocking
+ final StatusAdapter statusAdapter2 = createStatusAdapter("Oops2");
+ try {
+ // this job will try to report some "blocking" status
+ // if it does NOT deadlock, the "wait" will be unset and lock will
+ // be released
+ Job badJob = new Job("Will report blocking error") {
+ @Override
+ protected IStatus run(IProgressMonitor monitor) {
+ StatusManager.getManager().handle(statusAdapter2, StatusManager.BLOCK | StatusManager.LOG);
+ // stop blocking thread
+ semaphore.release();
+ return Status.OK_STATUS;
+ }
+ };
+
+ // start later so that we have time to lock ourselves
+ badJob.schedule(100);
+
+ // This will now block UI until the lock is released
+ // Without the patch for bug 501681 this will never happen
+ // because the job we start in a moment will wait for UI thread to
+ // show the blocking dialog
+ lock.get().lock();
+
+ // make sure job was done
+ badJob.join();
+ } finally {
+ // Will "unblock" the blocking dialog shown by the job, must be
+ // posted with delay, because dialog is not yet shown
+ postUnblockingTask();
+
+ // Allow the blocking dialog to be shown
+ UITestCase.processEvents();
+ StatusManager.getManager().removeListener(listener);
+ }
+ assertFalse("Job should successfully finish", semaphore.hasQueuedThreads());
+ assertNotNull("Status adapter was not logged", reported.get());
+ assertSame(statusAdapter2, reported.get()[0]);
+ }
+
+ private void postUnblockingTask() {
+ Display.getCurrent().timerExec(100, () -> {
+ Shell shell = StatusDialogUtil.getStatusShellImmediately();
+ if (shell != null) {
+ shell.dispose();
+ } else {
+ // shell not shown yet? re-post
+ postUnblockingTask();
+ }
+ });
+ }
/**
* Delivers custom support area.
@@ -1238,6 +1340,7 @@
@Override
protected void tearDown() throws Exception {
Shell shell = StatusDialogUtil.getStatusShell();
+ FreezeMonitor.done();
if (shell != null) {
shell.dispose();
WorkbenchStatusDialogManagerImpl impl = (WorkbenchStatusDialogManagerImpl) wsdm
diff --git a/tests/org.eclipse.ui.tests/Eclipse UI Tests/org/eclipse/ui/tests/statushandlers/StatusDialogUtil.java b/tests/org.eclipse.ui.tests/Eclipse UI Tests/org/eclipse/ui/tests/statushandlers/StatusDialogUtil.java
index ff82c48..96fa614 100644
--- a/tests/org.eclipse.ui.tests/Eclipse UI Tests/org/eclipse/ui/tests/statushandlers/StatusDialogUtil.java
+++ b/tests/org.eclipse.ui.tests/Eclipse UI Tests/org/eclipse/ui/tests/statushandlers/StatusDialogUtil.java
@@ -59,6 +59,10 @@
public static Shell getStatusShell(){
UITestCase.waitForJobs(100, 1000);
+ return getStatusShellImmediately();
+ }
+
+ public static Shell getStatusShellImmediately() {
Shell[] shells = Display.getDefault().getShells();
for (int i = 0; i < shells.length; i++) {
if (shells[i].getText().equals("Problem Occurred")
diff --git a/tests/org.eclipse.ui.tests/META-INF/MANIFEST.MF b/tests/org.eclipse.ui.tests/META-INF/MANIFEST.MF
index 5e73ce0..649e040 100644
--- a/tests/org.eclipse.ui.tests/META-INF/MANIFEST.MF
+++ b/tests/org.eclipse.ui.tests/META-INF/MANIFEST.MF
@@ -2,7 +2,7 @@
Bundle-ManifestVersion: 2
Bundle-Name: Eclipse UI Tests
Bundle-SymbolicName: org.eclipse.ui.tests; singleton:=true
-Bundle-Version: 3.11.2.qualifier
+Bundle-Version: 3.12.0.qualifier
Eclipse-BundleShape: dir
Bundle-Activator: org.eclipse.ui.tests.TestPlugin
Bundle-Vendor: Eclipse.org
diff --git a/tests/org.eclipse.ui.tests/pom.xml b/tests/org.eclipse.ui.tests/pom.xml
index 2637ef9..448e428 100644
--- a/tests/org.eclipse.ui.tests/pom.xml
+++ b/tests/org.eclipse.ui.tests/pom.xml
@@ -19,7 +19,7 @@
</parent>
<groupId>org.eclipse.ui</groupId>
<artifactId>org.eclipse.ui.tests</artifactId>
- <version>3.11.2-SNAPSHOT</version>
+ <version>3.12.0-SNAPSHOT</version>
<packaging>eclipse-test-plugin</packaging>
<properties>
