Fixed #535: ThreadSensitiveContext.__aexit__ blocking the event loop.

[helgefmi]

Fixes #535.

ThreadSensitiveContext.__aexit__ is async, but it calls executor.shutdown(), which blocks the thread (it's a Thread.join). Running a blocking call straight on the event loop can deadlock the whole loop, in this case when the executor's worker thread is itself waiting on that loop. When it happens the server stops responding to everything.

The fix runs the shutdown off the loop with await loop.run_in_executor(None, executor.shutdown), so the loop stays free. This is what asyncio itself does in loop.shutdown_default_executor(). I also moved the contextvar reset to before the await.

My test deadlocks on current main and passes with the change. It runs in a separate process because the hung thread would otherwise hang the test suite.

AI disclosure: I used Claude Opus 4.8 to help track down the deadlock, write the fix and the test.

[carltongibson]

Thanks for this @helgefmi. Just about to disappear on holiday, so will resolve this when I return 🏖️

[Arfey]

Nice, the new unit test makes the error obvious 👍

[Arfey]

Still working on the code review. Investigating a cancellation problem with nested async_to_sync

[Arfey]

Finished my investigation 😅 First, thanks again for the test case, it really helped.

Running executor.shutdown() in a separate thread works, but I think it treats the symptom rather than the cause. Let me walk through what I found.

We run sync code through sync_to_async, which schedules it on the executor via loop.run_in_executor. While the sync function is running, the task gets cancelled, so run_in_executor raises CancelledError on the loop side. But the sync function itself keeps running. It then calls async_to_sync, which does loop.call_soon_threadsafe(loop.create_task, ...) and blocks the executor thread waiting for that coroutine to complete.

Now the parent reaches __aexit__, calls executor.shutdown(), which is a Thread.join, and waits for the executor thread. But that thread is parked waiting on the event loop, and the loop is the very thing being blocked by shutdown(). Deadlock.

So the real issue: we "cancel" the sync function, but it doesn't actually stop, it runs on into async_to_sync and parks. await loop.run_in_executor(None, executor.shutdown) unblocks the loop so the queued create_task callback can drain and the thread eventually unparks. That fixes the hang.

I want to float a different angle: fix the cause instead. We can't truly cancel running sync code anyway, so instead of cancelling mid-flight, we let it run until it finishes or reaches the async_to_sync handoff, and only then propagate the CancelledError. I put a rough illustration in #567.

I don't have a strong opinion on which approach is better. await loop.run_in_executor(None, executor.shutdown) has a cost, but it's simple and keeps the existing cancellation behaviour. The second one fixes the root of the problem and is closer to how async should behave, but it changes the existing cancellation behaviour. 🤷‍♂️

Thoughts?

[helgefmi]

Thanks for your time digging into this @Arfey.

Honest disclosure: This is a bit over my paygrade. I hit the deadlock running https://github.com/django/daphne, used py-spy and Claude Code to work out what was happening, and it looked like the same issue @patx had already proposed a solution for in #535. So I don't want to pretend I have a strong architectural opinion here. You guys know asgiref's cancellation semantics far better than I do, and I'm happy to defer to whatever you think is best. 😊

[patx]

I think @Arfey has a very valid point here.

loop.run_in_executor(None, executor.shutdown) seems like a safe and practical fix for the deadlock, but I agree it does not really address the underlying cause. The sync callable has already been scheduled into a worker thread, and once it is running we cannot actually cancel it in the same way we can cancel an async task. So propagating CancelledError on the loop side while the sync function continues running can leave the system in this half-cancelled state, which is awkward for lack of a better word?

So delaying cancellation until the sync function either finishes or reaches the async_to_sync handoff seems conceptually cleaner to me. It reflects the reality that the sync code is still alive, and it avoids forcing shutdown code to compensate for work that was never actually stopped.

That said, I am in the same boat as @helgefmi and I do not know asgiref’s cancellation semantics deeply enough to judge the compatibility impact. If changing that behavior is acceptable for a core lib liek this, I would lean toward fixing the cancellation path rather than only moving executor.shutdown() off the event loop. But if the behavior change is too risky for a patch-level fix, the run_in_executor shutdown change still seems like a reasonable defensive improvement.

[carltongibson]

Just a quick comment:

Yes, @Arfey is right on this. (It's similar in kind to various Argh! Where did my event loop go? issues we've been working on recently...)

I'm working on a release now, and will look at the various suggested options here as part of that. (So we'll do something here.)

[Arfey]

@carltongibson Sounds good, thanks for picking this up! Happy to help test or take a piece of it if that's useful. just ping me.

[carltongibson]

Right, thanks for this @helgefmi and @patx.

I've pushed an amended version runs the shutdown in its own thread, rather then the default shared executor, which might itself be busy.

Whilst related, I think @Arfey's suggestion in #567 is orthogonal, and needs a little bit more work to pull in.

This will be part of the next release, which I'm working on at the moment.