From 647d485bc3525be91e488fa3338111860480f9e6 Mon Sep 17 00:00:00 2001 From: Andrey Loskutov Date: Mon, 8 Sep 2025 17:29:24 +0200 Subject: [PATCH] Few WorkManager fields are not MT safe Few fields are not volatile/atomic but could be accessed from different threads, so even if the modification happens while holding workspace locks, other threads might see stale values. Fields in question are - `WorkManager.operationCanceled` - `WorkManager.nestedOperations` - `WorkManager.preparedOperations` Additionally changed Workspace.endOperation() to read workManager.getPreparedOperationDepth() only once to avoid inconsistent reads. Fixes https://github.com/eclipse-platform/eclipse.platform/issues/2148 --- .../core/internal/resources/WorkManager.java | 26 +++++++++++-------- .../core/internal/resources/Workspace.java | 9 ++++--- 2 files changed, 20 insertions(+), 15 deletions(-) diff --git a/resources/bundles/org.eclipse.core.resources/src/org/eclipse/core/internal/resources/WorkManager.java b/resources/bundles/org.eclipse.core.resources/src/org/eclipse/core/internal/resources/WorkManager.java index b687851f062..3753fbaf551 100644 --- a/resources/bundles/org.eclipse.core.resources/src/org/eclipse/core/internal/resources/WorkManager.java +++ b/resources/bundles/org.eclipse.core.resources/src/org/eclipse/core/internal/resources/WorkManager.java @@ -15,13 +15,17 @@ *******************************************************************************/ package org.eclipse.core.internal.resources; +import java.util.concurrent.atomic.AtomicInteger; import org.eclipse.core.internal.utils.Messages; import org.eclipse.core.internal.utils.Policy; import org.eclipse.core.resources.IResource; import org.eclipse.core.resources.IResourceStatus; import org.eclipse.core.runtime.CoreException; import org.eclipse.core.runtime.IProgressMonitor; -import org.eclipse.core.runtime.jobs.*; +import org.eclipse.core.runtime.jobs.IJobManager; +import org.eclipse.core.runtime.jobs.ILock; +import org.eclipse.core.runtime.jobs.ISchedulingRule; +import org.eclipse.core.runtime.jobs.Job; /** * The work manager governs concurrent access to the workspace tree. The {@link #lock} @@ -78,16 +82,16 @@ public boolean isConflicting(ISchedulingRule rule) { /** * The current depth of running nested operations. */ - private int nestedOperations = 0; + private final AtomicInteger nestedOperations = new AtomicInteger(0); private final NotifyRule notifyRule = new NotifyRule(); - private boolean operationCanceled = false; + private volatile boolean operationCanceled; /** * The current depth of prepared operations. */ - private int preparedOperations = 0; + private final AtomicInteger preparedOperations = new AtomicInteger(0); private final Workspace workspace; WorkManager(Workspace workspace) { @@ -165,7 +169,7 @@ synchronized void checkOut(ISchedulingRule rule) { decrementPreparedOperations(); rebalanceNestedOperations(); //reset state if this is the end of a top level operation - if (preparedOperations == 0) { + if (preparedOperations.get() == 0) { hasBuildChanges = false; } //don't let cancelation of this operation affect other operations @@ -184,7 +188,7 @@ synchronized void checkOut(ISchedulingRule rule) { * prepareOperation/endOperation block. */ private void decrementPreparedOperations() { - preparedOperations--; + preparedOperations.decrementAndGet(); } /** @@ -218,7 +222,7 @@ ISchedulingRule getNotifyRule() { * prepareOperation/endOperation block. */ synchronized int getPreparedOperationDepth() { - return preparedOperations; + return preparedOperations.get(); } /** @@ -227,7 +231,7 @@ synchronized int getPreparedOperationDepth() { * prepareOperation/endOperation block. */ void incrementNestedOperations() { - nestedOperations++; + nestedOperations.incrementAndGet(); } /** @@ -236,7 +240,7 @@ void incrementNestedOperations() { * prepareOperation/endOperation block. */ private void incrementPreparedOperations() { - preparedOperations++; + preparedOperations.incrementAndGet(); } /** @@ -246,7 +250,7 @@ private void incrementPreparedOperations() { * outside a prepareOperation/endOperation block. */ boolean isBalanced() { - return nestedOperations == preparedOperations; + return nestedOperations.get() == preparedOperations.get(); } /** @@ -285,7 +289,7 @@ void operationCanceled() { * be called from outside a prepareOperation/endOperation block. */ void rebalanceNestedOperations() { - nestedOperations = preparedOperations; + nestedOperations.set(preparedOperations.get()); } /** diff --git a/resources/bundles/org.eclipse.core.resources/src/org/eclipse/core/internal/resources/Workspace.java b/resources/bundles/org.eclipse.core.resources/src/org/eclipse/core/internal/resources/Workspace.java index 690cb8fe72e..550122c3a3f 100644 --- a/resources/bundles/org.eclipse.core.resources/src/org/eclipse/core/internal/resources/Workspace.java +++ b/resources/bundles/org.eclipse.core.resources/src/org/eclipse/core/internal/resources/Workspace.java @@ -1593,9 +1593,10 @@ public void endOperation(ISchedulingRule rule, boolean build) throws CoreExcepti boolean depthOne = false; try { workManager.setBuild(build); - // if we are not exiting a top level operation then just decrement the count and return - depthOne = workManager.getPreparedOperationDepth() == 1; - if (!(notificationManager.shouldNotify() || depthOne)) { + // if we are not exiting a top level operation then schedule notify and return + final int preparedOperationDepth = workManager.getPreparedOperationDepth(); + depthOne = preparedOperationDepth == 1; + if (!depthOne && !notificationManager.shouldNotify()) { notificationManager.requestNotify(); return; } @@ -1604,7 +1605,7 @@ public void endOperation(ISchedulingRule rule, boolean build) throws CoreExcepti try { notificationManager.beginNotify(); // check for a programming error on using beginOperation/endOperation - Assert.isTrue(workManager.getPreparedOperationDepth() > 0, "Mismatched begin/endOperation"); //$NON-NLS-1$ + Assert.isTrue(preparedOperationDepth > 0, "Mismatched begin/endOperation"); //$NON-NLS-1$ // At this time we need to re-balance the nested operations. It is necessary because // build() and snapshot() should not fail if they are called.