Bug 285130 - reuse same background thread to process most target events

Prevents JDI events fork large number of background jobs, which
result in OutOfMemoryError: unable to create new native thread in
extreme cases.

Events are processed in the same order they were provided by the
target jvm. Events that require expression evaluation are then
forked to separate jobs (to avoid deadlock waiting on the main
event processing job).

Change-Id: Iddfff226dd101eb5288ae372b1511280e8fc7db8
Signed-off-by: Igor Fedorenko <igor@ifedorenko.com>

7 files changed, 221 insertions, 57 deletions

diff --git a/org.eclipse.jdt.debug.tests/testprograms/BulkThreadCreationTest.java b/org.eclipse.jdt.debug.tests/testprograms/BulkThreadCreationTest.java
new file mode 100644
index 000000000..ca0dad13b
--- /dev/null
+++ b/org.eclipse.jdt.debug.tests/testprograms/BulkThreadCreationTest.java
@@ -0,0 +1,27 @@
+/*******************************************************************************
+ * Copyright (c) 2017 salesforce.com.
+ * 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:
+ *     salesforce.com - initial API and implementation
+ *******************************************************************************/
+
+public class BulkThreadCreationTest {
+
+	public static void main(String[] args) throws Exception {
+		int count = 1000;
+		Thread[] threads = new Thread[count];
+		for (int i = 0; i < count; i++) {
+			threads[i] = new Thread("bulk-" + i);
+			threads[i].start();
+		}
+		for (int i = 0; i < count; i++) {
+			threads[i].join();
+		}
+		return;
+	}
+
+}
diff --git a/org.eclipse.jdt.debug.tests/tests/org/eclipse/jdt/debug/tests/AbstractDebugTest.java b/org.eclipse.jdt.debug.tests/tests/org/eclipse/jdt/debug/tests/AbstractDebugTest.java
index 9687b15d3..395fb8fd2 100644
--- a/org.eclipse.jdt.debug.tests/tests/org/eclipse/jdt/debug/tests/AbstractDebugTest.java
+++ b/org.eclipse.jdt.debug.tests/tests/org/eclipse/jdt/debug/tests/AbstractDebugTest.java
@@ -201,7 +201,7 @@ public abstract class AbstractDebugTest extends TestCase implements  IEvaluation
 			"bug329294", "bug401270", "org.eclipse.debug.tests.targets.HcrClass2", "org.eclipse.debug.tests.targets.HcrClass3", "org.eclipse.debug.tests.targets.HcrClass4",
 			"org.eclipse.debug.tests.targets.HcrClass5", "org.eclipse.debug.tests.targets.HcrClass6", "org.eclipse.debug.tests.targets.HcrClass7", "org.eclipse.debug.tests.targets.HcrClass8",
 			"org.eclipse.debug.tests.targets.HcrClass9", "TestContributedStepFilterClass", "TerminateAll_01", "TerminateAll_02", "StepResult1",
-			"StepResult2", "StepResult3", "StepUncaught", "TriggerPoint_01" };
+			"StepResult2", "StepResult3", "StepUncaught", "TriggerPoint_01", "BulkThreadCreationTest" };
 
 	final String[] LAUNCH_CONFIG_NAMES_1_8 = {"LargeSourceFile"};
 
diff --git a/org.eclipse.jdt.debug.tests/tests/org/eclipse/jdt/debug/tests/AutomatedSuite.java b/org.eclipse.jdt.debug.tests/tests/org/eclipse/jdt/debug/tests/AutomatedSuite.java
index 6e8ee0ff4..7c772c8da 100644
--- a/org.eclipse.jdt.debug.tests/tests/org/eclipse/jdt/debug/tests/AutomatedSuite.java
+++ b/org.eclipse.jdt.debug.tests/tests/org/eclipse/jdt/debug/tests/AutomatedSuite.java
@@ -27,6 +27,7 @@ import org.eclipse.jdt.debug.tests.breakpoints.ExceptionBreakpointTests;
 import org.eclipse.jdt.debug.tests.breakpoints.HitCountBreakpointsTests;
 import org.eclipse.jdt.debug.tests.breakpoints.ImportBreakpointsTest;
 import org.eclipse.jdt.debug.tests.breakpoints.JavaBreakpointListenerTests;
+import org.eclipse.jdt.debug.tests.breakpoints.JavaThreadEventHandlerTests;
 import org.eclipse.jdt.debug.tests.breakpoints.MethodBreakpointTests;
 import org.eclipse.jdt.debug.tests.breakpoints.MethodBreakpointTests15;
 import org.eclipse.jdt.debug.tests.breakpoints.MiscBreakpointsTests;
@@ -38,7 +39,6 @@ import org.eclipse.jdt.debug.tests.breakpoints.TargetPatternBreakpointTests;
 import org.eclipse.jdt.debug.tests.breakpoints.TestToggleBreakpointsTarget;
 import org.eclipse.jdt.debug.tests.breakpoints.TestToggleBreakpointsTarget8;
 import org.eclipse.jdt.debug.tests.breakpoints.ThreadFilterBreakpointsTests;
-import org.eclipse.jdt.debug.tests.breakpoints.JavaThreadEventHandlerTests;
 import org.eclipse.jdt.debug.tests.breakpoints.TriggerPointBreakpointsTests;
 import org.eclipse.jdt.debug.tests.breakpoints.TypeNameBreakpointTests;
 import org.eclipse.jdt.debug.tests.breakpoints.WatchpointTests;
@@ -58,6 +58,7 @@ import org.eclipse.jdt.debug.tests.core.ConsoleTests;
 import org.eclipse.jdt.debug.tests.core.DebugEventTests;
 import org.eclipse.jdt.debug.tests.core.EEDefinitionTests;
 import org.eclipse.jdt.debug.tests.core.EnvironmentTests;
+import org.eclipse.jdt.debug.tests.core.EventDispatcherTest;
 import org.eclipse.jdt.debug.tests.core.EventSetTests;
 import org.eclipse.jdt.debug.tests.core.ExecutionEnvironmentTests;
 import org.eclipse.jdt.debug.tests.core.HcrTests;
@@ -277,6 +278,7 @@ public class AutomatedSuite extends DebugSuite {
 		addTest(new TestSuite(StratumTests.class));
 		addTest(new TestSuite(JavaDebugTargetTests.class));
 		addTest(new TestSuite(WorkingDirectoryTests.class));
+		addTest(new TestSuite(EventDispatcherTest.class));
 
 	// Refactoring tests
 		//TODO: project rename
diff --git a/org.eclipse.jdt.debug.tests/tests/org/eclipse/jdt/debug/tests/core/EventDispatcherTest.java b/org.eclipse.jdt.debug.tests/tests/org/eclipse/jdt/debug/tests/core/EventDispatcherTest.java
new file mode 100644
index 000000000..18a5d45e0
--- /dev/null
+++ b/org.eclipse.jdt.debug.tests/tests/org/eclipse/jdt/debug/tests/core/EventDispatcherTest.java
@@ -0,0 +1,127 @@
+/*******************************************************************************
+ *  Copyright (c) 2017 salesforce.com.
+ *  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:
+ *     salesforce.com - initial API and implementation
+ *******************************************************************************/
+package org.eclipse.jdt.debug.tests.core;
+
+import java.util.Collections;
+import java.util.IdentityHashMap;
+import java.util.Map;
+import java.util.concurrent.atomic.AtomicInteger;
+
+import org.eclipse.core.runtime.jobs.IJobChangeEvent;
+import org.eclipse.core.runtime.jobs.Job;
+import org.eclipse.core.runtime.jobs.JobChangeAdapter;
+import org.eclipse.debug.core.DebugEvent;
+import org.eclipse.debug.core.DebugException;
+import org.eclipse.debug.core.DebugPlugin;
+import org.eclipse.debug.core.IDebugEventSetListener;
+import org.eclipse.debug.core.model.IStackFrame;
+import org.eclipse.jdt.debug.core.IJavaLineBreakpoint;
+import org.eclipse.jdt.debug.core.IJavaThread;
+import org.eclipse.jdt.debug.tests.AbstractDebugTest;
+import org.eclipse.jdt.internal.debug.core.EventDispatcher.AbstractDispatchJob;
+import org.eclipse.jdt.internal.debug.core.model.JDIThread;
+
+public class EventDispatcherTest extends AbstractDebugTest {
+
+	private JobChangeAdapter jobListener;
+	private Map<AbstractDispatchJob, Object> jobs;
+
+	public EventDispatcherTest(String name) {
+		super(name);
+	}
+
+	@Override
+	protected void setUp() throws Exception {
+		super.setUp();
+		jobs = Collections.synchronizedMap(new IdentityHashMap<AbstractDispatchJob, Object>());
+		jobListener = new JobChangeAdapter() {
+			@Override
+			public void aboutToRun(IJobChangeEvent event) {
+				Job job = event.getJob();
+				if (job instanceof AbstractDispatchJob) {
+					jobs.put((AbstractDispatchJob) job, Boolean.TRUE);
+				}
+			}
+		};
+		Job.getJobManager().addJobChangeListener(jobListener);
+	}
+
+	@Override
+	protected void tearDown() throws Exception {
+		if (jobListener != null) {
+			Job.getJobManager().removeJobChangeListener(jobListener);
+		}
+		super.tearDown();
+	}
+
+	public void test285130_bulkThreadCreation() throws Exception {
+		// the real goal of this test is to validate that rapidly delivered JDI events do not start large number of threads
+		// unfortunately there is no direct way to observe startup of the JDI event processing threads
+		// as approximation, assert single JDI processing Job was ever run during the test
+		// as sanity check, assert expected number of DebugEvent was delivered during the test
+
+		// sanity check: count number of JDIThread thread-create events
+		AtomicInteger threadCount = new AtomicInteger();
+		IDebugEventSetListener debugListener = events -> {
+			for (DebugEvent event : events) {
+				if (event.getKind() == DebugEvent.CREATE && event.getSource() instanceof JDIThread) {
+					JDIThread thread = (JDIThread) event.getSource();
+					try {
+						if (thread.getName().startsWith("bulk-")) {
+							threadCount.incrementAndGet();
+						}
+					}
+					catch (DebugException e) {
+					}
+				}
+			}
+		};
+		try {
+			DebugPlugin.getDefault().addDebugEventListener(debugListener);
+			createLineBreakpoint(24, "BulkThreadCreationTest");
+			launchToBreakpoint("BulkThreadCreationTest");
+		}
+		finally {
+			removeAllBreakpoints();
+			DebugPlugin.getDefault().removeDebugEventListener(debugListener);
+		}
+
+		assertEquals("Unexpected number of JDIThread thread-create events", 1000, threadCount.get());
+		assertEquals("Unexpected number of event dispatching jobs: " + jobs.size() + " | " + jobs.keySet(), 0, jobs.size());
+	}
+
+	/**
+	 * Tests that a conditional breakpoint with an expression that will hit a breakpoint will complete the conditional expression evaluation (bug
+	 * 269231) and that we dispatch events for conditional breakpoints in dedicated jobs.
+	 *
+	 * @throws Exception
+	 */
+	public void testConditionalExpressionEventDispatching() throws Exception {
+		String typeName = "BreakpointListenerTest";
+		createConditionalLineBreakpoint(15, typeName, "foo(); return false;", true);
+		IJavaLineBreakpoint breakpoint = createLineBreakpoint(20, typeName);
+		IJavaThread thread = null;
+		try {
+			thread = launchToLineBreakpoint(typeName, breakpoint);
+			IStackFrame top = thread.getTopStackFrame();
+			assertNotNull("Missing top frame", top);
+			assertTrue("Thread should be suspended", thread.isSuspended());
+			assertEquals("Wrong location", breakpoint.getLineNumber(), top.getLineNumber());
+		}
+		finally {
+			terminateAndRemove(thread);
+			removeAllBreakpoints();
+		}
+		// Expect to see two jobs for conditional breakpoint with 1) class prepare and 2) breakpoint hit events
+		assertEquals("Unexpected number of event dispatching jobs: " + jobs.size() + " | " + jobs.keySet(), 2, jobs.size());
+	}
+
+}
\ No newline at end of file
diff --git a/org.eclipse.jdt.debug/model/org/eclipse/jdt/internal/debug/core/EventDispatcher.java b/org.eclipse.jdt.debug/model/org/eclipse/jdt/internal/debug/core/EventDispatcher.java
index 216b9c4ba..299d99b17 100644
--- a/org.eclipse.jdt.debug/model/org/eclipse/jdt/internal/debug/core/EventDispatcher.java
+++ b/org.eclipse.jdt.debug/model/org/eclipse/jdt/internal/debug/core/EventDispatcher.java
@@ -65,7 +65,7 @@ public class EventDispatcher implements Runnable {
 	/**
 	 * Whether this dispatcher is shutdown.
 	 */
-	private boolean fShutdown;
+	private volatile boolean fShutdown;
 	/**
 	 * Table of event listeners. Table is a mapping of <code>EventRequest</code>
 	 * to <code>IJDIEventListener</code>.
@@ -220,6 +220,50 @@ public class EventDispatcher implements Runnable {
 		}
 	}
 
+	private boolean requiresExpressionEvaluation(EventSet eventSet) {
+		EventIterator iter = eventSet.eventIterator();
+		while (iter.hasNext()) {
+			Event event = iter.nextEvent();
+			if (event == null) {
+				continue;
+			}
+			IJDIEventListener listener = fEventHandlers.get(event.request());
+			if (listener instanceof IJavaLineBreakpoint) {
+				try {
+					if (((IJavaLineBreakpoint) listener).isConditionEnabled()) {
+						return true;
+					}
+				}
+				catch (CoreException e) {
+					return true; // assume the worst
+				}
+			}
+		}
+		return false;
+	}
+
+	/** @noreference public for test purposes */
+	public abstract class AbstractDispatchJob extends Job {
+		protected AbstractDispatchJob(String name) {
+			super(name);
+		}
+
+		@Override
+		public boolean belongsTo(Object family) {
+			return family == EventDispatcher.class || family == EventDispatcher.this;
+		}
+
+		@Override
+		public String toString() {
+			try {
+				return super.toString() + " for [" + fTarget.getName() + "]"; //$NON-NLS-1$ //$NON-NLS-2$
+			}
+			catch (DebugException e) {
+				return super.toString();
+			}
+		}
+	}
+
 	/**
 	 * Continuously reads events that are coming from the event queue, until
 	 * this event dispatcher is shutdown. A debug target starts a thread on this
@@ -232,9 +276,9 @@ public class EventDispatcher implements Runnable {
 		VirtualMachine vm = fTarget.getVM();
 		if (vm != null) {
 			EventQueue q = vm.eventQueue();
-			EventSet eventSet = null;
 			while (!isShutdown()) {
 				try {
+					EventSet eventSet;
 					try {
 						// Get the next event set.
 						eventSet = q.remove(1000);
@@ -242,42 +286,21 @@ public class EventDispatcher implements Runnable {
 						break;
 					}
 
-					if (!isShutdown() && eventSet != null) {
-						final EventSet set = eventSet;
-						Job job = new Job("JDI Event Dispatch") { //$NON-NLS-1$
-							@Override
-							protected IStatus run(IProgressMonitor monitor) {
-								dispatch(set);
-								return Status.OK_STATUS;
-							}
-
-							@Override
-							public boolean belongsTo(Object family) {
-								if (family instanceof Class) {
-									Class<?> clazz = (Class<?>) family;
-									EventIterator iterator = set.eventIterator();
-									while (iterator.hasNext()) {
-										Event event = iterator.nextEvent();
-										if (clazz.isInstance(event)) {
-											return true;
-										}
-									}
-								}
-								return false;
-							}
-
-							@Override
-							public String toString() {
-								try {
-									return super.toString() + " for [" + fTarget.getName() + "]"; //$NON-NLS-1$ //$NON-NLS-2$
-								}
-								catch (DebugException e) {
-									return super.toString();
+					if (eventSet != null) {
+						if (!requiresExpressionEvaluation(eventSet)) {
+							dispatch(eventSet);
+						} else {
+							// 269231 always evaluate expressions in a separate job to avoid deadlocks
+							Job job = new AbstractDispatchJob("JDI Expression Evaluation Event Dispatch") { //$NON-NLS-1$
+								@Override
+								protected IStatus run(IProgressMonitor monitor) {
+									dispatch(eventSet);
+									return Status.OK_STATUS;
 								}
-							}
-						};
-						job.setSystem(true);
-						job.schedule();
+							};
+							job.setSystem(true);
+							job.schedule();
+						}
 					}
 				} catch (InterruptedException e) {
 					break;
@@ -293,6 +316,7 @@ public class EventDispatcher implements Runnable {
 	 */
 	public void shutdown() {
 		fShutdown = true;
+		Job.getJobManager().cancel(this);
 	}
 
 	/**
diff --git a/org.eclipse.jdt.debug/model/org/eclipse/jdt/internal/debug/core/breakpoints/JavaBreakpoint.java b/org.eclipse.jdt.debug/model/org/eclipse/jdt/internal/debug/core/breakpoints/JavaBreakpoint.java
index cad7552d6..ba6a3fe8e 100644
--- a/org.eclipse.jdt.debug/model/org/eclipse/jdt/internal/debug/core/breakpoints/JavaBreakpoint.java
+++ b/org.eclipse.jdt.debug/model/org/eclipse/jdt/internal/debug/core/breakpoints/JavaBreakpoint.java
@@ -22,9 +22,7 @@ import java.util.Set;
 import org.eclipse.core.resources.IMarker;
 import org.eclipse.core.runtime.CoreException;
 import org.eclipse.core.runtime.MultiStatus;
-import org.eclipse.core.runtime.OperationCanceledException;
 import org.eclipse.core.runtime.Platform;
-import org.eclipse.core.runtime.jobs.Job;
 import org.eclipse.debug.core.DebugEvent;
 import org.eclipse.debug.core.DebugPlugin;
 import org.eclipse.debug.core.IDebugEventSetListener;
@@ -53,7 +51,6 @@ import com.sun.jdi.event.ClassPrepareEvent;
 import com.sun.jdi.event.Event;
 import com.sun.jdi.event.EventSet;
 import com.sun.jdi.event.LocatableEvent;
-import com.sun.jdi.event.ThreadStartEvent;
 import com.sun.jdi.request.ClassPrepareRequest;
 import com.sun.jdi.request.EventRequest;
 import com.sun.jdi.request.EventRequestManager;
@@ -307,13 +304,6 @@ public abstract class JavaBreakpoint extends Breakpoint implements IJavaBreakpoi
 		ThreadReference threadRef = ((LocatableEvent) event).thread();
 		JDIThread thread = target.findThread(threadRef);
 		if (thread == null) {
-			// wait for any thread start event sets to complete processing
-			// see bug 271700
-			try {
-				Job.getJobManager().join(ThreadStartEvent.class, null);
-			} catch (OperationCanceledException e) {
-			} catch (InterruptedException e) {
-			}
 			thread = target.findThread(threadRef);
 		}
 		if (thread == null || thread.isIgnoringBreakpoints()) {
@@ -425,8 +415,9 @@ public abstract class JavaBreakpoint extends Breakpoint implements IJavaBreakpoi
 	protected boolean installableReferenceType(ReferenceType type,
 			JDIDebugTarget target) throws CoreException {
 		String installableType = getTypeName();
-		if (installableType == null )
+		if (installableType == null ) {
 			return false;
+		}
 		String queriedType = type.name();
 		if( queriedType == null) {
 			return false;
diff --git a/org.eclipse.jdt.debug/model/org/eclipse/jdt/internal/debug/core/model/JDIDebugTarget.java b/org.eclipse.jdt.debug/model/org/eclipse/jdt/internal/debug/core/model/JDIDebugTarget.java
index 01ae84ac2..320cefae0 100644
--- a/org.eclipse.jdt.debug/model/org/eclipse/jdt/internal/debug/core/model/JDIDebugTarget.java
+++ b/org.eclipse.jdt.debug/model/org/eclipse/jdt/internal/debug/core/model/JDIDebugTarget.java
@@ -2440,13 +2440,6 @@ public class JDIDebugTarget extends JDIDebugElement implements
 			ThreadReference ref = ((ThreadDeathEvent) event).thread();
 			JDIThread thread = findThread(ref);
 			if (thread == null) {
-				// wait for any thread start event sets to complete processing
-				// see bug 272494
-				try {
-					Job.getJobManager().join(ThreadStartEvent.class, null);
-				} catch (OperationCanceledException e) {
-				} catch (InterruptedException e) {
-				}
 				thread = target.findThread(ref);
 			}
 			if (thread != null) {