From 2077a793faadfc5bcb2cab2685fdee898386591c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christoph=20L=C3=A4ubrich?= Date: Wed, 27 Apr 2022 10:59:28 +0200 Subject: [PATCH] Prevent JobManager UI freeze when waiting for a rule in the UI Thread Fix https://github.com/eclipse-platform/eclipse.platform.ui/issues/8 --- .../META-INF/MANIFEST.MF | 3 +- bundles/org.eclipse.e4.ui.progress/pom.xml | 2 +- .../ui/progress/internal/ProgressManager.java | 13 +- .../ui/internal/progress/ProgressManager.java | 12 +- .../META-INF/MANIFEST.MF | 1 + .../concurrency/ConcurrencyTestSuite.java | 1 + .../NoFreezeWhileWaitingForRuleTest.java | 192 ++++++++++++++++++ 7 files changed, 215 insertions(+), 9 deletions(-) create mode 100644 tests/org.eclipse.ui.tests/Eclipse UI Tests/org/eclipse/ui/tests/concurrency/NoFreezeWhileWaitingForRuleTest.java diff --git a/bundles/org.eclipse.e4.ui.progress/META-INF/MANIFEST.MF b/bundles/org.eclipse.e4.ui.progress/META-INF/MANIFEST.MF index 549f08e74ca..b54315af226 100644 --- a/bundles/org.eclipse.e4.ui.progress/META-INF/MANIFEST.MF +++ b/bundles/org.eclipse.e4.ui.progress/META-INF/MANIFEST.MF @@ -1,7 +1,7 @@ Manifest-Version: 1.0 Bundle-ManifestVersion: 2 Bundle-SymbolicName: org.eclipse.e4.ui.progress;singleton:=true -Bundle-Version: 0.3.200.qualifier +Bundle-Version: 0.3.400.qualifier Bundle-Name: %pluginName Bundle-Vendor: %providerName Bundle-Localization: plugin @@ -12,6 +12,7 @@ Require-Bundle: org.eclipse.core.jobs;bundle-version="3.5.400", org.eclipse.e4.core.di;bundle-version="1.3.0", org.eclipse.e4.ui.di;bundle-version="1.0.0", org.eclipse.jface;bundle-version="3.10.0", + org.eclipse.core.jobs;bundle-version="3.13.0", org.eclipse.e4.ui.workbench, org.eclipse.e4.core.commands, org.eclipse.e4.core.services, diff --git a/bundles/org.eclipse.e4.ui.progress/pom.xml b/bundles/org.eclipse.e4.ui.progress/pom.xml index 7e2123e48c8..e24783dc85e 100644 --- a/bundles/org.eclipse.e4.ui.progress/pom.xml +++ b/bundles/org.eclipse.e4.ui.progress/pom.xml @@ -16,7 +16,7 @@ org.eclipse.e4 org.eclipse.e4.ui.progress - 0.3.200-SNAPSHOT + 0.3.400-SNAPSHOT eclipse-plugin diff --git a/bundles/org.eclipse.e4.ui.progress/src/org/eclipse/e4/ui/progress/internal/ProgressManager.java b/bundles/org.eclipse.e4.ui.progress/src/org/eclipse/e4/ui/progress/internal/ProgressManager.java index a778c2ebbdb..9fa1ae975a9 100644 --- a/bundles/org.eclipse.e4.ui.progress/src/org/eclipse/e4/ui/progress/internal/ProgressManager.java +++ b/bundles/org.eclipse.e4.ui.progress/src/org/eclipse/e4/ui/progress/internal/ProgressManager.java @@ -1,5 +1,5 @@ /******************************************************************************* - * Copyright (c) 2003, 2020 IBM Corporation and others. + * Copyright (c) 2003, 2022 IBM Corporation and others. * * This program and the accompanying materials * are made available under the terms of the Eclipse Public License 2.0 @@ -14,6 +14,7 @@ * - Fix for Bug 151204 [Progress] Blocked status of jobs are not applied/reported * Lars Vogel - Bug 422040 * Philipp Bumann - Bug 477602 + * Christoph Läubrich - Issue #8 *******************************************************************************/ package org.eclipse.e4.ui.progress.internal; @@ -35,7 +36,6 @@ import org.eclipse.core.runtime.IProgressMonitor; import org.eclipse.core.runtime.IStatus; import org.eclipse.core.runtime.ListenerList; -import org.eclipse.core.runtime.NullProgressMonitor; import org.eclipse.core.runtime.QualifiedName; import org.eclipse.core.runtime.Status; import org.eclipse.core.runtime.jobs.IJobChangeEvent; @@ -511,16 +511,21 @@ protected void sleepJobInfo(JobInfo info) { @Override public IProgressMonitor getDefaultMonitor() { + return monitorFor(null); + } + + @Override + public IProgressMonitor monitorFor(IProgressMonitor monitor) { // only need a default monitor for operations the UI thread // and only if there is a display Display display; if (PlatformUI.isWorkbenchRunning() && !PlatformUI.isWorkbenchStarting()) { display = getDisplay(); if (!display.isDisposed() && (display.getThread() == Thread.currentThread())) { - return new EventLoopProgressMonitor(new NullProgressMonitor()); + return new EventLoopProgressMonitor(IProgressMonitor.nullSafe(monitor)); } } - return super.getDefaultMonitor(); + return IProgressMonitor.nullSafe(monitor); } /** diff --git a/bundles/org.eclipse.ui.workbench/Eclipse UI/org/eclipse/ui/internal/progress/ProgressManager.java b/bundles/org.eclipse.ui.workbench/Eclipse UI/org/eclipse/ui/internal/progress/ProgressManager.java index ca4209b58b9..648d65d4cf9 100644 --- a/bundles/org.eclipse.ui.workbench/Eclipse UI/org/eclipse/ui/internal/progress/ProgressManager.java +++ b/bundles/org.eclipse.ui.workbench/Eclipse UI/org/eclipse/ui/internal/progress/ProgressManager.java @@ -1,5 +1,5 @@ /******************************************************************************* - * Copyright (c) 2003, 2020 IBM Corporation and others. + * Copyright (c) 2003, 2022 IBM Corporation and others. * * This program and the accompanying materials * are made available under the terms of the Eclipse Public License 2.0 @@ -14,6 +14,7 @@ * - Fix for Bug 151204 [Progress] Blocked status of jobs are not applied/reported * Lars Vogel - Bug 422040 * Terry Parker - Bug 454633, Report the cumulative error status of job groups in the ProgressManager + * Christoph Läubrich - Issue #8 *******************************************************************************/ package org.eclipse.ui.internal.progress; @@ -575,16 +576,21 @@ public IProgressMonitor createMonitor(Job job) { @Override public IProgressMonitor getDefaultMonitor() { + return monitorFor(null); + } + + @Override + public IProgressMonitor monitorFor(IProgressMonitor monitor) { // only need a default monitor for operations the UI thread // and only if there is a display Display display; if (PlatformUI.isWorkbenchRunning() && !PlatformUI.getWorkbench().isStarting()) { display = PlatformUI.getWorkbench().getDisplay(); if (!display.isDisposed() && (display.getThread() == Thread.currentThread())) { - return new EventLoopProgressMonitor(new NullProgressMonitor()); + return new EventLoopProgressMonitor(IProgressMonitor.nullSafe(monitor)); } } - return super.getDefaultMonitor(); + return IProgressMonitor.nullSafe(monitor); } /** diff --git a/bundles/org.eclipse.ui.workbench/META-INF/MANIFEST.MF b/bundles/org.eclipse.ui.workbench/META-INF/MANIFEST.MF index f2bbb591c70..9e0a2b7d60b 100644 --- a/bundles/org.eclipse.ui.workbench/META-INF/MANIFEST.MF +++ b/bundles/org.eclipse.ui.workbench/META-INF/MANIFEST.MF @@ -103,6 +103,7 @@ Require-Bundle: org.eclipse.core.runtime;bundle-version="[3.25.0,4.0.0)", org.eclipse.core.databinding.property;bundle-version="[1.2.0,2.0.0)", org.eclipse.core.databinding.observable;bundle-version="[1.2.0,2.0.0)", org.eclipse.core.expressions;bundle-version="[3.7.0,4.0.0)", + org.eclipse.core.jobs;bundle-version="[3.13.0,4.0.0)", org.eclipse.e4.core.services;bundle-version="2.2.0", org.eclipse.e4.core.contexts;bundle-version="1.0.0", org.eclipse.e4.core.di;bundle-version="1.1.0", diff --git a/tests/org.eclipse.ui.tests/Eclipse UI Tests/org/eclipse/ui/tests/concurrency/ConcurrencyTestSuite.java b/tests/org.eclipse.ui.tests/Eclipse UI Tests/org/eclipse/ui/tests/concurrency/ConcurrencyTestSuite.java index 785eff8c370..b72df654f4c 100644 --- a/tests/org.eclipse.ui.tests/Eclipse UI Tests/org/eclipse/ui/tests/concurrency/ConcurrencyTestSuite.java +++ b/tests/org.eclipse.ui.tests/Eclipse UI Tests/org/eclipse/ui/tests/concurrency/ConcurrencyTestSuite.java @@ -27,6 +27,7 @@ NestedSyncExecDeadlockTest.class, SyncExecWhileUIThreadWaitsForRuleTest.class, SyncExecWhileUIThreadWaitsForLock.class, + NoFreezeWhileWaitingForRuleTest.class, TestBug105491.class, TestBug108162.class, TestBug98621.class, diff --git a/tests/org.eclipse.ui.tests/Eclipse UI Tests/org/eclipse/ui/tests/concurrency/NoFreezeWhileWaitingForRuleTest.java b/tests/org.eclipse.ui.tests/Eclipse UI Tests/org/eclipse/ui/tests/concurrency/NoFreezeWhileWaitingForRuleTest.java new file mode 100644 index 00000000000..b0f51bb620a --- /dev/null +++ b/tests/org.eclipse.ui.tests/Eclipse UI Tests/org/eclipse/ui/tests/concurrency/NoFreezeWhileWaitingForRuleTest.java @@ -0,0 +1,192 @@ +/******************************************************************************* + * Copyright (c) 2022 Christoph Läubrich and others. + * + * This program and the accompanying materials + * are made available under the terms of the Eclipse Public License 2.0 + * which accompanies this distribution, and is available at + * https://www.eclipse.org/legal/epl-2.0/ + * + * SPDX-License-Identifier: EPL-2.0 + * + * Contributors: + * Christoph Läubrich - initial API and implementation + ******************************************************************************/ +package org.eclipse.ui.tests.concurrency; + +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; +import static org.junit.Assert.fail; + +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicBoolean; + +import org.eclipse.core.runtime.ICoreRunnable; +import org.eclipse.core.runtime.IProgressMonitor; +import org.eclipse.core.runtime.NullProgressMonitor; +import org.eclipse.core.runtime.ProgressMonitorWrapper; +import org.eclipse.core.runtime.jobs.Job; +import org.eclipse.swt.widgets.Display; +import org.eclipse.ui.tests.concurrency.SyncExecWhileUIThreadWaitsForRuleTest.TestRule; +import org.junit.After; +import org.junit.Before; +import org.junit.Test; + +/** + * This test makes sure that a waiting on a rule do not blocks the UI thread, it + * has three participants + * + *
    + *
  1. A Job that acquire and block a rule until it is notified
    2. + *
  2. A runnable in the UI thread that tries to acquire the rule and will be + * blocked
    3. + *
  3. A thread that notify the first job via the UI thread to simulate some UI + * events after a certain number of UI events have happened
    4. + *
+ * + */ +public class NoFreezeWhileWaitingForRuleTest { + + TestRule rule; + IProgressMonitor ruleMonitor; + private CountDownLatch eventQueueLatch; + + @Before + public void setUp() { + rule = new SyncExecWhileUIThreadWaitsForRuleTest.TestRule(); + ruleMonitor = new ProgressMonitorWrapper(new NullProgressMonitor()) { + AtomicBoolean canceled = new AtomicBoolean(); + + @Override + public void setCanceled(boolean cancel) { + canceled.set(cancel); + } + + @Override + public boolean isCanceled() { + return canceled.get(); + } + }; + eventQueueLatch = new CountDownLatch(1); + } + + @After + public void tearDown() { + ruleMonitor.setCanceled(true); // just in case... + } + + @Test + public void testWaiting() { + // If you want to see the blocking uncomment the following line: + // Job.getJobManager().setProgressProvider(null); + Display display = Display.getDefault(); + assertTrue(display.getThread() == Thread.currentThread()); + try { + Job blockingJob = spinRuleBlockingJob(); + CountDownLatch runnableLatch = spinUIblockingRunnable(display); + Thread uiEventProducer = spinUIEventProducer(runnableLatch, display); + while (!eventQueueLatch.await(10, TimeUnit.MILLISECONDS) && !ruleMonitor.isCanceled()) { + while (display.readAndDispatch()) { + } + Thread.onSpinWait(); + } + uiEventProducer.interrupt(); + blockingJob.join(); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + fail(); + } + assertFalse("Timeout reached, blocking occurred!", ruleMonitor.isCanceled()); + } + + /** + * @param runnableLatch + * @param display + * @return + */ + private Thread spinUIEventProducer(CountDownLatch runnableLatch, Display display) { + Thread thread = new Thread(() -> { + // Stage 1: Wait for the UI-Thread to block... + try { + runnableLatch.await(); + } catch (InterruptedException e) { + return; + } + // Stage 2: Schedule async exec to simulate some UI events happen... + CountDownLatch eventLatch = new CountDownLatch(10); + display.asyncExec(new Runnable() { + @Override + public void run() { + if (ruleMonitor.isCanceled()) { + // break out... + return; + } + try { + TimeUnit.MILLISECONDS.sleep(100); + } catch (InterruptedException e) { + } + if (eventLatch.getCount() > 0) { + eventLatch.countDown(); + display.asyncExec(this); + } + } + }); + // Stage 3: wait for the events + try { + eventLatch.await(); + } catch (InterruptedException e) { + return; + } + // Stage 4: signal the blocking thread... + eventQueueLatch.countDown(); + }); + thread.setDaemon(true); + thread.start(); + return thread; + } + + /** + * @param display + * @return + * + */ + private CountDownLatch spinUIblockingRunnable(Display display) { + CountDownLatch runnableRunning = new CountDownLatch(1); + display.asyncExec(() -> { + runnableRunning.countDown(); + Job.getJobManager().beginRule(rule, ruleMonitor); + Job.getJobManager().endRule(rule); + }); + return runnableRunning; + } + + private Job spinRuleBlockingJob() throws InterruptedException { + CountDownLatch jobStarted = new CountDownLatch(1); + long timeout = System.currentTimeMillis() + TimeUnit.SECONDS.toMillis(20); + Job job = Job.create("Runs until notified", new ICoreRunnable() { + + @Override + public void run(IProgressMonitor monitor) { + Job.getJobManager().beginRule(rule, ruleMonitor); + jobStarted.countDown(); + try { + while (!eventQueueLatch.await(1, TimeUnit.SECONDS)) { + if (System.currentTimeMillis() > timeout) { + ruleMonitor.setCanceled(true); + break; + } + Thread.yield(); + } + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + } finally { + Job.getJobManager().endRule(rule); + } + } + }); + job.schedule(); + assertTrue("Job was not started", jobStarted.await(10, TimeUnit.SECONDS)); + return job; + + } +}