diff options
author | Andrey Loskutov | 2016-10-21 12:43:03 +0000 |
---|---|---|
committer | Sarika Sinha | 2016-11-09 10:04:10 +0000 |
commit | 55be2b2039f8c5815b319fed1f3f7c62f122130c (patch) | |
tree | d7e779bed330389f4959466d09e8d500fdd60ac7 | |
parent | 3fa670f3c24690b6b741ae5a88b57829972ccfd7 (diff) | |
download | eclipse.platform.debug-55be2b2039f8c5815b319fed1f3f7c62f122130c.tar.gz eclipse.platform.debug-55be2b2039f8c5815b319fed1f3f7c62f122130c.tar.xz eclipse.platform.debug-55be2b2039f8c5815b319fed1f3f7c62f122130c.zip |
Bug 506182 - Launch is not multi-thread safe
Protect access to fTargets and fProcesses with read/write lock to avoid
possible ConcurrentModificationExceptions.
Change-Id: I1a42b263cd9b64c7ebb914b5fdea0d2d3e408b73
Signed-off-by: Andrey Loskutov <loskutov@gmx.de>
5 files changed, 368 insertions, 63 deletions
diff --git a/org.eclipse.debug.core/core/org/eclipse/debug/core/Launch.java b/org.eclipse.debug.core/core/org/eclipse/debug/core/Launch.java index 1c0c8ccd9..3fe0b3ade 100644 --- a/org.eclipse.debug.core/core/org/eclipse/debug/core/Launch.java +++ b/org.eclipse.debug.core/core/org/eclipse/debug/core/Launch.java @@ -1,5 +1,5 @@ /******************************************************************************* - * Copyright (c) 2000, 2015 IBM Corporation and others. + * Copyright (c) 2000, 2016 IBM Corporation 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 @@ -8,6 +8,7 @@ * Contributors: * IBM Corporation - initial API and implementation * Pawel Piech - Bug 82003: The IDisconnect implementation by Launch module is too restrictive. + * Andrey Loskutov <loskutov@gmx.de> - Bug 506182 - Launch is not multi-thread safe *******************************************************************************/ package org.eclipse.debug.core; @@ -15,11 +16,13 @@ package org.eclipse.debug.core; import java.util.ArrayList; import java.util.HashMap; import java.util.List; +import java.util.concurrent.locks.Lock; +import java.util.concurrent.locks.ReadWriteLock; +import java.util.concurrent.locks.ReentrantReadWriteLock; import org.eclipse.core.runtime.IStatus; import org.eclipse.core.runtime.MultiStatus; import org.eclipse.core.runtime.PlatformObject; - import org.eclipse.debug.core.model.IDebugTarget; import org.eclipse.debug.core.model.IDisconnect; import org.eclipse.debug.core.model.IProcess; @@ -41,6 +44,15 @@ import org.eclipse.debug.internal.core.LaunchManager; public class Launch extends PlatformObject implements ILaunch, IDisconnect, ILaunchListener, ILaunchConfigurationListener, IDebugEventSetListener { /** + * Lock object for controlling access to processes and targets + */ + private final ReadWriteLock lock = new ReentrantReadWriteLock(); + + private final Lock readLock = lock.readLock(); + + private final Lock writeLock = lock.writeLock(); + + /** * The debug targets associated with this * launch (the primary target is the first one * in this collection), or empty if @@ -119,15 +131,20 @@ public class Launch extends PlatformObject implements ILaunch, IDisconnect, ILau */ @Override public boolean canTerminate() { - for (IProcess process : getProcesses0()) { - if (process.canTerminate()) { - return true; + readLock.lock(); + try { + for (IProcess process : getProcesses0()) { + if (process.canTerminate()) { + return true; + } } - } - for (IDebugTarget target : getDebugTargets0()) { - if (target.canTerminate() || target.canDisconnect()) { - return true; + for (IDebugTarget target : getDebugTargets0()) { + if (target.canTerminate() || target.canDisconnect()) { + return true; + } } + } finally { + readLock.unlock(); } return false; } @@ -137,8 +154,14 @@ public class Launch extends PlatformObject implements ILaunch, IDisconnect, ILau */ @Override public Object[] getChildren() { - ArrayList<Object> children = new ArrayList<Object>(getDebugTargets0()); - children.addAll(getProcesses0()); + readLock.lock(); + ArrayList<Object> children; + try { + children = new ArrayList<Object>(getDebugTargets0()); + children.addAll(getProcesses0()); + } finally { + readLock.unlock(); + } return children.toArray(); } @@ -147,8 +170,13 @@ public class Launch extends PlatformObject implements ILaunch, IDisconnect, ILau */ @Override public IDebugTarget getDebugTarget() { - if (!getDebugTargets0().isEmpty()) { - return getDebugTargets0().get(0); + readLock.lock(); + try { + if (!getDebugTargets0().isEmpty()) { + return getDebugTargets0().get(0); + } + } finally { + readLock.unlock(); } return null; } @@ -158,7 +186,12 @@ public class Launch extends PlatformObject implements ILaunch, IDisconnect, ILau */ @Override public IProcess[] getProcesses() { - return getProcesses0().toArray(new IProcess[getProcesses0().size()]); + readLock.lock(); + try { + return getProcesses0().toArray(new IProcess[getProcesses0().size()]); + } finally { + readLock.unlock(); + } } /** @@ -192,18 +225,23 @@ public class Launch extends PlatformObject implements ILaunch, IDisconnect, ILau */ @Override public boolean isTerminated() { - if (getProcesses0().isEmpty() && getDebugTargets0().isEmpty()) { - return false; - } - for (IProcess process : getProcesses0()) { - if (!process.isTerminated()) { + readLock.lock(); + try { + if (getProcesses0().isEmpty() && getDebugTargets0().isEmpty()) { return false; } - } - for (IDebugTarget target : getDebugTargets0()) { - if (!(target.isTerminated() || target.isDisconnected())) { - return false; + for (IProcess process : getProcesses0()) { + if (!process.isTerminated()) { + return false; + } + } + for (IDebugTarget target : getDebugTargets0()) { + if (!(target.isTerminated() || target.isDisconnected())) { + return false; + } } + } finally { + readLock.unlock(); } return true; } @@ -304,7 +342,12 @@ public class Launch extends PlatformObject implements ILaunch, IDisconnect, ILau */ @Override public IDebugTarget[] getDebugTargets() { - return fTargets.toArray(new IDebugTarget[fTargets.size()]); + readLock.lock(); + try { + return fTargets.toArray(new IDebugTarget[fTargets.size()]); + } finally { + readLock.unlock(); + } } /** @@ -323,10 +366,18 @@ public class Launch extends PlatformObject implements ILaunch, IDisconnect, ILau @Override public void addDebugTarget(IDebugTarget target) { if (target != null) { - if (!getDebugTargets0().contains(target)) { - addEventListener(); - getDebugTargets0().add(target); - fireChanged(); + writeLock.lock(); + boolean changed = false; + try { + if (!getDebugTargets0().contains(target)) { + addEventListener(); + changed = getDebugTargets0().add(target); + } + } finally { + writeLock.unlock(); + if (changed) { + fireChanged(); + } } } } @@ -337,8 +388,15 @@ public class Launch extends PlatformObject implements ILaunch, IDisconnect, ILau @Override public void removeDebugTarget(IDebugTarget target) { if (target != null) { - if (getDebugTargets0().remove(target)) { - fireChanged(); + writeLock.lock(); + boolean changed = false; + try { + changed = getDebugTargets0().remove(target); + } finally { + writeLock.unlock(); + if (changed) { + fireChanged(); + } } } } @@ -349,10 +407,18 @@ public class Launch extends PlatformObject implements ILaunch, IDisconnect, ILau @Override public void addProcess(IProcess process) { if (process != null) { - if (!getProcesses0().contains(process)) { - addEventListener(); - getProcesses0().add(process); - fireChanged(); + writeLock.lock(); + boolean changed = false; + try { + if (!getProcesses0().contains(process)) { + addEventListener(); + changed = getProcesses0().add(process); + } + } finally { + writeLock.unlock(); + if (changed) { + fireChanged(); + } } } } @@ -363,8 +429,15 @@ public class Launch extends PlatformObject implements ILaunch, IDisconnect, ILau @Override public void removeProcess(IProcess process) { if (process != null) { - if (getProcesses0().remove(process)) { - fireChanged(); + writeLock.lock(); + boolean changed = false; + try { + changed = getProcesses0().remove(process); + } finally { + writeLock.unlock(); + if (changed) { + fireChanged(); + } } } } @@ -424,17 +497,22 @@ public class Launch extends PlatformObject implements ILaunch, IDisconnect, ILau */ @Override public boolean canDisconnect() { - for (IProcess process : getProcesses0()) { - if (process instanceof IDisconnect) { - if (((IDisconnect) process).canDisconnect()) { - return true; + readLock.lock(); + try { + for (IProcess process : getProcesses0()) { + if (process instanceof IDisconnect) { + if (((IDisconnect) process).canDisconnect()) { + return true; + } } } - } - for (IDebugTarget target : getDebugTargets0()) { - if (target.canDisconnect()) { - return true; + for (IDebugTarget target : getDebugTargets0()) { + if (target.canDisconnect()) { + return true; + } } + } finally { + readLock.unlock(); } return false; } @@ -444,18 +522,23 @@ public class Launch extends PlatformObject implements ILaunch, IDisconnect, ILau */ @Override public void disconnect() throws DebugException { - for (IProcess process : getProcesses0()) { - if (process instanceof IDisconnect) { - IDisconnect dis = (IDisconnect) process; - if (dis.canDisconnect()) { - dis.disconnect(); + readLock.lock(); + try { + for (IProcess process : getProcesses0()) { + if (process instanceof IDisconnect) { + IDisconnect dis = (IDisconnect) process; + if (dis.canDisconnect()) { + dis.disconnect(); + } } } - } - for (IDebugTarget target : getDebugTargets0()) { - if (target.canDisconnect()) { - target.disconnect(); + for (IDebugTarget target : getDebugTargets0()) { + if (target.canDisconnect()) { + target.disconnect(); + } } + } finally { + readLock.unlock(); } } @@ -468,17 +551,22 @@ public class Launch extends PlatformObject implements ILaunch, IDisconnect, ILau */ @Override public boolean isDisconnected() { - for (IProcess process : getProcesses0()) { - if (process instanceof IDisconnect) { - if (!((IDisconnect) process).isDisconnected()) { - return false; + readLock.lock(); + try { + for (IProcess process : getProcesses0()) { + if (process instanceof IDisconnect) { + if (!((IDisconnect) process).isDisconnected()) { + return false; + } } } - } - for (IDebugTarget target : getDebugTargets0()) { - if (!target.isDisconnected()) { - return false; + for (IDebugTarget target : getDebugTargets0()) { + if (!target.isDisconnected()) { + return false; + } } + } finally { + readLock.unlock(); } // only return true if there are processes or targets that are disconnected return hasChildren(); diff --git a/org.eclipse.debug.tests/META-INF/MANIFEST.MF b/org.eclipse.debug.tests/META-INF/MANIFEST.MF index 6cec1e3ba..5f2db82d6 100644 --- a/org.eclipse.debug.tests/META-INF/MANIFEST.MF +++ b/org.eclipse.debug.tests/META-INF/MANIFEST.MF @@ -2,7 +2,7 @@ Manifest-Version: 1.0 Bundle-ManifestVersion: 2 Bundle-Name: %pluginName Bundle-SymbolicName: org.eclipse.debug.tests;singleton:=true -Bundle-Version: 3.10.0.qualifier +Bundle-Version: 3.10.100.qualifier Bundle-Activator: org.eclipse.debug.tests.TestsPlugin Bundle-Localization: plugin Require-Bundle: org.eclipse.ui;bundle-version="[3.6.0,4.0.0)", diff --git a/org.eclipse.debug.tests/pom.xml b/org.eclipse.debug.tests/pom.xml index b1be7e41b..7c60fd22e 100644 --- a/org.eclipse.debug.tests/pom.xml +++ b/org.eclipse.debug.tests/pom.xml @@ -18,7 +18,7 @@ </parent> <groupId>org.eclipse.debug</groupId> <artifactId>org.eclipse.debug.tests</artifactId> - <version>3.10.0-SNAPSHOT</version> + <version>3.10.100-SNAPSHOT</version> <packaging>eclipse-test-plugin</packaging> <properties> <code.ignoredWarnings>${tests.ignoredWarnings}</code.ignoredWarnings> diff --git a/org.eclipse.debug.tests/src/org/eclipse/debug/tests/AutomatedSuite.java b/org.eclipse.debug.tests/src/org/eclipse/debug/tests/AutomatedSuite.java index 92efb0d1c..c06c6c868 100644 --- a/org.eclipse.debug.tests/src/org/eclipse/debug/tests/AutomatedSuite.java +++ b/org.eclipse.debug.tests/src/org/eclipse/debug/tests/AutomatedSuite.java @@ -23,6 +23,7 @@ import org.eclipse.debug.tests.launching.LaunchConfigurationTests; import org.eclipse.debug.tests.launching.LaunchFavoriteTests; import org.eclipse.debug.tests.launching.LaunchHistoryTests; import org.eclipse.debug.tests.launching.LaunchManagerTests; +import org.eclipse.debug.tests.launching.LaunchTests; import org.eclipse.debug.tests.launching.RefreshTabTests; import org.eclipse.debug.tests.sourcelookup.SourceLookupFacilityTests; import org.eclipse.debug.tests.statushandlers.StatusHandlerTests; @@ -92,6 +93,7 @@ public class AutomatedSuite extends TestSuite { addTest(new TestSuite(LaunchManagerTests.class)); addTest(new TestSuite(RefreshTabTests.class)); addTest(new TestSuite(ArgumentParsingTests.class)); + addTest(new TestSuite(LaunchTests.class)); // Status handlers addTest(new TestSuite(StatusHandlerTests.class)); diff --git a/org.eclipse.debug.tests/src/org/eclipse/debug/tests/launching/LaunchTests.java b/org.eclipse.debug.tests/src/org/eclipse/debug/tests/launching/LaunchTests.java new file mode 100644 index 000000000..2e50be204 --- /dev/null +++ b/org.eclipse.debug.tests/src/org/eclipse/debug/tests/launching/LaunchTests.java @@ -0,0 +1,215 @@ +/******************************************************************************* + * Copyright (c) 2016 Andrey Loskutov. + * 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: + * Andrey Loskutov <loskutov@gmx.de> - initial API and implementation + *******************************************************************************/ +package org.eclipse.debug.tests.launching; + +import java.lang.reflect.InvocationHandler; +import java.lang.reflect.Method; +import java.lang.reflect.Proxy; +import java.util.ConcurrentModificationException; +import java.util.concurrent.Semaphore; +import java.util.concurrent.atomic.AtomicInteger; + +import org.eclipse.core.runtime.IProgressMonitor; +import org.eclipse.core.runtime.IStatus; +import org.eclipse.core.runtime.Status; +import org.eclipse.core.runtime.jobs.Job; +import org.eclipse.debug.core.ILaunchManager; +import org.eclipse.debug.core.Launch; +import org.eclipse.debug.core.model.IDebugTarget; +import org.eclipse.debug.core.model.IDisconnect; +import org.eclipse.debug.core.model.IProcess; + +/** + * Tests for the {@link Launch} class + * + * @since 3.10 + */ +public class LaunchTests extends AbstractLaunchTest { + + private InvocationHandler handler; + private Runnable readIsTerminatedTask; + private Runnable readIsDisconnectedTask; + private Runnable writeProcessesTask; + private Runnable writeDebugTargetsTask; + + /** + * Constructor + * @param name + */ + public LaunchTests(String name) { + super(name); + } + + @Override + protected void setUp() throws Exception { + super.setUp(); + final Launch launch = new Launch(null, ILaunchManager.RUN_MODE, null); + + handler = new InvocationHandler() { + @Override + public Object invoke(Object proxy, Method method, Object[] args) throws Throwable { + String name = method.getName(); + if (name.equals("equals")) { //$NON-NLS-1$ + return args.length == 1 && proxy == args[0]; + } + return Boolean.TRUE; + } + }; + + readIsTerminatedTask = new Runnable() { + @Override + public void run() { + launch.isTerminated(); + } + }; + + readIsDisconnectedTask = new Runnable() { + @Override + public void run() { + launch.isDisconnected(); + } + }; + + writeProcessesTask = new Runnable() { + @Override + public void run() { + IProcess process = createProcessProxy(); + launch.addProcess(process); + launch.removeProcess(process); + try { + Thread.sleep(0, 1); + } catch (InterruptedException e) { + // + } + launch.addProcess(process); + launch.removeProcess(process); + } + }; + + writeDebugTargetsTask = new Runnable() { + @Override + public void run() { + IDebugTarget target2 = createDebugTargetProxy(); + launch.addDebugTarget(target2); + launch.removeDebugTarget(target2); + try { + Thread.sleep(0, 1); + } catch (InterruptedException e) { + // + } + launch.addDebugTarget(target2); + launch.removeDebugTarget(target2); + } + }; + } + + @Override + protected void tearDown() throws Exception { + super.tearDown(); + } + + /** + * Modifies debug targets and checks if this causes + * {@link ConcurrentModificationException} in the another thread + */ + public void testTerminatedAndWriteTargets() throws Exception { + assertTrue(testExecution(readIsTerminatedTask, writeDebugTargetsTask)); + } + + public void testDisconnectedAndWriteTargets() throws Exception { + assertTrue(testExecution(readIsDisconnectedTask, writeDebugTargetsTask)); + } + + /** + * Modifies processes and checks if this causes + * {@link ConcurrentModificationException} in the another thread + */ + public void testTerminatedAndWriteProcesses() throws Exception { + assertTrue(testExecution(readIsTerminatedTask, writeProcessesTask)); + } + + /** + * Modifies processes and checks if this causes + * {@link ConcurrentModificationException} in the another thread + */ + public void testDisconnectedAndWriteProcesses() throws Exception { + assertTrue(testExecution(readIsDisconnectedTask, writeProcessesTask)); + } + + private boolean testExecution(final Runnable readTask, final Runnable writeTask) { + /* + * Normally 10 times trial is sufficient to reproduce concurrent + * modification error, but 2000 is chosen for better stability of test. + * (the test execution time is less than 2 sec) + */ + final int maxTrialCount = 2000; + + final Semaphore semaphore = new Semaphore(0); + final AtomicInteger runs = new AtomicInteger(); + + Job job = new Job("modify debug target") { //$NON-NLS-1$ + + @Override + protected IStatus run(IProgressMonitor monitor) { + try { + semaphore.acquire(); + for (int i = 0; i < maxTrialCount; i++) { + if (monitor.isCanceled()) { + return Status.CANCEL_STATUS; + } + // try to modify launch data + writeTask.run(); + } + } catch (Exception e1) { + // we don't care + return Status.CANCEL_STATUS; + } finally { + runs.set(maxTrialCount); + } + return Status.OK_STATUS; + } + }; + + job.schedule(); + semaphore.release(); + + try { + while (runs.get() < maxTrialCount) { + // try to read launch data + readTask.run(); + + // avoid endless loop if job already finished + if (job.getResult() != null) { + break; + } + } + } finally { + System.out.println(getName() + " runs: " + runs); //$NON-NLS-1$ + job.cancel(); + } + + assertEquals(maxTrialCount, runs.get()); + return true; + } + + private IDebugTarget createDebugTargetProxy() { + IDebugTarget debugTarget = (IDebugTarget) Proxy.newProxyInstance(LaunchTests.class.getClassLoader(), new Class[] { + IDebugTarget.class }, handler); + return debugTarget; + } + + private IProcess createProcessProxy() { + IProcess process = (IProcess) Proxy.newProxyInstance(LaunchTests.class.getClassLoader(), new Class[] { + IProcess.class, IDisconnect.class }, handler); + return process; + } + +} |