classification
Title: contextlib.ContextDecorator decorating async functions
Type: enhancement Stage:
Components: Library (Lib) Versions: Python 3.9
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: John Belmonte, asvetlov, ncoghlan, njs, xtreak, yselivanov
Priority: normal Keywords:

Created on 2019-06-25 06:50 by John Belmonte, last changed 2019-08-12 15:12 by yselivanov.

Messages (15)
msg346485 - (view) Author: John Belmonte (John Belmonte) Date: 2019-06-25 06:50
A case came up where I'd like a non-async context manager to be able to decorate both regular and async functions.  I wonder why contextlib.ContextDecorator doesn't just support both cases.

The implementation seems straightforward, switching on iscoroutinefunction().
msg346491 - (view) Author: Andrew Svetlov (asvetlov) * (Python committer) Date: 2019-06-25 08:19
I wonder why do you need it? What is your use case?
A code snippet can help a lot.
msg346509 - (view) Author: John Belmonte (John Belmonte) Date: 2019-06-25 11:14
I have a context manager used for timing sections of code or whole functions (when used as a decorator).  It's aware of the Trio async/await scheduler and is able to exclude periods of time where the task is not executing.  The context manager itself is not async.

Synopsis of decorator case:

  @PerfTimer('query')
  async def query_or_throw(self, q):
      return _parse_result(await self._send_query(q))

I'd like to just use the existing ContextDecorator to define the PerfTimer context manager, and have it wrap coroutine functions properly.  New API is not required.
msg346511 - (view) Author: Andrew Svetlov (asvetlov) * (Python committer) Date: 2019-06-25 11:21
Sorry, I still have no idea what is the code for PerfTimer class.
msg346513 - (view) Author: John Belmonte (John Belmonte) Date: 2019-06-25 11:35
PerfTimer is a reusable, non-reentrant context manager implemented as a class.

There are already use cases of ContextDecorator described in the contextlib documentation, such as the track_entry_and_exit logger.  What reason would you have for such context managers to *not* wrap async functions properly?
msg346517 - (view) Author: Andrew Svetlov (asvetlov) * (Python committer) Date: 2019-06-25 12:17
Got you.

iscoroutinefunction() is not reliable: it detects async functions only but fails if we have a regular function that returns awaitable object.

I think AsyncContextDecorator is needed here to explicitly point that you want to wrap async function.

Another question is: sync or async context manager should be applied? I see use cases for both variants
msg346519 - (view) Author: John Belmonte (John Belmonte) Date: 2019-06-25 12:36
My use case is for a non-async context manager, and if we take track_entry_and_exit as an example that's non-async as well.

The explicit approach (e.g. separate base class for decorators applied to sync vs. async functions) doesn't seem ideal, because it precludes having a single context manager for both cases.  track_entry_and_exit is an example where a context manager would want to decorate both types of functions.

I'm not sure how big of a problem the iscoroutinefunction() limitation is-- in Trio style we don't pass around coroutines by normal functions.  The `async` qualifier exists to make it clear when a function returns a coroutine, and ContextDecorator already doesn't work for the case of a regular function returning a coroutine.  I think the scope here is to enhance ContextDecorator to work with async functions which are properly qualified with `async`.
msg348426 - (view) Author: John Belmonte (John Belmonte) Date: 2019-07-25 06:22
I was caught by this again, indirectly via @contextmanager.

If I define a non-async context manager using @contextmanager, I expect the decorator support to work on async functions.  I.e. the following should be equivalent:

```
async def foo():
    with non_async_cm:
        ...
```
```
@non_async_cm
async def foo():
    ...
```

Not only does the 2nd case not work, but the decorator is (effectively) silently ignored :(
msg348427 - (view) Author: Nathaniel Smith (njs) * (Python committer) Date: 2019-07-25 06:40
There are two different axes of sync/async here, and it's important not to mix them up: the context manager could be sync or async, and the function that's being decorated could be sync or async. This issue is about the case where you have a sync context manager, and it's decorating an async function.

The current behavior is definitely super confusing and not at all useful.

In general, I'm also very skeptical about using iscoroutinefunction to automatically change semantics, because of the issue Andrew mentions about failing to handle complicated objects like functools.partial instances or sync functions that return awaitables. In this *specific* case though, I'm not sure... maybe it actually is OK?

What's special about this case is that when you write:

@sync_cm()
async def foo():
    ...

...the same person is writing both the @ line and the 'async def' line, and they're right next to each other in the source code. So the normal concern about some unexpected foreign object being passed in doesn't really apply. And in fact, if I did have an awaitable-returning-function:

@sync_cm()
def foo():
    return some_async_fn()

...then I think most people would actually *expect* this to be equivalent to:

def foo():
    with sync_cm():
        return some_async_fn()

Not very useful, but not surprising either. So maybe this is the rare case where switching on iscoroutinefunction is actually OK? The only cases where it would fail is if someone invokes the decorator form directly, e.g.

new_fn = sync_cm()(old_fn)

This seems pretty rare, but probably someone does do it, so I dunno.

If we think that's too magic, another option would be to make up some more explicit syntax, like:

@sync_cm().wrap_async
async def foo(): ...

...Oh, but I think this breaks the Guido's gut feeling rule (https://mail.python.org/pipermail/python-dev/2004-August/046711.html), so it would have to instead be:

@sync_cm.wrap_async()
async def foo(): ...

And even if we decide not to automatically switch on iscoroutinefunction, we should probably add a warning or something at least, because right now

@sync_cm()
async def ...

is definitely a mistake.

(Adding Nick to nosy as the contextlib maintainer.)
msg348434 - (view) Author: Andrew Svetlov (asvetlov) * (Python committer) Date: 2019-07-25 11:29
I still think that iscoroutinefunction should not be used for async/sync decoration type autodetection.

A day ago stuck with a case when decorator used iscoroutinefunction() for separation but returned a regular function wrapper that returns awaitable in turn.

As a result, the transparent decorator which should not modify a decorated function signature cannot be applied twice, e.g.

@retry
@retry
async def fetch(): 
    ...

This is very confusing behavior, the obvious fix is providing `sync_retry` and `async_retry` decorators to never mix the behavior.

Seems very close to the problem discussed here.
msg349300 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2019-08-09 17:19
I wouldn't be OK with magic switching in the behaviour of ContextDecorator (that's not only semantically confusing, it's also going to make the contextlib function wrappers even slower than they already are).

I'm also entirely unclear on what you would expect a synchronous context manager to do when applied to an asynchronous function, as embedding an "await" call inside a synchronous with statement is unlikely to end well.
msg349313 - (view) Author: Nathaniel Smith (njs) * (Python committer) Date: 2019-08-09 20:18
> I wouldn't be OK with magic switching in the behaviour of ContextDecorator (that's not only semantically confusing, it's also going to make the contextlib function wrappers even slower than they already are).

I hear you on the semantic confusion, but is a single check at definition time really that expensive? The runtime cost is zero.

> I'm also entirely unclear on what you would expect a synchronous context manager to do when applied to an asynchronous function, as embedding an "await" call inside a synchronous with statement is unlikely to end well.

It would be like:

async def blah():
    with something():
        await foo()

There's nothing weird about this; people write the long version all the time. You'd only do it when 'something()' doesn't do I/O, but there are lots of context managers that don't do I/O.
msg349314 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2019-08-09 20:25
> I hear you on the semantic confusion, but is a single check at definition time really that expensive? 

Do you know how to make that single check 100% reliable?
msg349316 - (view) Author: Nathaniel Smith (njs) * (Python committer) Date: 2019-08-09 20:32
@Yury: depends on what you mean by "100% reliable" :-). Like I said above, I'm normally super against automagic detection of sync-vs-async functions because of all the edge cases where it goes wrong, but in this specific case where people are writing a decorator one line above their def/async def, I think a simple iscoroutinefunction check will be pretty close to 100% matching what users expect.

Or, if we don't change the semantics, then we can still be 100% confident that if iscoroutinefunction returns true, then the user has made a mistake. (I.e., if we make this issue a warning, then it's possible we'll miss print a warning in some complicated cases, but we can be confident that all the warnings we do print are correct.)
msg349474 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2019-08-12 15:12
> I think a simple iscoroutinefunction check will be pretty close to 100% matching what users expect.

Yes, "pretty close", but not reliable. :)  E.g. I can easily design a decorator that when applied first would break the proposed iscoroutinefunction logic.  While decorators like that aren't popular, I'd be wary about introducing a solution that can lead to hours of debugging in some weird (and maybe stupid) cases.

> Or, if we don't change the semantics, then we can still be 100% confident that if iscoroutinefunction returns true, then the user has made a mistake. (I.e., if we make this issue a warning, then it's possible we'll miss print a warning in some complicated cases, but we can be confident that all the warnings we do print are correct.)

+1 to implement a warning the way you suggest.
History
Date User Action Args
2019-08-12 15:12:06yselivanovsetmessages: + msg349474
2019-08-09 20:32:22njssetmessages: + msg349316
2019-08-09 20:25:51yselivanovsetmessages: + msg349314
2019-08-09 20:18:19njssetmessages: + msg349313
2019-08-09 17:19:17ncoghlansetmessages: + msg349300
2019-07-25 11:29:02asvetlovsetmessages: + msg348434
2019-07-25 06:40:43njssetnosy: + ncoghlan, njs
messages: + msg348427
2019-07-25 06:22:48John Belmontesetmessages: + msg348426
2019-06-25 12:36:43John Belmontesetmessages: + msg346519
2019-06-25 12:17:15asvetlovsetmessages: + msg346517
2019-06-25 11:35:22John Belmontesetmessages: + msg346513
2019-06-25 11:21:42asvetlovsetmessages: + msg346511
2019-06-25 11:14:11John Belmontesetmessages: + msg346509
2019-06-25 08:19:31asvetlovsetmessages: + msg346491
2019-06-25 07:14:27xtreaksetnosy: + asvetlov, yselivanov, xtreak

versions: + Python 3.9
2019-06-25 06:50:03John Belmontecreate