Fix race condition in AbstractReconciler.reset() event ordering#3794
Merged
vogella merged 2 commits intoeclipse-platform:masterfrom Mar 27, 2026
Merged
Fix race condition in AbstractReconciler.reset() event ordering#3794vogella merged 2 commits intoeclipse-platform:masterfrom
vogella merged 2 commits intoeclipse-platform:masterfrom
Conversation
Contributor
|
This pull request changes some projects for the first time in this development cycle. An additional commit containing all the necessary changes was pushed to the top of this PR's branch. To obtain these changes (for example if you want to push more changes) either fetch from your fork or apply the git patch. Git patchFurther information are available in Common Build Issues - Missing version increments. |
Contributor
Contributor
Author
|
Test failure is reported here: #3796 |
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 eclipse-platform#2708
Contributor
Author
|
Merging so that this gets in early and we can spot issue with it. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
FastAbstractReconcilerTest.testReplacingDocumentWhenCleanby reorderingBackgroundWorker.reset()so that thereconcilerReset()hook executes beforefIsDirtyis set to true and before any queue notifications wake the background threadReconcilerResetOrderingTestthat deterministically reproduces the race condition by widening the race window with a 50ms delay — this test always fails without the fix and always passes with itRoot Cause
In
BackgroundWorker.reset(),reconcilerReset()was called afterfIsDirtywas set and afternotifyAll()/informNotFinished(). Two race windows existed:aboutToBeReconciledInternal()(called beforereset()in bothdocumentChangedandinputDocumentChanged) triggerssignalWaitForFinish()→notifyAll(), waking the background thread. It could then seefIsDirty=true(set at the start ofreset()), consume thefResetflag, skip delay (waitFinish=true), and reachprocess()beforereconcilerReset()ran.Within
reset()itself,notifyAll()andinformNotFinished()were called beforereconcilerReset(), providing additional wake-up points.Fix
Move
reconcilerReset()to the very beginning ofreset(), beforefIsDirtyis set. The background thread cannot find work (isDirty()returns false) and blocks indelay()until after the hook completes and state flags are set.User-visible impact
The
AbstractReconcilerdrives background processing in text editors (syntax validation, spell checking, error/warning markers, code analysis). The race occurs when the document changes or is replaced (e.g., switching files, reverting). Without the reset hook firing first,process()can run with stale state, causing brief flickers of incorrect error markers or squiggles from a previous document. Being a race condition, this is intermittent and self-corrects on the next reconciling pass.Test plan
AbstractReconcilerTest(7 tests) — base reconciler tests, all passFastAbstractReconcilerTest(7 tests) — fast-mode tests including previously flakytestReplacingDocumentWhenClean, all passReconcilerResetOrderingTest(7 tests) — new regression test with 50ms delay, deterministically fails without fix, passes with fixFixes #2708