Missing test case for demonstrating why SyncToAsync.get_current_task has to occur for thread_critical Locals?

[kezabelle]

My guess (given the complexity of juggling asyncio and threads is beyond my ken) is that there is a situation where, in Local._get_context_id, this bit is super important:

def _get_context_id(self):
    ...
    # First, pull the current task if we can
    context_id = SyncToAsync.get_current_task()
    context_is_async = True
    # OK, let's try for a thread ID
    if context_id is None:
        context_id = threading.current_thread()
        context_is_async = False
    if self._thread_critical:
        return context_id

Note how the presence of a current task is always checked first, falling back to the current thread if that's None and then returning the thread ID (or task ID?) immediately if it's thread critical.

But if I change it to:

def _get_context_id(self):
    if self._thread_critical:
        return threading.current_thread()
    from .sync import AsyncToSync, SyncToAsync
    ...

All the tests seem to pass, which is great if that's a valid change (because according to line profiler SyncToAsync.get_current_task is expensive, even ignoring #289), but I suspect it isn't valid, and is just missing from test coverage (which I'm wholly reliant on because I cannot fathom all of the underpinnings; As such, I'm incapable of writing a test to demonstrate it's importance, and I'm burdening someone else with doing so if appropriate, for which I apologise :))

[andrewgodwin]

I barely understand the logic behind all those tests most days, so you're not alone here!

I'm unsure if this is a valid optimisation or not; it might be, because thread_critical is commented as "Set thread_critical to True to not allow locals to pass from an async Task to a thread it spawns", so it would make sense that it entrely ignores the actual async Task and instead is instrumented entirely off the current thread ID.

The case I would want to work through to check, though, is this other comment: "Thread-critical code will still be differentiated per-Task within a thread as it is expected it does not like concurrent access." - so, maybe we're missing a Locals test that makes sure different tasks in the same thread/async loop get different values?