Support Cython-based async functions #550
Comments
|
Not sure how relevant but Cython changelog for 0.23.0 says:
So maybe the difference is with vanilla vs patched (Did you mean "Cython issue", not "CPython issue", right?) |
From my point of view the trouble comes from the fact CPython provide two Cython just choose the "wrong" coroutine style for
Really interesting point, this seems to indicate Cython patched both inspect and asyncio's iscoroutine, so both should work which is of course no the case... Maybe it's a Cython bug after all ^^ |
|
@scoder Do you have any insights here? It sounds like the issue is that |
As the docs say, Cython only patches these if the compiled module itself imports them directly. Since the compiled module only imports
Yes. The
It's probably what you want. The only reason to require exactly Python coroutine functions is when you depend on their C-level details or other implementation specifics. In all other cases, any object that supports the coroutine protocol should be enough. |
Ah, that is pretty confusing. I feel like I would either never patch, or else always patch
We don't care about the C-level details, but in general I think we assume that coroutine objects really implement the full coroutine interface, including the |
I prefer the word non-obvious, but yes. Any kind of stdlib monkey patching has the downside of being unexpected for some code. I'd rather not patch at all.
As much as they can. They don't have actual byte code, obviously, nor a frame, but they have a valid |
This is important because Cython has its own generator/coroutine types that are interchangeable with the ones built into the interpreter, but not (always) detectable by the inspect module. There's some more checks in trio/_core/_ki.py that still use the inspect module. Unfortunately, they're using the inspect.is*function forms, which are not easily replaceable by ABC checks. Fortunately, usage of @enable_ki_protection and @disable_ki_protection are very rare, so hopefully this won't affect too much? Fixes python-triogh-550.
|
Proposed fix in #560. I haven't actually tested it though :-). @touilleMan, would it be easy for you to try it out? |
|
Well, crud. @touilleMan tested my proposed fix, and it doesn't work (traceback and reproducer are here). The problem is that for functions passed to The problem arises as part of Trio's control-C handling: when a SIGINT arrives, the signal handler needs to figure out whether it's inside user code where it's safe to raise # Pretend this is regular Python code
def regular_python():
return 1 + 1
# Pretend this is Cython code
async def cython():
return regular_python()
trio.run(cython)Now let's assume the user hits control-C while the interpreter is in the middle of executing the body of :-( Now, this problem only happens for the top call inside a trio task. The thing you pass to from cython_module import cython_main
async def python_main(*args, **kwargs):
return await cython_main(*args, **kwargs)
trio.run(python_main)You could even wrap this in a decorator: # In a Python file:
def make_trio_friendly(cython_async_fn):
@functools.wraps(cython_async_fn)
async def wrapper(*args, **kwargs):
return await cython_async_fn(*args, **kwargs)
return wrapper
# In Cython:
from whatever import make_trio_friendly
@make_trio_friendly
async def main():
...For a real solution, then we would need some alternative way for Trio's SIGINT handler to detect whether the signal arrived in user code or Trio code. Some ideas: Cython could start creating stack frames for coroutines and making them visible to stack introspection. (Doesn't matter if Cython actually uses them for anything, they'd just need to exist.) We could somehow stash this information somewhere else? The tricky thing is that when we resume a user task, the "are we in user mode" state has to change atomically with the call to We could use some ad hoc thread local, and write an extension function in C/Cython whose only purpose is to toggle the thread local state and then invoke .....mayyybe the contextvars trick is the best option, and we would just tell people that they need to upgrade to 3.7 if they want to use Cython async functions? I'm not thinking of a simple easy solution here. |
This should never happen in normal use, but sometimes there are bugs that can cause the init task to crash. (For example, I hit this while trying to debug python-triogh-550.) Previously, this would trigger an assertion failure, and the original exception would be lost. Let's preserve the original exception to make debugging easier.
njsmith
changed the title
|
The contextvars idea actually has some problems. We can't use a ContextVar to track whether we're inside a protected generator frame, because PEP 568 isn't implemented -- and that's pretty important for using But... I guess we could detect when we've hit the top of a task stack, and at that point switch to checking a |
This is important because Cython has its own generator/coroutine types that are interchangeable with the ones built into the interpreter, but not (always) detectable by the inspect module. There's some more checks in trio/_core/_ki.py that still use the inspect module. Unfortunately, they're using the inspect.is*function forms, which are not easily replaceable by ABC checks. Fortunately, usage of @enable_ki_protection and @disable_ki_protection are very rare, so hopefully this won't affect too much? Unfortunately, this is not sufficient to support Cython – we also need to do something about how we handle KI protection. See python-triogh-550 for details. But this is still a good idea on its own.
|
I think I just hit this bug (on cython |
|
@kawing-chiu sorry to hear that! If you search the page for the string "workaround", there is one buried in the middle of one of the messages up above. It would be great to hear if it works for you. |
|
Great! I was tired enough to miss that last night. And the workaround does work. Thanks very much! |
|
Looking at this again, I suppose we could, like, automatically apply the workaround when this happens? |
|
environment: cython:0.29.23 and trio:0.18.0 fix code: |
|
@xueyoo, I backported the implementation of |
|
@scoder Thank you! This helped @didimelli in chat: https://gitter.im/python-trio/general?at=60a23cd601edef5b504d292f |
The title say it all, some projects re-implement their own iscoroutine function by looking for
CO_COROUTINEandCO_ITERABLE_COROUTINEflags (for instance) to address this...This is more of a CPython issue of course, but this currently means the coroutine generated by cython are not compatible with trio:
I didn't dig into CPython internal so my understanding on why
inspect.iscoroutine!=asyncio.iscoroutineon this is fairly limited, but I guess we should do something on our own to fix this anyway ;-)The text was updated successfully, but these errors were encountered: