Fix race condition in Synchronizer.addLast() causing missed wakeThread() calls

[HeikoKlare]

Problem

Synchronizer.addLast() had a TOCTOU race introduced in #77 (replacing synchronized blocks with ConcurrentLinkedQueue). The two steps — checking isEmpty() and calling add() — were not atomic, creating this window:

1. Producer thread: isEmpty() → false (queue has items) → wake = false
2. UI thread: drains all remaining items, calls sleep() → blocks in WaitMessage() / g_main_context_query()
3. Producer thread: messages.add(lock)
4. Producer thread: if (wake) → false → wakeThread() never called
5. Display sleeps indefinitely despite having a pending message

Fix

Add the lock first, then use peek() == lock (reference equality on ConcurrentLinkedQueue) to detect that our item landed at the head of the queue — meaning the queue was effectively empty at insertion time and the UI thread needs waking:

// Before
void addLast(RunnableLock lock) {
    boolean wake = messages.isEmpty();
    messages.add(lock);
    if (wake) display.wakeThread();
}

// After
void addLast(RunnableLock lock) {
    messages.add(lock);
    if (messages.peek() == lock) display.wakeThread();
}

If the UI thread consumed our lock between add() and peek(), peek() returns null or another lock — the task was already processed, no wake needed. RunnableLock objects are never pooled or reused, so reference equality is safe.

Testing

Added Test_org_eclipse_swt_widgets_Synchronizer with a regression test that runs 4 concurrent producer threads each posting 1000 asyncExec tasks while the UI thread alternately drains and sleeps. A timerExec sentinel bounds the test duration so a hung Display produces a clear assertion failure rather than an infinite hang in CI.

Fixes #3151

[github-actions]

Test Results

  170 files  ±0    170 suites  ±0   26m 11s ⏱️ -53s
4 679 tests +1  4 658 ✅ +1   21 💤 ±0  0 ❌ ±0 
6 592 runs  +6  6 437 ✅ +6  155 💤 ±0  0 ❌ ±0 

Results for commit f669e73. ± Comparison against base commit 6171cf3.

♻️ This comment has been updated with latest results.

[Copilot]

Pull request overview

Fixes a TOCTOU race in SWT’s Synchronizer.addLast() that could leave the UI thread sleeping indefinitely despite pending async work, and adds a regression test to catch missed wake-ups.

Changes:

• Make Synchronizer.addLast() wake the UI thread based on whether the just-added lock is at the queue head (peek() == lock), avoiding the non-atomic isEmpty()/add() sequence.
• Add a concurrent-producer regression test validating that asyncExec() tasks always complete under drain/sleep cycling.
• Register the new test in the widget test suite.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File	Description
bundles/org.eclipse.swt/Eclipse SWT/common/org/eclipse/swt/widgets/Synchronizer.java	Removes isEmpty()-then-add() race by switching to add() then head-check to decide waking.
tests/org.eclipse.swt.tests/JUnit Tests/org/eclipse/swt/tests/junit/Test_org_eclipse_swt_widgets_Synchronizer.java	Adds stress/regression test for missed wakeThread() behavior under concurrent asyncExec() producers.
tests/org.eclipse.swt.tests/JUnit Tests/org/eclipse/swt/tests/junit/AllWidgetTests.java	Includes the new Synchronizer regression test in the suite.

[HeikoKlare]

@laeubi with your interest and expertise in concurrency topics, do you have any concern regarding this solution? To me it looks sound but since the Synchronizer is a very central class, having another pair of eyes on it may not hurt. The change was generated by Claude. I also asked Copilot for a solution, which produced the same (which is not very surprising as they both used Sonnet 4.6). Would be nice to get this into M1 so that we have sufficient time for implicit testing before the next release.

[laeubi]

It looks sound to me as well, I'd rather think we should merge it sooner than later (so right now) as it might be manifesting already with the test failures for the BusyIndicator tests.

@akurtakov WDYT?

[akurtakov]

Looks good to me too. Merging.