Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Asyncio and coroutine synchronization support? #236

Open
OnceUponATimeInAmerica opened this issue May 25, 2023 · 10 comments
Open

Asyncio and coroutine synchronization support? #236

OnceUponATimeInAmerica opened this issue May 25, 2023 · 10 comments

Comments

@OnceUponATimeInAmerica
Copy link

OnceUponATimeInAmerica commented May 25, 2023

Thanks for wrapt.

I was wondering if you are going to add support for synchronizing coroutines (using @synchronized decorator) and accepting asyncio Locks to wrapt? Currently it does not accept asyncio locks.
Also because ayncio model is mostly single-threaded but highly re-entrant, rlocks (which are more suitable for parallel scenarios) would not work (I think).

@GrahamDumpleton
Copy link
Owner

Can you explain better what you think it needs to do and what would need to change in the implementation? Is it just a matter of it it using asyncio.Lock instead threading.RLock at:

I wasn't even aware that asyncio had locks so know nothing about them and how they are used.

@OnceUponATimeInAmerica
Copy link
Author

OnceUponATimeInAmerica commented May 25, 2023

Thanks for the reply.
I have to warn I am just a user.

To begin, asycio locks are awaited (their acquire method is a coroutine and invoked by async with ...).

Also (I assume) they would not be reentrant as it would basically allow the single thread servicing the event loop (and executing most/all the coroutines in the program, perhaps even concurrently) to go basically anywhere without any restrictions.

And because wrapper of a coroutine itself needs to be a coroutine (a chain of await ...) the syntax has to accommodate this. If we want to use the same decorator for both synchronous functions and also coroutines, the decorator function (which itself, of course, is an ordinary synchronous function) has to detect the type (of the wrapped routine) and return the appropriate wrapper. Somewhat similar to this or this.

Also if we couple this to the present feature in wrapt that the same synchronized decorator can be applied to both instance methods and also to static methods/class methods or even standalone functions (basically, it automatically decides the granularity for synchronization) the situation can get somewhat complex. Although the former problem (routine's type: sync vs. async) is orthogonal to the latter problem (instance vs class method).

Hope this was somewhat helpful.

@mentalisttraceur
Copy link

Quick note on wrapping either sync-or-async, since I've done this in compose: the only universal way to do this, last I checked, is to wait until you had the return value - because an async callable might be implemented as a normal sync function returning an "awaitable" value.

@pbryan
Copy link

pbryan commented Oct 6, 2023

I think adapting wrapt.synchronized to handle to asyncio coroutines is doable. As @mentalisttraceur mentions, the wrapper will need to perform runtime introspection on the wrapped function to determine if it is a synchronous or coroutine function. Also, if the semaphore is passed, it too would need to be identified as having synchronous or coroutine methods. @GrahamDumpleton If you see wrapt.synchronized as having a solid future, I'd be willing to take a crack at adding asyncio support to it, if you want.

@GrahamDumpleton
Copy link
Owner

Even if you can come up with a separate asynchronized decorator just to see if it is possible for async alone that would certainly be educational and help work out what to do and whether one could come up with a single decorator that does both. I have not played enough with Python async libraries to be sure that anything I worked out would be correct. So any help/guidance would be much appreciated.

@pbryan
Copy link

pbryan commented Oct 6, 2023

OK, will work on it. Just paste here for now, or propose in a PR?

@GrahamDumpleton
Copy link
Owner

Just follow up here initially with any conclusions you come to. My feeling right now is that an asynchronized decorator is only going to cover a subset of what synchronized can be used for. The asynchronized decorator might even need to use the synchronized decorator, or more basic standard mutex locks, internally to gate updates to stuff.

@mentalisttraceur
Copy link

mentalisttraceur commented Oct 6, 2023

Correction:

As @mentalisttraceur mentions, the wrapper will need to perform runtime introspection on the wrapped function to determine if it is a synchronous or coroutine function.

... on the return value of the wrapped function, to determine if it is an awaitable or not.

My point was that inspecting the function (as with inspect.iscoroutinefunction or asyncio.iscoroutinefunction) is an incomplete solution. It will miss cases such as

  1. a callable class instance whose __call__ is async (to also detect those ahead-of-time, you gotta do inspect.iscoroutinefunction(f) or inspect.iscoroutinefunction(f.__call__));
  2. a function not declared async but which still needs an await because it returned an "awaitable" value (this is impossible to detect ahead-of-time, but can be detected at call time by calling the function synchronously and then doing an inspect.isawaitable on the result).

That last method will work 100% of the time in all cases, but it can be trickier (or in some cases impossible) to write the logic in a way that can only decide how to act after calling the possibly-async function.

Take a look at how I've got it implemented in acompose, and then once that makes sense, also scroll down to the sacompose variant (whose __call__ only sometimes returns an awaitable).

The downside of just using the isawaitable method for all cases is that we end up with a wrapper that will always be false for the iscoroutinefunction check - it loses the ahead-of-time check in cases where it could be possible.

So the ideal wrapper combines these technique. Here's an untested/incomplete sketch of what that might look like:

def wrapper(function):
    if inspect.iscoroutinefunction(function) or inspect.iscoroutinefunction(function.__call__):
        async def _wrapper(*args, **kwargs):
            async with lock:
                return function(*args, **kwargs)
    else:
        def _wrapper(*args, **kwargs):
            result = function(*args, **kwargs)
            if inspect.isawaitable(result):
                async def _finish():
                    async with lock:
                        return await result
                return _finish()  # `await` intentionally omitted
            else:
                return result
    return _wrapper

@GrahamDumpleton
Copy link
Owner

FWIW, old example which tries to deal with sync or async functions.

#150 (comment)

Based on comments above though, that too probably needs more work to be complete, but important in that example is that it still uses @wrapt.decorator so introspection works on the resulting wrapped function.

@mentalisttraceur
Copy link

mentalisttraceur commented Nov 2, 2023

!!! 3.12 added inspect.markcoroutinefunction, arguably wiping out all my objections.

I think this tips the balance strongly in favor of: if a function returns an awaitable object but doesn't pass inspect.iscoroutinefunction, that's a bug in that callable, not something you need to handle.

If I had to re-design acompose and sacompose today, in a world where every Python implementation has a way to make any callable pass inspect.iscoroutinefunction, I would probably never bother with all that at-call-time inspect.isawaitable stuff - if inspect.iscoroutinefunction(function) or inspect.iscoroutinefunction(function.__call__): would be good enough.

Personally, I think it's fine for wrapt to have async detection which is basically perfect on Python 3.12+ and only fails for an obscure niche edge case on older Pythons.

(In fact, it might be possible to backport inspect.markcoroutinefunction using ctypes or a C extension to older Python versions, at least for CPython.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants