`AsyncToSync` incorrectly passes arguments when called function arguments to internal wrapper

[Krismix1]

If the function being wrapped uses a kwarg that has the same name as the AsyncToSync.main_wrap function, it causes the execution to fail with AsyncToSync.main_wrap() got multiple values for argument '<kwarg>'.

How to reproduce - run the following example:

from asgiref.sync import async_to_sync

@async_to_sync
async def example(context):
    pass

example(context=1)

Stacktrace:

Traceback (most recent call last):
  File "example.py", line 9, in <module>
    example(context=1)
  File "<venv>/asgiref/sync.py", line 215, in __call__
    awaitable = self.main_wrap(
                ^^^^^^^^^^^^^^^
TypeError: AsyncToSync.main_wrap() got multiple values for argument 'context'

I have tested this on latest main, on 3.8.1, 3.7.2, 3.7.1, and 3.7.0 versions of asgiref.
From my investigation, the culprit is #390. I tested on the commit before that PR (fd7a63c) and the bug does not reproduce. So the last working version which does not exhibit the bug is 3.6.0.

[andrewgodwin]

Right, yes - the movement of the kwargs to actual **kwargs in order to make things type correctly would have indeed introduced this conflict.

I'd rather not undo the typing work so I suspect the only real approach here is to rename the kwargs in the main_wrap function to be less common - e.g. preceded by underscores.

[andrewgodwin]

Hmm, actually, on closer inspection I don't know why the kwargs were unpacked like that - I don't think it's necessary. Should be possible to revert it to the old way, I think.

@bellini666 Do you remember enough about this change to be able to comment either way?

[Krismix1]

I'll share some extra findings I had here, perhaps they will help.
I tried to change the signature of main_wrap from *args: _P.args, **kwargs: _P.kwargs to args: _P.args, kwargs: _P.kwargs, but my LSP (pyright) raises an error saying that ParamSpec must be used with the *args and **kwargs syntax. I am not familiar enough with the specification of ParamSpec, so I don't know if pyright is right or wrong. So it made me cautious about this approach.

We can also make use of functools.partial to generate a callable that takes no arguments and pass it to main_wrap. This removes the need to pass-through the arguments. As long as main_wrap does not need to modify the arguments (like for example injecting self or something), it may work.

diff --git a/asgiref/sync.py b/asgiref/sync.py
index 4427fc2..179bf48 100644
--- a/asgiref/sync.py
+++ b/asgiref/sync.py
@@ -212,13 +212,13 @@ class AsyncToSync(Generic[_P, _R]):
         # main event loop's thread if it's there, otherwise make a new loop
         # in this thread.
         try:
+            partial_fn = functools.partial(self.awaitable, *args, **kwargs)
             awaitable = self.main_wrap(
                 call_result,
                 sys.exc_info(),
                 task_context,
                 context,
-                *args,
-                **kwargs,
+                partial_fn,
             )
 
             if not (self.main_event_loop and self.main_event_loop.is_running()):
@@ -302,8 +302,7 @@ class AsyncToSync(Generic[_P, _R]):
         exc_info: "OptExcInfo",
         task_context: "Optional[List[asyncio.Task[Any]]]",
         context: List[contextvars.Context],
-        *args: _P.args,
-        **kwargs: _P.kwargs,
+        awaitable: Callable[[], Awaitable[_R]],
     ) -> None:
         """
         Wraps the awaitable with something that puts the result into the
@@ -326,9 +325,9 @@ class AsyncToSync(Generic[_P, _R]):
                 try:
                     raise exc_info[1]
                 except BaseException:
-                    result = await self.awaitable(*args, **kwargs)
+                    result = await awaitable()
             else:
-                result = await self.awaitable(*args, **kwargs)
+                result = await awaitable()
         except BaseException as e:
             call_result.set_exception(e)
         else:

[andrewgodwin]

OK, that confirms my suspicion of why it was changed, then - ParamSpec is slightly weird.

I don't see the reason not to wrap everything in a further partial except for potential performance concerns - we used to treat args and kwargs as opaque objects anyway, so I can't think of anything it would disrupt.

Do all the tests pass with your proposed change, and is the typechecker happy?

[bellini666]

Yes, as @Krismix1 discovered, the reason was the ParamSpec. It unfortunately requires *args/**kwargs.

I think the partial idea is the way to go, indeed, and I don't think it would have any performance implications as partials add close to nothing in terms of overhead (AFAIK they are even faster than lambdas)

Btw, my bad here for not noticing this at the time 🥲

[Krismix1]

@andrewgodwin yes, it passes the type checker and all tests, including a new test for this bug.

[andrewgodwin]

Let's get it in a PR and merge it then!

[Krismix1]

I'll prep a PR for it

[Krismix1]

PR #471 is up. I realized that we don't necessarily need to use functools.partial, as we could also just create the inner awaitable earlier. Tests pass, and I don't see any immediate downside to this approach.
But let me know if I missed something

[andrewgodwin]

I think that approach makes sense - merged it in! I'll give it a week for anyone who wants to early-test it then cut a release; if I forget and you remember, do remind me to!