iscoroutinefunction returns False for wrapper functions with update_wrapper applied #100317

Activity

1. jonasBoss

   added

   type-bugAn unexpected behavior, bug, or errorAn unexpected behavior, bug, or error

   on Dec 17, 2022
2. jonasBoss

   changed the title [-] `iscoroutinefunction` returns False for wrapped functions with `update_wrapper` applied[/-] [+] `iscoroutinefunction` returns False for wrapper functions with `update_wrapper` applied[/+] on Dec 17, 2022
3. AlexWaygood

   added

   stdlibStandard Library Python modules in the Lib/ directoryStandard Library Python modules in the Lib/ directory

   on Dec 17, 2022
4. JosephSBoyle commented on Dec 18, 2022

   

   JosephSBoyle

   on Dec 18, 2022

   Last edited by JosephSBoyle

   Contributor

   More actions

   I agree that the behaviour you describe is what I would also expect.

   Searching for the phrase "def is coroutine" in the source I found this link:
   https://github.com/python/cpython/blob/f4c03484da59049eb62a9bf7777b963e2267d187/Misc/NEWS.d/3.5.0b3.rst

   " coroutines; inspect.iscoroutine no longer uses collections.abc.Coroutine... it's intended to test for pure 'async def' coroutines only;"

   Which might suggest that your use case is not supported. (I think it ought to be).
5. carljm commented on Dec 20, 2022

   

   carljm

   on Dec 20, 2022

   Member

   More actions

   #99247 recently added inspect.markcoroutinefunction which allows marking arbitrary functions as coroutine functions even if they were not defined with async def. This can be used manually to handle your situation:

   def signature_preserving_decorator(fn):
       @functools.wraps(fn)
       def wrapper(*args, **kwargs):
           return fn(*args, **kwargs)
       if inspect.iscoroutinefunction(fn):
          wrapper = inspect.markcoroutinefunction(wrapper)
       return wrapper
   

   But this is a lot of boilerplate.

   This does raise some questions for functools.wraps:

   1. (Narrow version) should it automatically transfer the "mark" from markcoroutinefunction, if present on the wrapped function, to the wrapper?
   2. (Broad version) should it automatically apply markcoroutinefunction to the wrapper anytime the wrapped function passes iscoroutinefunction?

   I think we should discard option 1 as breaking the contract of markcoroutinefunction: it is supposed to mean "treat me as if I were an async def," so we should not intentionally introduce cases where a function is treated differently if it is "marked" vs actual async def.

   Given that iscoroutinefunction already natively supports functools.partial wrappers around coroutine functions, it seems most consistent to me that functools.wraps would also transparently pass through iscoroutinefunction-ness (i.e. option 2). My only question here is backward-compatibility: option 2 would cause existing wrappers around async functions using functools.wraps to change behavior with iscoroutinefunction. We can only do this if we are confident the existing behavior is a bug, i.e. nobody would want the current behavior. I'm not entirely sure of this.

   CCing people actively involved in #99247 @carltongibson @kumaraditya303 @gvanrossum
6. gvanrossum commented on Dec 21, 2022

   

   gvanrossum

   on Dec 21, 2022

   Member

   More actions

   Is two lines really that much boilerplate? (If you need it enough you can write a helper that does it in one. :-)

   I worry there's a fundamental problem with automatically passing the "iscoroutinefunction" bit in functools.wraps(): what if the decorator is something that takes an async function and produces a sync version of it (e.g. by calling asyncio.run() on it)? I'm not a big user of wraps() myself, but from reading its docs (and those for update_wrapper()) it is far from clear that this would be an inappropriate use for those functions -- the docs mostly talk about things like preserving the function name and docstring.

   For this reason I'm not a fan of automatically applying markcoroutinefunction in wraps(). But maybe we could add a new keyword arg to it (and to update_wrapper()) that transfers the flag?

   I also note that wrapping an async function in a sync wrapper may not do the right thing. E.g. a wrapper like this typical example:

   def logcalls(func):
       def wrapper(*args, **kwds):
           print("args=", args)
           res = func(*args, **kwds)
           print("res=", res)
           return res
   

   would print the "result" immediately when a wrapped async function is called, not when it returns.

   👍React with 👍1Reacted by Carl Meyer
7. carltongibson commented on Dec 21, 2022

   

   carltongibson

   on Dec 21, 2022

   Last edited by carltongibson

   Contributor

   More actions

   Thanks @carljm.

   I have Django's various decorators in the queue for updating to support async views correctly. I think I'd expect that I'd have to write this version that you have (with the additional lines):

   def signature_preserving_decorator(fn):
       @functools.wraps(fn)
       def wrapper(*args, **kwargs):
           return fn(*args, **kwargs)
       if inspect.iscoroutinefunction(fn):
          wrapper = inspect.markcoroutinefunction(wrapper)
       return wrapper
   

   I note that if you define wrapper as async def, the example script passes (as expected, I guess):

   async def fn():
       pass
   
   def signature_preserving_decorator(fn):
       @functools.wraps(fn)
       async def wrapper(*args, **kwargs):
           return fn(*args, **kwargs)
   
       return wrapper
   
   assert inspect.iscoroutinefunction(signature_preserving_decorator(fn))
   

   So then we're in the exact use-case for markcoroutinefunction: I need (for reasons not given in this example, but Django's usage is wrapping both sync and async views) to have a sync def function that returns a coroutine object (but I can't just call it and use iscoroutine) so you need to trust me — In that case having to apply markcoroutinefunction by-hand seems reasonable.

   👍React with 👍1Reacted by Carl Meyer
8. gvanrossum

   closed this as not plannedon Dec 21, 2022