Repository: Textualize/textual
Pull request: Fix win sleep bug
Fixes #2711 

Problem statement:
On Windows `set_interval` blocks a full exit of the application until the next interval fires
Take this code as an example:

```python
from textual.app        import App, ComposeResult
from textual.containers import Center
from textual.reactive   import var
from textual.widgets    import Label

class WindowsIntervalBugApp( App[ None ] ):

    CSS = """
    Screen {
        align: center middle;
    }
    """

    counter: var[int] = var(0)

    def compose( self ) -> ComposeResult:
        yield Center( Label() )

    def watch_counter( self ) -> None:
        self.query_one( Label ).update( str( self.counter ) )

    def bump_counter( self ) -> None:
        self.counter += 1

    def on_mount( self ) -> None:
        self.set_interval( 30, self.bump_counter )

if __name__ == "__main__":
    WindowsIntervalBugApp().run()
```

When run on macOS, if you run it up and then <kbd>Ctrl</kbd>+<kbd>C</kbd> right away, the application instantly exits. On Windows, the display of the application will close down but you don't get your shell prompt back for around 30 seconds, minus the time it took you to <kbd>Ctrl</kbd>+<kbd>C</kbd>.

I've not tested this yet but I suspect the same would be true with `set_timer` as well.

Hints from issue discussion:
We need to implement a cancelable timer on Windows.

Talk to @willmcgugan before attempting this.

Commit message: Implement cancellable Windows sleep.

Related issues: #2711.

Patch to review:
diff --git a/src/textual/_time.py b/src/textual/_time.py
index 7e24cef109..985744f2ff 100644
--- a/src/textual/_time.py
+++ b/src/textual/_time.py
@@ -1,5 +1,5 @@
 import platform
-from asyncio import get_running_loop
+import asyncio
 from asyncio import sleep as asyncio_sleep
 from time import monotonic, perf_counter
 
@@ -26,7 +26,7 @@ async def sleep(secs: float) -> None:
         Args:
             secs: Number of seconds to sleep for.
         """
-        await get_running_loop().run_in_executor(None, win_sleep, secs)
+        await asyncio.create_task(win_sleep(secs))
 
 else:
 
diff --git a/src/textual/_win_sleep.py b/src/textual/_win_sleep.py
index ae24002426..87c690bf10 100644
--- a/src/textual/_win_sleep.py
+++ b/src/textual/_win_sleep.py
@@ -4,7 +4,9 @@
 This should only be imported on Windows.
 """
 
+import asyncio
 from time import sleep as time_sleep
+from typing import Coroutine
 
 __all__ = ["sleep"]
 
@@ -14,16 +16,26 @@
 CREATE_WAITABLE_TIMER_HIGH_RESOLUTION = 0x00000002
 TIMER_ALL_ACCESS = 0x1F0003
 
+
+async def time_sleep_coro(secs: float):
+    """Coroutine wrapper around `time.sleep`."""
+    time_sleep(secs)
+
+
 try:
     import ctypes
-    from ctypes.wintypes import LARGE_INTEGER
+    from ctypes.wintypes import HANDLE, LARGE_INTEGER
 
     kernel32 = ctypes.windll.kernel32  # type: ignore[attr-defined]
 except Exception:
-    sleep = time_sleep
+    print("Exception found")
+
+    def sleep(secs: float) -> Coroutine[None, None, None]:
+        return time_sleep_coro(secs)
+
 else:
 
-    def sleep(secs: float) -> None:
+    def sleep(secs: float) -> Coroutine[None, None, None]:
         """A replacement sleep for Windows.
 
         Note that unlike `time.sleep` this *may* sleep for slightly less than the
@@ -37,32 +49,62 @@ def sleep(secs: float) -> None:
         sleep_for = max(0, secs - 0.001)
         if sleep_for < 0.0005:
             # Less than 0.5ms and its not worth doing the sleep
-            return
+            return time_sleep_coro(0)
 
-        handle = kernel32.CreateWaitableTimerExW(
+        timer = kernel32.CreateWaitableTimerExW(
             None,
             None,
             CREATE_WAITABLE_TIMER_HIGH_RESOLUTION,
             TIMER_ALL_ACCESS,
         )
-        if not handle:
-            time_sleep(sleep_for)
-            return
-
-        try:
-            if not kernel32.SetWaitableTimer(
-                handle,
-                ctypes.byref(LARGE_INTEGER(int(sleep_for * -10_000_000))),
-                0,
-                None,
-                None,
-                0,
+        if not timer:
+            return time_sleep_coro(sleep_for)
+
+        if not kernel32.SetWaitableTimer(
+            timer,
+            ctypes.byref(LARGE_INTEGER(int(sleep_for * -10_000_000))),
+            0,
+            None,
+            None,
+            0,
+        ):
+            kernel32.CloseHandle(timer)
+            return time_sleep_coro(sleep_for)
+
+        cancel_event = kernel32.CreateEventExW(None, None, 0, TIMER_ALL_ACCESS)
+        if not cancel_event:
+            kernel32.CloseHandle(timer)
+            return time_sleep_coro(sleep_for)
+
+        def cancel_inner():
+            kernel32.SetEvent(cancel_event)
+
+        async def cancel():
+            """Cancels the timer by setting the cancel event."""
+            await asyncio.get_running_loop().run_in_executor(None, cancel_inner)
+
+        def wait_inner():
+            """Function responsible for waiting for the timer or the cancel event."""
+            if (
+                kernel32.WaitForMultipleObjects(
+                    2,
+                    ctypes.pointer((HANDLE * 2)(cancel_event, timer)),
+                    False,
+                    INFINITE,
+                )
+                == WAIT_FAILED
             ):
-                kernel32.CloseHandle(handle)
                 time_sleep(sleep_for)
-                return
 
-            if kernel32.WaitForSingleObject(handle, INFINITE) == WAIT_FAILED:
-                time_sleep(sleep_for)
-        finally:
-            kernel32.CloseHandle(handle)
+        async def wait():
+            """Wraps the actual sleeping so we can detect if the thread was cancelled."""
+            try:
+                await asyncio.get_running_loop().run_in_executor(None, wait_inner)
+            except asyncio.CancelledError:
+                await cancel()
+                raise
+            finally:
+                kernel32.CloseHandle(timer)
+                kernel32.CloseHandle(cancel_event)
+
+        return wait()
diff --git a/tests/test_win_sleep.py b/tests/test_win_sleep.py
new file mode 100644
index 0000000000..b18b5a991f
--- /dev/null
+++ b/tests/test_win_sleep.py
@@ -0,0 +1,41 @@
+from functools import partial
+import time
+import sys
+
+import pytest
+
+from textual._win_sleep import sleep
+from textual.app import App
+
+pytestmark = pytest.mark.skipif(
+    sys.platform != "win32", reason="We only need to test this on Windows."
+)
+
+
+@pytest.mark.parametrize("sleep_for", [0.1, 1, 10])
+async def test_win_sleep_duration(sleep_for):
+    start = time.perf_counter()
+    await sleep(sleep_for)
+    end = time.perf_counter()
+    assert end - start == pytest.approx(sleep_for, abs=min(sleep_for, 0.1))
+    assert (end - start) < min(sleep_for, 0.1) + sleep_for
+    assert end - start
+
+
+@pytest.mark.parametrize("sleep_for", [0.1, 1, 10])
+async def test_win_sleep_timer_is_cancellable(sleep_for):
+    """Regression test for https://github.com/Textualize/textual/issues/2711."""
+
+    class WindowsIntervalBugApp(App[None]):
+        def on_mount(self) -> None:
+            self.set_interval(sleep_for, lambda: None)
+
+        def key_e(self):
+            self.exit()
+
+    async with WindowsIntervalBugApp().run_test() as pilot:
+        start = time.perf_counter()
+        await pilot.press("e")
+        end = time.perf_counter()
+
+    assert (end - start) < 0.05


Code review comments:
diff_hunk: @@ -37,32 +49,62 @@ def sleep(secs: float) -> None:
         sleep_for = max(0, secs - 0.001)
         if sleep_for < 0.0005:
             # Less than 0.5ms and its not worth doing the sleep
-            return
+            return time_sleep_coro(0)
path: src/textual/_win_sleep.py
text: @user1:
If this isn't run in a thread context somewhere, it will block all asyncio tasks from running.

If this is ultimately calling `time.sleep(0)` then it could end up sleeping for several milliseconds. `sleep(0)` basically means give up your time slice and go run another thread. 

@author:
How about returning `None` in this case and then checking for it in `_time`?
See 0c32c05.

@user1:
You could just return a coroutine that does nothing. 

@author:
Done e3f9dc1.