Let Display#sleep() react to thread interruption

[laeubi]

Fix #3059

I did not found a good way on Windows (@HeikoKlare ?) and would like to get feedback on the MacOS part, maybe @akurtakov / @jonahgraham can take a peek on the gtk side?

[HeikoKlare]

I did not find any OS API method that allows to wait for a message until a timeout occurs instead of the inifinitely blocking to OS.WaitMessage().
Maybe something like this replacing the OS.WaitMessage() call in Display.sleep()could work (just as a conceptual idea, not a clean solution):

	boolean messageReceived = false;
	while (!messageReceived && !thread.isInterrupted()) {
		messageReceived = OS.PeekMessage (msg, 0, 0, 0, OS.PM_NOREMOVE);
		try {
			Thread.sleep(20);
		} catch (InterruptedException e) {
			thread.interrupt();
			break;
		}
	}

At least the test locally passes with it.

[laeubi]

just as a conceptual idea, not a clean solution

Thanks for looking into it, the problem is that it will immediately return but always with 20ms delay (what might be too much), I think we can't do any better then on windows for the moment.

[jonahgraham]

On win32 I think you can leave in waitmessage, but always queue a 50ms timer so that you never wait more than 50ms. I don't think it is particularly trivial to do that though as ideally you want the sleep method to handle that particular timer going off locally rather than waiting for the main event loop to handle it.

The gtk side looks good to me on code inspection, but I don't know the implications of changing return value, for example in cases where there are two conditions that caused loop to exit, should it still return true. I don't see any real uses of the sleep return value so far in the code base, so not enough of an issue for me to want to spend more time on it. I guess its too bad we can't throw InterruptedException as that would be a very disruptive API change.

[github-actions]

Test Results (win32)

   28 files  ±0     28 suites  ±0   4m 36s ⏱️ +11s
4 654 tests +1  4 580 ✅ ±0  74 💤 +1  0 ❌ ±0 
1 188 runs  +1  1 163 ✅ ±0  25 💤 +1  0 ❌ ±0 

Results for commit 304c0e9. ± Comparison against base commit e476728.

♻️ This comment has been updated with latest results.

[github-actions]

Test Results (linux)

   59 files   -    29     59 suites   - 29   10m 10s ⏱️ - 4m 40s
4 558 tests +    1  4 338 ✅ +    1  220 💤 ± 0  0 ❌ ±0 
2 194 runs   - 1 081  2 140 ✅  - 1 066   54 💤  - 15  0 ❌ ±0 

Results for commit 304c0e9. ± Comparison against base commit e476728.

♻️ This comment has been updated with latest results.

[HeikoKlare]

On win32 I think you can leave in waitmessage, but always queue a 50ms timer so that you never wait more than 50ms. I don't think it is particularly trivial to do that though as ideally you want the sleep method to handle that particular timer going off locally rather than waiting for the main event loop to handle it.

That's a nice idea but you would indeed need to process the timer event inside the sleep() method.

I shortly experimented with a simple timer and a timer callback and both of them do not seem to be processed by the event queue afterwards if the timer is killed after it elapsed (and thus made OS.WaitMessage() return). But I do not find anything in the Windows documentation that guarantees this behavior.

long wakeupId = OS.SetTimer(0, 0, 50, 0);
boolean result = OS.WaitMessage();
OS.KillTimer(0, wakeupId);

long timer = WakeupMessage.sendAfter(50);
boolean result = OS.WaitMessage();
OS.KillTimer(0, timer);

...

private static class WakeupMessage  {
	private final Callback osCallback;

	private WakeupMessage() {
		// Has to have TIMERPROC signature, see https://learn.microsoft.com/en-us/windows/win32/api/winuser/nc-winuser-timerproc
		osCallback = new Callback(this, "run", void.class, new Type[] { int.class, int.class, int.class, int.class} );
	}

	@SuppressWarnings("unused") // Executed as callback method referenced by signature description
	public void run(int hwnd, int msg , int idEvent, int dwTime) {
		OS.KillTimer(hwnd, idEvent);
		System.out.println("lalala");
		osCallback.dispose();
	}

	static long sendAfter(int millis) {
		try {
			WakeupMessage fakeMessage = new WakeupMessage();
			return OS.SetTimer(0, 0, millis, fakeMessage.osCallback.getAddress());
		} catch  (SWTError error) {
		}
		return 0;
	}
}

I now found that is the OS method MsgWaitForMultipleObjectsEx that is proposed when wanting to have a timeout while waiting for messages to arrive. However, there are several configuration options for that method via the parameters of which I would not be immediately sure how to use them.

[laeubi]

I just wanted to mention that I don't want to overcomplicate the matter here and if its not working on windows it is fine for me
The same applies to the return value, I have never seen any use of that and it always returns true unless a mysterious "config change" in gtk.

[trancexpress]

Isn't the problem setting wake to true before this line with "bad" timing?

I wonder what this line is for... @akurtakov any idea? Maybe it was from a time before the timeout above was set? It seems very odd that Display.wake() would work only if timed to be within the looping 50 ms sleep...

[akurtakov]

TBH, I have no clue . This code has been there since the famous SWT HEAD reset commit so no clue where/why/how/what it is there for.

[HeikoKlare]

If I am not mistaken, the wake = false line was introduced together with the timeout in this commit: 400a419

[github-actions]

Test Results

  200 files  ±    0    200 suites  ±0   21m 9s ⏱️ - 9m 27s
4 888 tests +    1  4 864 ✅ +    1   24 💤 ± 0  0 ❌ ±0 
4 960 runs   - 2 080  4 857 ✅  - 2 015  103 💤  - 65  0 ❌ ±0 

Results for commit 99966a7. ± Comparison against base commit 7b3acfd.

♻️ This comment has been updated with latest results.

[Copilot]

Pull request overview

This PR addresses #3059 by making Display#sleep() on GTK and Cocoa stop blocking when the UI thread is interrupted, and adds a JUnit test to validate the behavior (skipping Windows due to WaitMessage() limitations).

Changes:

• GTK: exit the sleep() wait loop when the UI thread is interrupted and propagate interruption via the method’s return path.
• Cocoa: replace the “sleep forever” run-loop wait with a timeout-based loop to allow interruption checks.
• Tests: add a new unit test ensuring sleep() reacts to thread interruption (disabled on Windows).

Reviewed changes

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

File	Description
tests/org.eclipse.swt.tests/JUnit Tests/org/eclipse/swt/tests/junit/Test_org_eclipse_swt_widgets_Display.java	Adds regression test for interruption responsiveness in Display#sleep()
bundles/org.eclipse.swt/Eclipse SWT/gtk/org/eclipse/swt/widgets/Display.java	Updates GTK sleep() loop/return to account for thread interruption
bundles/org.eclipse.swt/Eclipse SWT/cocoa/org/eclipse/swt/widgets/Display.java	Implements timeout-based run-loop waiting to allow interruption checks

Comments suppressed due to low confidence (1)

bundles/org.eclipse.swt/Eclipse SWT/cocoa/org/eclipse/swt/widgets/Display.java:5226

• Returning only !thread.isInterrupted() can incorrectly report "no event" when the thread is interrupted at the same time an SWT message is queued (e.g., asyncExec/wake). This can violate the documented return semantics (“true if an event requiring dispatching was placed on the queue”). Consider returning true when the synchronizer queue is non-empty, even if the interrupt flag is set.

	return !thread.isInterrupted();