Avoid BusyIndicator putting Display to sleep after being done

[HeikoKlare]

Due to a race condition between the BusyIndicator's event loop spinning and the display.wake() call performed after the future to wait for is done, the BusyIndicator execution may get stuck in a display.sleep() call. While on Windows and MacOS the wake() call will effectively still ensure that display.sleep() return immediately if invoked display.sleep() was called, this is neither guaranteed nor does that happen on Linux.

This fixes the race condition by replacing the wake() call in the future with scheduling an empty task that ensures that the event queue remains spinned (like everywhere else).

Contributes to #3044

[github-actions]

Test Results

  170 files  ±0    170 suites  ±0   27m 33s ⏱️ + 2m 19s
4 679 tests ±0  4 658 ✅ ±0   21 💤 ±0  0 ❌ ±0 
6 592 runs  ±0  6 437 ✅ ±0  155 💤 ±0  0 ❌ ±0 

Results for commit 1ddb382. ± Comparison against base commit a589ee9.

♻️ This comment has been updated with latest results.

[Copilot]

Pull request overview

This PR fixes a Linux-specific race condition in BusyIndicator.showWhile(Future<?>) where the display thread could get stuck in display.sleep() if a Display#wake() happens just before sleep() is entered. It replaces the completion wake-up mechanism for CompletionStage/CompletableFuture with a no-op asyncExec, ensuring an event is always queued and sleep() is reliably interrupted across platforms.

Changes:

• Replace display.wake() with a no-op display.asyncExec(...) in the CompletionStage completion handler to avoid lost wakeups.
• Add inline comments explaining why asyncExec() is used to prevent the race.

[akurtakov]

Looks like a good next step to me.

[akurtakov]

I am unsure what is better to push it for M1 now with the risk to have to delay contribution or delay it for M2.

[merks]

It would not be a big problem to delay m1 to Saturday if that’s better for the team. I would recommend that.

I think we just need to let @MohananRahul know.

[laeubi]

It seems not harmful here, the only improvement I see is we can make the lambda a static final field. If I remember correctly SWT even accept a null argument here.

I don't think this is required for M1 but also won't harm... I do not expect any special improvement or risk with having it or not in real world use-cases.

[HeikoKlare]

It seems not harmful here, the only improvement I see is we can make the lambda a static final field. If I remember correctly SWT even accept a null argument here.

Makes sense. I updated the PR so that we use a static final field. You could also pass null, but that has a different effect: it would fall back to wake() on Windows, keeping the race condition there.

eclipse.platform.swt/bundles/org.eclipse.swt/Eclipse SWT/common/org/eclipse/swt/widgets/Synchronizer.java

Lines 93 to 99 in a589ee9

	if (runnable == null) {
	//TEMPORARY CODE
	if (!(IS_GTK || IS_COCOA)) {
	display.wake ();
	return;
	}
	}

I don't think this is required for M1 but also won't harm... I do not expect any special improvement or risk with having it or not in real world use-cases.

We are now beyond M1 anyway. I propose to merge this now so that we could find any unexpected suspicious behavior soon.