coroutine wrapper reentrancy

[1st1]

BPO	24342
Nosy	@gvanrossum, @smontanaro, @ncoghlan, @vstinner, @ericsnowcurrently, @1st1
Files	coro_wrapper.patch

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields: ```python assignee = 'https://github.com/1st1' closed_at = created_at = labels = ['interpreter-core', 'type-bug'] title = 'coroutine wrapper reentrancy' updated_at = user = 'https://github.com/1st1' ``` bugs.python.org fields: ```python activity = actor = 'python-dev' assignee = 'yselivanov' closed = True closed_date = closer = 'yselivanov' components = ['Interpreter Core'] creation = creator = 'yselivanov' dependencies = [] files = ['39578'] hgrepos = [] issue_num = 24342 keywords = ['patch'] message_count = 10.0 messages = ['244569', '244577', '244585', '244593', '244594', '244595', '244621', '244635', '244706', '244724'] nosy_count = 8.0 nosy_names = ['gvanrossum', 'skip.montanaro', 'ncoghlan', 'vstinner', 'Arfrever', 'python-dev', 'eric.snow', 'yselivanov'] pr_nums = [] priority = 'normal' resolution = 'fixed' stage = 'resolved' status = 'closed' superseder = None type = 'behavior' url = 'https://bugs.python.org/issue24342' versions = ['Python 3.5', 'Python 3.6'] ```

[1st1]

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.

[smontanaro]

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?

[ericsnowcurrently]

Changing the title back. :)

[1st1]

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.

[smontanaro]

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.

[ericsnowcurrently]

@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.

[ncoghlan]

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).

[1st1]

Thanks, Nick! I'll commit the patch with your error message (it's much better!)

[1762cc99-3127-4a62-9baf-30c3d0f51ef7]

New changeset 19d613c2cd5f by Yury Selivanov in branch '3.5': bpo-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': bpo-24342: Let wrapper set by sys.set_coroutine_wrapper fail gracefully https://hg.python.org/cpython/rev/8a6db1679a23

[1762cc99-3127-4a62-9baf-30c3d0f51ef7]

New changeset d11cb1218489 by Yury Selivanov in branch '3.5': bpo-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': bpo-24342: No need to use PyAPI_FUNC for _PyEval_ApplyCoroutineWrapper https://hg.python.org/cpython/rev/b83fbc13ae1e