Implemented voluntary cancellation in worker threads

[agronholm]

No description provided.

[richardsheridan]

Docs impl and tests are good. I don't know what the exact implications of using _parent_cancelled() for this are, but any edge cases would need documentation rather than fixing.

Without implementing parent task reuse, there might be some divergence of from_thread behaviors between backends. Most users won't run into that though, so I won't suggest you hold a release over it.

[richardsheridan]

Allowing both aliases to be passed is a valid choice but departs from trio. Maybe also document/test which one overrides the other.

[zanieb]

I agree if both are passed it'd be nice to throw an error, but then you need to change the types a little 🤷‍♀️ happy with either implementation in practice since a warning will still be raised.

[agronholm]

Docs impl and tests are good. I don't know what the exact implications of using _parent_cancelled() for this are, but any edge cases would need documentation rather than fixing.

Without implementing parent task reuse, there might be some divergence of from_thread behaviors between backbends. Most users won't run into that though, so I won't suggest you hold a release over it.

Can you elaborate on the concept of parent task reuse?

[agronholm]

Docs impl and tests are good. I don't know what the exact implications of using _parent_cancelled() for this are, but any edge cases would need documentation rather than fixing.
Without implementing parent task reuse, there might be some divergence of from_thread behaviors between backbends. Most users won't run into that though, so I won't suggest you hold a release over it.

Can you elaborate on the concept of parent task reuse?

I think a snippet which demonstrates the differences between this PR and trio would be great.

[richardsheridan]

Sure, pulled from the test suite:

async def test_cancelscope_propagation():
    async def async_time_bomb():
        cancel_scope.cancel()
        with fail_after(1):
            await sleep(3)

    with CancelScope() as cancel_scope:
        await run_sync(from_thread_run, async_time_bomb)

    assert cancel_scope.cancelled_caught


async def test_from_thread_reuses_task():
    task = get_current_task()

    async def async_current_task():
        return get_current_task()

    assert task is await run_sync(from_thread_run_sync, get_current_task)
    assert task is await run_sync(from_thread_run, async_current_task)
    assert task is not await run_sync(from_thread_run_sync, get_current_task, abandon_on_cancel=True)
    assert task is not await run_sync(from_thread_run, async_current_task, abandon_on_cancel=True)

[richardsheridan]

@graingert pushed for it in the first place so maybe he can give some concrete use case?

python-trio/trio#2392 (comment)

[agronholm]

Right, so what happens here is Task 1 -> worker thread -> Task 2 which then cancels Task 1's scope, but Task 1 can't exit because it still waits for the worker thread. I think this was a good example and I may need to rework the implementation.

[agronholm]

I have a creeping feeling that implementing this for the trio backend could be very problematic.

[richardsheridan]

Right, so what happens here is Task 1 -> worker thread -> Task 2 which then cancels Task 1's scope, but Task 1 can't exit because it still waits for the worker thread. I think this was a good example and I may need to rework the implementation.

Well on Trio it's implemented as Task 1 -> worker thread -> Task 1 if abandon_on_cancel==False. This means Task 1 indeed cancels itself, but also unwinds the thread and waits for that unwinding to complete. You could implement it as Task 1 -> worker thread -> Task 2 as long as Task 2 is subject to the CancelScope.

Again, depending on how long you think it might take to do this on both backends, it might be a higher priority to release as-is and save parent task reuse for the next version.

[agronholm]

I've been scratching my head trying to figure out why test_cancelscope_propagation() doesn't work as expected with the trio backend. If I use trio.from_thread.run() to run the nested task, it works as expected, but not if I use anyio.from_thread.run(). This is unexpected because behind the scenes, anyio.from_thread.run() also calls trio.from_thread.run()!

[agronholm]

I've been scratching my head trying to figure out why test_cancelscope_propagation() doesn't work as expected with the trio backend. If I use trio.from_thread.run() to run the nested task, it works as expected, but not if I use anyio.from_thread.run(). This is unexpected because behind the scenes, anyio.from_thread.run() also calls trio.from_thread.run()!

Oh yeah, I noticed that if I pass the trio token explicitly via trio_token= to trio.from_thread_run(), it fails to work as expected. Why? I checked that in the absence of an explicit task, it automatically retrieves the same token from the PARENT_TASK_DATA threadlocal! As a matter of fact, if I remove the trio_token kwarg from the call in TrioBackend, the test suite passes anyway, so I might just do that, but I want to understand why this is happening.

[agronholm]

Ohh...now I see. If an explicit token is provided, it always runs the new task in the system nursery, even if there is a parent task.

[richardsheridan]

Ohh...now I see. If an explicit token is provided, it always runs the new task in the system nursery, even if there is a parent task.

Yeah I view that both as consistency (passing trio_token always does the same thing) and as an optional switch (to get the old behavior where you don't abandon but still use a system task)

[agronholm]

I've made some progress on implementing the cancellation semantics for async tasks spawned from worker threads, but it broke some previous tests and I'm still sorting that out.

[agronholm]

Ok, this should have feature parity with Trio now. I have to admit that I don't fully understand why my latest fix worked 😅 but it does.

[richardsheridan]

I think the tests make a lot of sense, so that makes it easy to trust the implementation, even though that part is relatively confusing.

[zanieb]

Won't this break the chain of any exceptions attached to the CancelledError? I guess that's probably rare.

[agronholm]

I'm not sure if the chain is preserved anyway, but I can check if you like.

[agronholm]

I checked and it doesn't seem to matter. There's a C level black box between that reraise and the calling synchronous code.