In to_thread_run_sync(), add abandon_on_cancel= as an alias for the cancellable= flag

docs/source/reference-core.rst

@@ -1836,7 +1836,7 @@ to spawn a child thread, and then use a :ref:`memory channel
 
 You can also use :func:`trio.from_thread.check_cancelled` to check for cancellation from
 a thread that was spawned by :func:`trio.to_thread.run_sync`. If the call to
-:func:`~trio.to_thread.run_sync` was cancelled (even if ``abandon_on_cancel=False``!), then
+:func:`~trio.to_thread.run_sync` was cancelled, then
 :func:`~trio.from_thread.check_cancelled` will raise :func:`trio.Cancelled`.
 It's like ``trio.from_thread.run(trio.sleep, 0)``, but much faster.
 

newsfragments/2841.removal.rst

@@ -0,0 +1,8 @@
+To better reflect the underlying thread handling semantics,
+the keyword argument for `trio.to_thread.run_sync` that was
+previously called ``cancellable`` is now named ``abandon_on_cancel``.
+It still does the same thing -- allow the thread to be abandoned
+if the call to `trio.to_thread.run_sync` is cancelled -- but since we now
+have other ways to propagate a cancellation without abandoning
+the thread, "cancellable" has become somewhat of a misnomer.
+The old ``cancellable`` name is now deprecated.

trio/_tests/test_threads.py

@@ -337,7 +337,7 @@
         q.get()
         register[0] = "finished"
 
-    async def child(q, abandon_on_cancel):
+    async def child(q: stdlib_queue.Queue[None], abandon_on_cancel: bool) -> None:
         record.append("start")
         try:
             return await to_thread_run_sync(f, q, abandon_on_cancel=abandon_on_cancel)
@@ -400,8 +400,8 @@
         q1.get()
         q2.put(threading.current_thread())
 
-    async def main():
-        async def child():
+    async def main() -> None:
+        async def child() -> None:
             await to_thread_run_sync(thread_fn, abandon_on_cancel=True)
 
         async with _core.open_nursery() as nursery:
@@ -556,7 +556,7 @@
 
     # TODO: should CapacityLimiter have an abc or protocol so users can modify it?
     # because currently it's `final` so writing code like this is not allowed.
     await to_thread_run_sync(lambda: None, limiter=CustomLimiter())  # type: ignore[arg-type]
     assert record == ["acquire", "release"]
 
 
@@ -574,7 +574,7 @@
     bs = BadCapacityLimiter()
 
     with pytest.raises(ValueError) as excinfo:
         await to_thread_run_sync(lambda: None, limiter=bs)  # type: ignore[arg-type]
     assert excinfo.value.__context__ is None
     assert record == ["acquire", "release"]
     record = []
@@ -583,7 +583,7 @@
     # chains with it
     d: dict[str, object] = {}
     with pytest.raises(ValueError) as excinfo:
         await to_thread_run_sync(lambda: d["x"], limiter=bs)  # type: ignore[arg-type]
     assert isinstance(excinfo.value.__context__, KeyError)
     assert record == ["acquire", "release"]
 
@@ -884,15 +884,15 @@
     assert token is weak_reference()
 
 
-async def test_unsafe_abandon_on_cancel_kwarg():
+async def test_unsafe_abandon_on_cancel_kwarg() -> None:
     # This is a stand in for a numpy ndarray or other objects
     # that (maybe surprisingly) lack a notion of truthiness
     class BadBool:
         def __bool__(self) -> bool:
             raise NotImplementedError
 
     with pytest.raises(NotImplementedError):
-        await to_thread_run_sync(int, abandon_on_cancel=BadBool())
+        await to_thread_run_sync(int, abandon_on_cancel=BadBool())  # type: ignore[arg-type]
 
 
 async def test_from_thread_reuses_task() -> None:
@@ -979,7 +979,7 @@
 async def test_from_thread_check_cancelled() -> None:
     q: stdlib_queue.Queue[str] = stdlib_queue.Queue()
 
-    async def child(abandon_on_cancel, scope):
+    async def child(abandon_on_cancel: bool, scope: CancelScope) -> None:
         with scope:
             record.append("start")
             try:
@@ -1077,3 +1077,19 @@
         async with _core.open_nursery() as nursery:
             for _ in range(4):
                 nursery.start_soon(child)
+
+
+async def test_cancellable_and_abandon_raises() -> None:
+    with pytest.raises(ValueError):
+        await to_thread_run_sync(bool, cancellable=True, abandon_on_cancel=False)
+
+    with pytest.raises(ValueError):
+        await to_thread_run_sync(bool, cancellable=True, abandon_on_cancel=True)
+
+
+async def test_cancellable_warns() -> None:
+    with pytest.warns(DeprecationWarning):
+        await to_thread_run_sync(bool, cancellable=False)
+
+    with pytest.warns(DeprecationWarning):
+        await to_thread_run_sync(bool, cancellable=True)

trio/_threads.py

@@ -6,9 +6,10 @@
 import inspect
 import queue as stdlib_queue
 import threading
+import warnings
 from collections.abc import Awaitable, Callable
 from itertools import count
-from typing import Generic, TypeVar
+from typing import Generic, TypeVar, overload
 
 import attr
 import outcome
@@ -171,13 +172,35 @@
         token.run_sync_soon(self.run_sync)
 
 
+@overload
+async def to_thread_run_sync(
+    sync_fn: Callable[..., RetT],
+    *args: object,
+    thread_name: str | None = None,
+    abandon_on_cancel: bool = False,
+    limiter: CapacityLimiter | None = None,
+) -> RetT:
+    ...
+
+
+@overload
+async def to_thread_run_sync(
+    sync_fn: Callable[..., RetT],
+    *args: object,
+    thread_name: str | None = None,
+    cancellable: bool = False,
+    limiter: CapacityLimiter | None = None,
+) -> RetT:
+    ...
+
+
 @enable_ki_protection  # Decorator used on function with Coroutine[Any, Any, RetT]
 async def to_thread_run_sync(  # type: ignore[misc]
     sync_fn: Callable[..., RetT],
     *args: object,
     thread_name: str | None = None,
     abandon_on_cancel: bool | None = None,
-    cancellable: bool = False,
+    cancellable: bool | None = None,
     limiter: CapacityLimiter | None = None,
 ) -> RetT:
     """Convert a blocking operation into an async operation using a thread.
@@ -201,8 +224,6 @@
           arguments, use :func:`functools.partial`.
       abandon_on_cancel (bool): Whether to abandon this thread upon
           cancellation of this operation. See discussion below.
-      cancellable (bool): *Deprecated* synonym for ``abandon_on_cancel``.
-          Providing a value to ``abandon_on_cancel`` overrides this argument.
       thread_name (str): Optional string to set the name of the thread.
           Will always set `threading.Thread.name`, but only set the os name
           if pthread.h is available (i.e. most POSIX installations).
@@ -266,9 +287,20 @@
 
     """
     await trio.lowlevel.checkpoint_if_cancelled()
-    if abandon_on_cancel is not None:
-        cancellable = abandon_on_cancel
-    abandon_on_cancel = bool(cancellable)  # raise early if cancellable.__bool__ raises
+    if cancellable is not None:
+        if abandon_on_cancel is not None:
+            raise ValueError(
+                "Cannot set `cancellable` and `abandon_on_cancel` simultaneously."
+            )
+        warnings.warn(
+            DeprecationWarning(
+                "`cancellable` keyword argument is deprecated, "
+                "use `abandon on cancel` instead."
+            )
+        )
+        abandon_on_cancel = cancellable
+    # raise early if abandon_on_cancel.__bool__ raises
+    abandon_on_cancel = bool(abandon_on_cancel)
     if limiter is None:
         limiter = current_default_thread_limiter()