classification
Title: coroutine wrapper reentrancy
Type: behavior Stage: resolved
Components: Interpreter Core Versions: Python 3.6, Python 3.5
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: yselivanov Nosy List: Arfrever, eric.snow, gvanrossum, ncoghlan, python-dev, skip.montanaro, vstinner, yselivanov
Priority: normal Keywords: patch

Created on 2015-06-01 02:11 by yselivanov, last changed 2015-06-03 02:31 by python-dev. This issue is now closed.

Files
File name Uploaded Description Edit
coro_wrapper.patch yselivanov, 2015-06-01 02:11 review
Messages (10)
msg244569 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2015-06-01 02:11
Consider following piece of code:

    async def foo():
        return 'spam'

    def wrapper(coro):
        async def wrap(coro):
            print('before')
            try:
                return await coro
            finally:
                print('after')
        return wrap(coro)

    import sys
    sys.set_coroutine_wrapper(wrapper)
    print(foo().send(None))


Current python will crash with a "RuntimeError: maximum recursion depth exceeded", because  "wrap" is itself a coroutine, so ceval will call "wrapper" recursively.

There are three options here:

1. Leave things as is;

2. Add a flag in tstate that coroutine_wrapper is executing, and raise a RuntimeError if it's reentering;

3. Add a flag in tstate (see 2) and skip wrapping when reentering (i.e. return what was passed to the wrapper).

The attached patch implements (2).  It also makes PyEval*CoroWrapper methods private.

I, myself, vote for option 2.
msg244577 - (view) Author: Skip Montanaro (skip.montanaro) * (Python triager) Date: 2015-06-01 12:20
On Sun, May 31, 2015 at 9:11 PM, Yury Selivanov <report@bugs.python.org> wrote:
> Current python will crash with a "RuntimeError: maximum recursion depth exceeded" ...

If it's good enough for other programmer-induced infinite recursion
(and in my experience the cause is generally quite obvious from the
traceback), why wouldn't it be good enough in this case?
msg244585 - (view) Author: Eric Snow (eric.snow) * (Python committer) Date: 2015-06-01 14:48
Changing the title back. :)
msg244593 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2015-06-01 15:35
>  why wouldn't it be good enough in this case?

Because it's highly non-obvious, it took me a while to understand what's *actually* going on.
msg244594 - (view) Author: Skip Montanaro (skip.montanaro) * (Python triager) Date: 2015-06-01 15:47
This is a bit off topic, but why did my reply to Yuri's ticket by email change the title? I didn't mess with the subject in my mail.
msg244595 - (view) Author: Eric Snow (eric.snow) * (Python committer) Date: 2015-06-01 15:54
@Skip, because roundup will change the title to the subject of the email and the title had been changed after the message to which you replied.
msg244621 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2015-06-02 00:04
Making sure I'm following the issue correctly here:

1. wrapper is a normal function, so there's no change for set_couroutine_wrapper() to detect anything might be amiss

2. any "async def" statement will call the registered coroutine wrapper to wrap the created function and turn it into a coroutine

3. this means any coroutine wrapper that directly or indirectly includes an "async def" statement will fail with RecursionError, without the problem being at all obvious

4. Yury's proposed patch effectively detects the use of "async def" inside a coroutine wrapper definition

I like the idea in principle, I don't like the error message in the current patch (since it only makes sense if you already understand the chain of reasoning above).

While it's a little verbose, I suggest an error like: "Coroutine wrapper %r attempted to recursively wrap %r", passing in the currently registered coroutine wrapper, and the code object we're attempting to wrap, respectively.

The latter repr gives the name, filename and line number of the offending code object, while the former should give the qualname of the registered wrapper.

The docs for set_coroutine_wrapper() should also be tweaked to note the constraint that the wrapper function cannot itself define new asynchronous functions (neither directly nor indirectly).
msg244635 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2015-06-02 02:07
Thanks, Nick!  I'll commit the patch with your error message (it's much better!)
msg244706 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2015-06-02 22:45
New changeset 19d613c2cd5f by Yury Selivanov in branch '3.5':
Issue 24342: Let wrapper set by sys.set_coroutine_wrapper fail gracefully
https://hg.python.org/cpython/rev/19d613c2cd5f

New changeset 8a6db1679a23 by Yury Selivanov in branch 'default':
Issue 24342: Let wrapper set by sys.set_coroutine_wrapper fail gracefully
https://hg.python.org/cpython/rev/8a6db1679a23
msg244724 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2015-06-03 02:31
New changeset d11cb1218489 by Yury Selivanov in branch '3.5':
Issue 24342: No need to use PyAPI_FUNC for _PyEval_ApplyCoroutineWrapper
https://hg.python.org/cpython/rev/d11cb1218489

New changeset b83fbc13ae1e by Yury Selivanov in branch 'default':
Issue 24342: No need to use PyAPI_FUNC for _PyEval_ApplyCoroutineWrapper
https://hg.python.org/cpython/rev/b83fbc13ae1e
History
Date User Action Args
2015-06-03 02:31:01python-devsetmessages: + msg244724
2015-06-02 22:47:26yselivanovsetstatus: open -> closed
resolution: fixed
stage: patch review -> resolved
2015-06-02 22:45:25python-devsetnosy: + python-dev
messages: + msg244706
2015-06-02 02:07:53yselivanovsetmessages: + msg244635
2015-06-02 00:04:11ncoghlansetmessages: + msg244621
2015-06-01 17:22:16Arfreversetnosy: + Arfrever
2015-06-01 15:54:10eric.snowsetmessages: + msg244595
2015-06-01 15:47:49skip.montanarosetmessages: + msg244594
2015-06-01 15:35:30yselivanovsetmessages: + msg244593
2015-06-01 14:48:55eric.snowsetnosy: + eric.snow

messages: + msg244585
title: coroutine wrapper recursion -> coroutine wrapper reentrancy
2015-06-01 12:20:16skip.montanarosetnosy: + skip.montanaro

messages: + msg244577
title: coroutine wrapper reentrancy -> coroutine wrapper recursion
2015-06-01 02:12:29yselivanovsettitle: coroutine wrapper recursion -> coroutine wrapper reentrancy
type: behavior
stage: patch review
2015-06-01 02:11:34yselivanovcreate