Skip to content

Commit 334254f

Browse files
committed
Fix race condition in AbstractReconciler.reset() event ordering
In BackgroundWorker.reset(), reconcilerReset() was called after fIsDirty was set to true and after fDirtyRegionQueue.notifyAll() and informNotFinished(). This created two race windows: 1. aboutToBeReconciledInternal() (called before reset() in both documentChanged and inputDocumentChanged) triggers signalWaitForFinish() which wakes the background thread via notifyAll(). The background thread could then see fIsDirty=true (set at the start of reset()), consume the fReset flag, loop back, skip delay (waitFinish=true), and reach process() — all before the main thread reached reconcilerReset(). 2. Within reset() itself, notifyAll() and informNotFinished() (which also triggers signalWaitForFinish → notifyAll) were called before reconcilerReset(), providing additional wake-up points for the background thread. The fix moves reconcilerReset() to the very beginning of reset(), before fIsDirty is set to true. This way, even if the background thread is already awake from an earlier notification, it cannot find work to do (isDirty() returns false) and blocks in delay() until after reconcilerReset() completes and the state flags are set. The reordered reset() is now: 1. reconcilerReset() — hook runs while isDirty is still false 2. Set state flags (fIsDirty, fReset) in synchronized block 3. informNotFinished() — may wake background thread 4. fDirtyRegionQueue.notifyAll() — wakes background thread This guarantees the reconcilerReset hook completes before the background thread can proceed to process(). User-visible impact: The AbstractReconciler drives background processing in text editors (syntax validation, spell checking, error/warning markers, code analysis). The race condition occurs when the document changes or is replaced (e.g., switching files, reverting). Without the reset happening first, process() can run with stale state, causing brief flickers of incorrect error markers or squiggles from the previous document on the new content. Being a race condition, this is intermittent and self-corrects on the next reconciling pass. Test coverage: Added ReconcilerResetOrderingTest which extends FastAbstractReconcilerTest with a 50ms delay before logging reconcilerReset. This widens the race window so that without the fix, the background thread deterministically reaches process() before the reset hook logs — causing testReplacingDocumentWhenClean and testDirtyingWhenClean to always fail. With the fix, all tests pass because the background thread cannot find work (isDirty=false) during the delay. The full reconciler test suite (AbstractReconcilerTest + FastAbstractReconcilerTest + ReconcilerResetOrderingTest, 21 tests) covers reset() through all code paths. See #2708
1 parent fa50c5b commit 334254f

4 files changed

Lines changed: 53 additions & 8 deletions

File tree

bundles/org.eclipse.jface.text/src/org/eclipse/jface/text/reconciler/AbstractReconciler.java

Lines changed: 6 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -140,29 +140,27 @@ public void suspendCallerWhileDirty() {
140140
*/
141141
public void reset() {
142142

143+
reconcilerReset();
144+
143145
if (fDelay > 0) {
144146

145147
synchronized (this) {
146148
fIsDirty= true;
147149
fReset= true;
148150
}
149-
synchronized (fDirtyRegionQueue) {
150-
fDirtyRegionQueue.notifyAll(); // wake up wait(fDelay);
151-
}
152151

153152
} else {
154153

155154
synchronized (this) {
156155
fIsDirty= true;
157156
}
158-
159-
synchronized (fDirtyRegionQueue) {
160-
fDirtyRegionQueue.notifyAll();
161-
}
162157
}
163158

164159
informNotFinished();
165-
reconcilerReset();
160+
161+
synchronized (fDirtyRegionQueue) {
162+
fDirtyRegionQueue.notifyAll();
163+
}
166164
}
167165

168166
/**

tests/org.eclipse.jface.text.tests/src/org/eclipse/jface/text/tests/JFaceTextTestSuite.java

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@
2626
import org.eclipse.jface.text.tests.contentassist.IncrementalAsyncContentAssistTests;
2727
import org.eclipse.jface.text.tests.reconciler.AbstractReconcilerTest;
2828
import org.eclipse.jface.text.tests.reconciler.FastAbstractReconcilerTest;
29+
import org.eclipse.jface.text.tests.reconciler.ReconcilerResetOrderingTest;
2930
import org.eclipse.jface.text.tests.rules.FastPartitionerTest;
3031
import org.eclipse.jface.text.tests.rules.FastPartitionerZeroLengthTest;
3132
import org.eclipse.jface.text.tests.rules.ScannerColumnTest;
@@ -61,6 +62,7 @@
6162

6263
AbstractReconcilerTest.class,
6364
FastAbstractReconcilerTest.class,
65+
ReconcilerResetOrderingTest.class,
6466

6567
FastPartitionerZeroLengthTest.class,
6668
FastPartitionerTest.class,

tests/org.eclipse.jface.text.tests/src/org/eclipse/jface/text/tests/reconciler/AbstractReconcilerTest.java

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -173,6 +173,7 @@ protected void aboutToBeReconciled() {
173173
}
174174
@Override
175175
protected void reconcilerReset() {
176+
AbstractReconcilerTest.this.beforeReconcilerResetLogged();
176177
fCallLog.add("reconcilerReset");
177178
}
178179
@Override
@@ -202,6 +203,10 @@ void aboutToWork(@SuppressWarnings("unused") AbstractReconciler reconciler) {
202203
// nothing
203204
}
204205

206+
void beforeReconcilerResetLogged() {
207+
// hook for subclasses to widen race window
208+
}
209+
205210
@AfterEach
206211
public void tearDown() throws Exception {
207212
fBarrier.shutdown();
Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,40 @@
1+
/*******************************************************************************
2+
* Copyright (c) 2025 Contributors to the Eclipse Foundation.
3+
*
4+
* This program and the accompanying materials
5+
* are made available under the terms of the Eclipse Public License 2.0
6+
* which accompanies this distribution, and is available at
7+
* https://www.eclipse.org/legal/epl-2.0/
8+
*
9+
* SPDX-License-Identifier: EPL-2.0
10+
*******************************************************************************/
11+
package org.eclipse.jface.text.tests.reconciler;
12+
13+
import org.eclipse.jface.text.reconciler.AbstractReconciler;
14+
15+
/**
16+
* Regression test for the race condition in
17+
* {@link AbstractReconciler#install BackgroundWorker.reset()} where
18+
* {@code reconcilerReset()} could be called after the background thread was
19+
* already notified, allowing {@code process()} to run before the reset hook.
20+
* <p>
21+
* This test widens the race window by introducing a 50ms delay before logging
22+
* {@code reconcilerReset}. Without the fix (reconcilerReset called before queue
23+
* notification), the background thread is already awake during the delay and
24+
* reaches {@code process()} first, causing {@code testReplacingDocumentWhenClean}
25+
* to deterministically fail.
26+
* </p>
27+
*
28+
* @see <a href="https://github.com/eclipse-platform/eclipse.platform.ui/issues/2708">Issue 2708</a>
29+
*/
30+
public class ReconcilerResetOrderingTest extends FastAbstractReconcilerTest {
31+
32+
@Override
33+
void beforeReconcilerResetLogged() {
34+
try {
35+
Thread.sleep(50);
36+
} catch (InterruptedException e) {
37+
Thread.currentThread().interrupt();
38+
}
39+
}
40+
}

0 commit comments

Comments
 (0)