From b9d62d0b72f430a63e87e88117033bd4fa9c65f3 Mon Sep 17 00:00:00 2001
From: Heiko Klare <heiko.klare@vector.com>
Date: Tue, 5 Sep 2023 14:07:33 +0200
Subject: [PATCH] Fix race condition in ThreadJob.waitForRun #659

ThreadJob.waitForRun() suffers from a race condition. Between checking
for a blocking job with a conflicting scheduling rule and starting to
wait on the state lock of that blocking job to get notified when the
scheduling rule is released, the blocking job may have already changed
its state and acquired a different scheduling rule, such that no
conflict exists anymore. Since this condition is not reevaluated but
waiting on the lock object is started without a timeout, the blocked job
unnecessarily waits for the initially blocking job to change its state
again, i.e., to finish its execution.

With this change, the scheduling rules of blocking and blocked thread
are checked for conflicts again before starting to wait on the state
lock in the blocked job. This leads to a reevaluation of the block
condition in case the scheduling rule of the blocking job has changed in
between and avoids the job to unnecessarily block until the blocking job
finishes.

Fixes https://github.com/eclipse-platform/eclipse.platform/issues/659.
---
 .../builders/ParallelBuildChainTest.java      | 11 ++++++++
 .../eclipse/core/internal/jobs/ThreadJob.java | 25 +++++++++++++------
 2 files changed, 29 insertions(+), 7 deletions(-)

diff --git a/resources/tests/org.eclipse.core.tests.resources/src/org/eclipse/core/tests/internal/builders/ParallelBuildChainTest.java b/resources/tests/org.eclipse.core.tests.resources/src/org/eclipse/core/tests/internal/builders/ParallelBuildChainTest.java
index 3309eca97b6..5e1b78c1ec4 100644
--- a/resources/tests/org.eclipse.core.tests.resources/src/org/eclipse/core/tests/internal/builders/ParallelBuildChainTest.java
+++ b/resources/tests/org.eclipse.core.tests.resources/src/org/eclipse/core/tests/internal/builders/ParallelBuildChainTest.java
@@ -141,6 +141,17 @@ public void testIndividualProjectBuilds_ProjectRelaxedRule() throws Exception {
 		});
 	}
 
+	@Test
+	public void testIndividualProjectBuilds_WithManyProjects_ProjectRelaxedRule() throws Exception {
+		int numberOfParallelBuilds = 60;
+		var longRunningProjects = createMultipleTestProjects(numberOfParallelBuilds, BuildDurationType.LONG_RUNNING,
+				RuleType.CURRENT_PROJECT_RELAXED);
+		executeIndividualFullProjectBuilds(numberOfParallelBuilds, () -> {
+			assertBuildsToStart(getAllProjects());
+			assertMinimumNumberOfSimultaneousBuilds(longRunningProjects.size());
+		});
+	}
+
 	@Test
 	public void testWorkspaceBuild_NoConflictRule() throws Exception {
 		int numberOfParallelBuilds = 3;
diff --git a/runtime/bundles/org.eclipse.core.jobs/src/org/eclipse/core/internal/jobs/ThreadJob.java b/runtime/bundles/org.eclipse.core.jobs/src/org/eclipse/core/internal/jobs/ThreadJob.java
index 6ed718fdcd1..15c8828b3a1 100644
--- a/runtime/bundles/org.eclipse.core.jobs/src/org/eclipse/core/internal/jobs/ThreadJob.java
+++ b/runtime/bundles/org.eclipse.core.jobs/src/org/eclipse/core/internal/jobs/ThreadJob.java
@@ -15,8 +15,14 @@
 
 import java.util.List;
 import org.eclipse.core.internal.runtime.RuntimeLog;
-import org.eclipse.core.runtime.*;
-import org.eclipse.core.runtime.jobs.*;
+import org.eclipse.core.runtime.Assert;
+import org.eclipse.core.runtime.IProgressMonitor;
+import org.eclipse.core.runtime.IStatus;
+import org.eclipse.core.runtime.OperationCanceledException;
+import org.eclipse.core.runtime.Status;
+import org.eclipse.core.runtime.jobs.ISchedulingRule;
+import org.eclipse.core.runtime.jobs.Job;
+import org.eclipse.core.runtime.jobs.LockListener;
 
 /**
  * Captures the implicit job state for a given thread.
@@ -317,11 +323,16 @@ private static ThreadJob waitForRun(final ThreadJob threadJob, IProgressMonitor
 						// The actual exit conditions are listed above at the beginning of
 						// this while loop
 						int state = blockingJob.getState();
-						//ensure we don't wait forever if the blocker is waiting, because it might have yielded to me
-						if (state == Job.RUNNING && canBlock) {
-							blockingJob.jobStateLock.wait();
-						} else if (state != Job.NONE) {
-							blockingJob.jobStateLock.wait(250);
+						// Check that blockingJob has not acquired a different, non-conflicting
+						// scheduling rule since checking for conflicts by JobManager
+						if (state != Job.NONE && blockingJob.getRule().isConflicting(threadJob.getRule())) {
+							// ensure we don't wait forever if the blocker is waiting, because it might have
+							// yielded to me
+							if (state == Job.RUNNING && canBlock) {
+								blockingJob.jobStateLock.wait();
+							} else {
+								blockingJob.jobStateLock.wait(250);
+							}
 						}
 					} catch (InterruptedException e) {
 						// This thread may be interrupted via two common scenarios. 1) If