Skip to main content
aboutsummaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authorAndrey Loskutov2016-10-21 08:43:03 -0400
committerSarika Sinha2016-11-09 05:04:10 -0500
commit55be2b2039f8c5815b319fed1f3f7c62f122130c (patch)
treed7e779bed330389f4959466d09e8d500fdd60ac7
parent3fa670f3c24690b6b741ae5a88b57829972ccfd7 (diff)
downloadeclipse.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>
-rw-r--r--org.eclipse.debug.core/core/org/eclipse/debug/core/Launch.java210
-rw-r--r--org.eclipse.debug.tests/META-INF/MANIFEST.MF2
-rw-r--r--org.eclipse.debug.tests/pom.xml2
-rw-r--r--org.eclipse.debug.tests/src/org/eclipse/debug/tests/AutomatedSuite.java2
-rw-r--r--org.eclipse.debug.tests/src/org/eclipse/debug/tests/launching/LaunchTests.java215
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;
+ }
+
+}

Back to the top