@enable_ki_protection breaks inspect.iscoroutinefunction

[jakkdl]

>>> from trio.lowlevel import enable_ki_protection
>>> async def foo():
...     return
... 
>>> @enable_ki_protection
... async def bar():
...     return
... 
>>> import inspect
>>> inspect.iscoroutinefunction(foo)
True
>>> inspect.iscoroutinefunction(bar)
False

Found when I tried to update coroutine_or_error in trio/_util.py to use inspect.iscoroutinefunction in #2668

trio/trio/_util.py

Lines 137 to 141 in 3146638

	# We can't check iscoroutinefunction(async_fn), because that will fail
	# for things like functools.partial objects wrapping an async
	# function. So we have to just call it and then check whether the
	# return value is a coroutine object.
	# Note: will not be necessary on python>=3.8, see https://bugs.python.org/issue34890

There are also a ton of wrapper/helper methods in tests that un-coroutinefunction-ify functions, which'd need to be modified in order to actually use iscoroutinefunction in couroutine_or_error. This means it'd probably also break a bunch of downstream code if coroutine_or_error was rewritten, but the upside of not having to call the function might mean that it's worth it (after a deprecation period)? That's probably for another issue though.

[A5rocks]

3.12+ has it easy: python/cpython#99247

I'm not sure about before that.

[A5rocks]

Here's some spooky stuff that works pre-3.12:

(.venv) PS C:\Users\A5rocks\Documents\trio> ipython
Python 3.11.3 (tags/v3.11.3:f3909b8, Apr  4 2023, 23:49:59) [MSC v.1934 64 bit (AMD64)]
Type 'copyright', 'credits' or 'license' for more information
IPython 7.34.0 -- An enhanced Interactive Python. Type '?' for help.

In [1]: import functools

In [2]: class Based(functools.partial):
   ...:     def __init__(self, func):
   ...:         super().__new__(functools.partial, func)
   ...:
   ...:     def __call__(self):
   ...:         print("insert logic to call `self.func` here")
   ...:

In [3]: @Based
   ...: async def async_function():
   ...:     ...
   ...:

In [4]: async_function
Out[4]: Based(<function async_function at 0x000001B09888FF60>)

In [5]: import inspect

In [6]: inspect.iscoroutinefunction(async_function)
Out[6]: True

In [7]: async_function()
insert logic to call `self.func` here

[jakkdl]

Nice. Could add something like that to the exported symbols so users can easily slap a decorator onto their non-coroutine wrapper functions and make the transition to using iscoroutinefunction in coroutine_or_error much easier.

[A5rocks]

Er, actually, I just realized my approach is way too complicated compared to what it could be: can't we just make the returned wrapper in enable_ki_protection async?

Oh yeah, it'll add a frame. But so will my voodoo class-based thing, annoying.

[oremanj]

I don't think iscoroutinefunction is ever going to be perfect, and I would be hesitant to rely on it in a core piece of Trio. Even if we make KI protection not mess it up, other wrappers/decorators still might. Calling the function and checking the type of its return value is the only way to be sure.

That said, the fact that KI protection adds extra frames is a performance problem also and I think there's plenty of room for the KI protection mechanisms to be improved, which would solve this issue as well. #733 has a bunch of previous thoughts on this.

[jakkdl]

I don't think iscoroutinefunction is ever going to be perfect, and I would be hesitant to rely on it in a core piece of Trio. Even if we make KI protection not mess it up, other wrappers/decorators still might. Calling the function and checking the type of its return value is the only way to be sure.

I don't think it needs to be perfect, but rather a matter of "downsides of calling the function when trio could've instantly errored" vs "forcing users with weird sync-looking functions to add a decorator/wrapper so it actually looks like a coroutine function". Could also add a flag to trio.run that skips the iscoroutinefunction check.

I don't think the gain is huge though, and don't really know how the above tradeoff plays out in practice.

[smurfix]

I have to admit that I habitually write sync wrappers to async functions. Semantically there's nothing whatsoever wrong with

async def _foo(n: float):
    await trio.sleep(n)

def foo():
    return _foo(42)
...
await foo()

That being said, the likelihood that I'd use this kind of decorator on a function that's passed to trio.main is … somewhat less than zero. Thus adding an @is_really_async decorator to foo would not actually affect anything.

Things would get more annoying if that check also happened when we call nursery.start, but I assume we agree that the overhead of doing that is too high.

[Zac-HD]

I propose that we have @enable_ki_protection apply @inspect.markcoroutinefunction on Python 3.12+, for the benefit of downstream introspection, and then consider this issue resolved.