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

[richardsheridan]

As suggested in #2392 (comment)

[codecov]

Codecov Report

Merging #2841 (0c6e63a) into master (3b6b167) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #2841   +/-   ##
=======================================
  Coverage   99.18%   99.18%           
=======================================
  Files         115      115           
  Lines       17596    17612   +16     
  Branches     3143     3149    +6     
=======================================
+ Hits        17453    17469   +16     
  Misses         99       99           
  Partials       44       44           

Files	Coverage Δ
trio/_socket.py	100.00% <ø> (ø)
trio/_subprocess_platform/waitid.py	70.73% <ø> (ø)
trio/_tests/test_threads.py	100.00% <100.00%> (ø)
trio/_threads.py	100.00% <100.00%> (ø)
trio/_wait_for_object.py	100.00% <ø> (ø)

[TeamSpen210]

That's a much more understandable name. It's not necessary, but we could use @overload to have type checkers prevent you from passing both parameters:

@overload
async def to_thread_run_sync(
    sync_fn: Callable[..., RetT],
    *args: object,
    thread_name: str | None = None,
    abandon_on_cancel: bool,
    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: ...

[jakkdl]

If cancellable is actually deprecated, as it's phrased in the newsfragment, then it should raise a DeprecationWarning if cancellable is specified.
Alternatives would be to forever support both arguments (yuck), or to make it deprecated later for some reason.

[richardsheridan]

I'd sooner abandon this pull request than start raising warnings everywhere. The extra maintenance burden is 3 easy lines of code on our end, but a warning will send our users scrambling to perform busywork. Someone else should add a warning in a future PR for a real reason, like cancellable will be removed in 1.0, at the time such a decision is made to balance user and maintainer burdens.

[oremanj]

This newsfragment should get a .removal. name (not .feature.) so it shows up in the "Deprecations and removals" section of the release notes.

I would write this as

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.

[richardsheridan]

Guess I'm outvoted here. Hope everything goes ok downstream!

[richardsheridan]

trio/_threads.py:295:9: B028 No explicit `stacklevel` keyword argument found

I guess I don't know at what level of the stack to warn?

And the mypy errors are utterly beyond me

[A5rocks]

Re: mypy errors

[oremanj]

I'd sooner abandon this pull request than start raising warnings everywhere. The extra maintenance burden is 3 easy lines of code on our end, but a warning will send our users scrambling to perform busywork. Someone else should add a warning in a future PR for a real reason, like cancellable will be removed in 1.0, at the time such a decision is made to balance user and maintainer burdens.

Sorry, I reviewed without having seen this comment or I would've responded to it initially. I have empathy for this view, but it doesn't match how we've handled all other deprecations in the past, and I think the old name is actively harmful/confusing under the new semantics. (cancellable=False threads are IMO "more cancellable" than cancellable=True threads, because cancellable=False threads might actually terminate their operation if the to_thread.run_sync call is cancelled, while cancellable=True threads definitely won't.) I think most users of to_thread.run_sync probably don't specify cancellable at all, and for those that do, the warning is a nice prompt to reconsider whether they still need abandon-on-cancel now that actually-cancel-on-cancel is available.

[A5rocks]

I'll merge this in soon-ish unless someone has other thoughts!

[jakkdl]

Sorry for being a pain in the butt, looks great!

[richardsheridan]

sorry thought the merge was a merge to master not from master!

[jakkdl]

sorry thought the merge was a merge to master not from master!

No worries! With the ongoing updates to linters etc I wanted to double check that this passes before merging even if GitHub deemed it a safe merge :)