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

Deprecate sys.set_coroutine_wrapper and replace it with more focused API(s) #76772

Closed
njsmith opened this issue Jan 18, 2018 · 21 comments
Closed

Deprecate sys.set_coroutine_wrapper and replace it with more focused API(s) #76772

njsmith opened this issue Jan 18, 2018 · 21 comments
Labels
3.7 expert-asyncio interpreter-core type-feature

Comments

@njsmith
Copy link
Contributor

@njsmith njsmith commented Jan 18, 2018

BPO 32591
Nosy @gvanrossum, @Yhg1s, @vstinner, @giampaolo, @njsmith, @asvetlov, @1st1
PRs
  • #5250
  • #5263
  • #5337
  • #5412
  • #6536
  • #19247
  • 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:

    assignee = None
    closed_at = <Date 2018-05-28.16:48:02.601>
    created_at = <Date 2018-01-18.07:09:04.944>
    labels = ['interpreter-core', 'type-feature', '3.7', 'expert-asyncio']
    title = 'Deprecate sys.set_coroutine_wrapper and replace it with more focused API(s)'
    updated_at = <Date 2020-03-31.15:25:21.173>
    user = 'https://github.com/njsmith'

    bugs.python.org fields:

    activity = <Date 2020-03-31.15:25:21.173>
    actor = 'vstinner'
    assignee = 'none'
    closed = True
    closed_date = <Date 2018-05-28.16:48:02.601>
    closer = 'yselivanov'
    components = ['Interpreter Core', 'asyncio']
    creation = <Date 2018-01-18.07:09:04.944>
    creator = 'njs'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 32591
    keywords = ['patch']
    message_count = 21.0
    messages = ['310226', '310250', '310268', '310299', '310355', '310364', '310375', '310391', '310726', '310736', '310739', '310781', '310782', '310783', '310784', '311067', '311069', '311070', '311077', '318281', '365391']
    nosy_count = 7.0
    nosy_names = ['gvanrossum', 'twouters', 'vstinner', 'giampaolo.rodola', 'njs', 'asvetlov', 'yselivanov']
    pr_nums = ['5250', '5263', '5337', '5412', '6536', '19247']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue32591'
    versions = ['Python 3.7']

    @njsmith
    Copy link
    Contributor Author

    @njsmith njsmith commented Jan 18, 2018

    sys.set_coroutine_wrapper is a problematic API. It was added to solve a specific problem: asyncio debug mode wanting to track where coroutine objects are created, so that when unawaited coroutines are GC'ed, the warning they print can include a traceback. But to do this, it adds a *very* generic hook that lets you hook into the coroutine creation code to replace all instances of a built-in type with an arbitrary new type, which is highly unusual. It's hard to use correctly (see bpo-30578, and bpo-24342 before it). It's hard to use without creating performance problems, since anything you do here tends to add overhead to every async function call, and in many cases also every async function frame suspend/resume cycle.

    Fortunately, it's explicitly documented as provisional. (I think Yury intentionally excluded it from the stabilization of the rest of asyncio and async/await, because of the reasons above.) And the things we *actually* want to do here are very simple. The only known use cases are the one mentioned above (see asyncio.coroutines.CoroWrapper), and the one in bpo-30491. So after discussions with Yury, I'm proposing to migrate those both into the interpreter as directly usable, fast, built-in features that can be used individually or together, and deprecate sys.set_coroutine_wrapper.

    See bpo-30491 for details on that use case; here I'll discuss my plan for replacing the "coroutine origin tracking" that asyncio debug mode does.

    We add a new function sys.set_coroutine_origin_tracking(depth), and I guess sys.get_coroutine_origin_tracking() because why not. The depth is thread-local, and defaults to 0, which means that newly created coroutines don't capture any origin info.

    When a coroutine is created, it will check the current origin_tracking depth, and capture that many frames of traceback information. Like the current asyncio debug code and unlike real tracebacks, we won't capture actual frame objects -- we'll just capture (filename, lineno, function) information, to avoid pinning objects in memory. This is a debug helper, so it should avoid changing semantics whenever possible.

    Later, we can retrieve the captured information by accessing the new attribute coro.origin.

    In addition, the code in CoroutineType.__del__ that currently emits a warning for unawaited coroutines, will be modified to check for whether 'self' has origin information, and print it if so, similar to the current asyncio debug wrapper.

    Some particular points where I'd appreciate feedback:

    Should we add an envvar to configure coroutine source tracking? What about an -X switch?

    Should it be coro.origin or coro.cr_origin? On the one hand the cr_ prefixing is super annoying and I think we should probably get rid of it; on the other maybe we should keep it here so things stay consistent and then have a separate the debate about removing it in general.

    Most important: what format should we use for storing the origin information? I can see three plausible approaches:

    1. Call traceback.StackSummary.extract. This means re-entering the Python interpreter from inside the coroutine setup code, which makes me a little nervous. I guess there can't be any real re-entrancy issues here because if there were, set_coroutine_wrapper would already be hitting them. Can it cause problems during shutdown when the traceback module might have disappeared? (Not that anyone's likely to be instantiating coroutines at that point.) It also might be a bit slower than the other options below, but this isn't clear. OTOH, it has the benefit that coro.origin could be a full-fledged StackSummary with all its features, like traceback printing. Which honestly feels a little weird to me, because I know coroutine objects are built-in and StackSummary objects aren't, but it would certainly be convenient.

    2. Capture (filename, lineno, function) directly in a packed struct, similar to how tracemalloc works. Then coro.origin can unpack this into a list of tuples or whatever. This is definitely very fast, and avoids re-entering Python. In practice we'd probably still end up needing to re-enter Python and using the traceback module when it came time to print a warning, though, because traceback printing is complicated and we don't want to reimplement it. Also, if the origin comes from a custom importer like zipimport, then we may not be able to look up the line information later, because that requires access to frame.f_globals["__loader__"].

    3. Like (2), but insert a call to linecache.lazycache for each origin frame we capture. This would fix option (2)'s issue with printing source lines, but it means re-entering Python as well, so at this point maybe it'd just be simpler to go with option (1).

    Given this analysis I guess I'm leaning towards just calling StackSummary.extract, but I don't feel like I fully understand the consequences of calling into a Python module like that so want to hear what others think.

    @njsmith njsmith added 3.8 3.7 interpreter-core expert-asyncio labels Jan 18, 2018
    @1st1
    Copy link
    Member

    @1st1 1st1 commented Jan 18, 2018

    +1 on on the API and for adding it in 3.7 and for deprecating set_coroutine_wrapepr. This will simplify asyncio code and other event loops (like uvloop) and will lead to less overhead on asyncio debug mode.

    I always disliked the set_coroutine_wrapper API. Even at the time when I added it to PEP-492 I understood that we'll have to replace it eventually with something more sensible.

    I think we should start working on a patch. Nathaniel, do you have time to champion the work?

    Most important: what format should we use for storing the origin information? I can see three plausible approaches:

    I'd be strongly against calling traceback.StackSummary.extract when coroutine object is created. Let's go with the option 2. You can create a list of tuple objects without the need of inventing a new struct. coroutine.cr_origin (let's have the "cr_" prefix; dropping the prefixes should be considered in a separate issue) should return that list object directly.

    I'm not sure how exactly the warning print logic will work with a list of tuples in cr_origin, I propose to start working on the implementation and figure that out during the review process.

    @njsmith
    Copy link
    Contributor Author

    @njsmith njsmith commented Jan 19, 2018

    I'm not sure how exactly the warning print logic will work with a list of tuples in cr_origin, I propose to start working on the implementation and figure that out during the review process.

    I think the simplest thing is probably to write the warning code in Python and stash it in Lib/_corohelper.py or something. Warnings already go through warnings.py (in particular, the warning printing code is still warnings.showwarning, it's never been rewritten in C), so we're already transitioning to Python at this stage anyway.

    @1st1
    Copy link
    Member

    @1st1 1st1 commented Jan 19, 2018

    I think the simplest thing is probably to write the warning code in Python and stash it in Lib/_corohelper.py or something.

    I'd put the helper that accepts a list of traceback-like tuples and emits a warning in warnings.py and a C function to call it in warnings.c.

    @1st1
    Copy link
    Member

    @1st1 1st1 commented Jan 20, 2018

    Guido,

    I'd like to go forward with this and merge Nathaniel's PR.

    Quick summary:

    • This issue adds new APIs: a sys.set_coroutine_origin_tracking_depth(depth) function and a 'cr_origin' property to native coroutine objects.

    • By using this APIs, users can opt-in to save traceback (serialized as tuples of (funcname, filename, line)) of where native coroutines are created.

    • This allows us to deprecate the sys.set_coroutine_wrapper function, which I never liked. It's too powerful as it allows anyone to intercept/override native coroutine construction logic.

    • In turn, we are removing a lot of complexity from asyncio code: starting with 3.7 we'll use CoroWrapper only for generator-based corotuines; at some point (Python 3.9?) when we remove @asyncio.coroutine we'll be able to remove CoroWrapper. This will also make asyncio debug mode quite a bit faster, allowing people to use it in production.

    • Finally, sys.set_coroutine_wrapper was always documented as a debug-only API that might disappear in future CPython releases. We'll deprecate it in 3.7 and remove it in 3.8.

    Are you OK with this?

    @gvanrossum
    Copy link
    Member

    @gvanrossum gvanrossum commented Jan 21, 2018

    Yes, I'm OK with this plan. Note that I've only read Nathaniel's original
    message and Yury's last message, and I've not reviewed the PR, but getting
    rid of that eyesore sounds good, and it seems you two have reached
    agreement.

    @1st1
    Copy link
    Member

    @1st1 1st1 commented Jan 21, 2018

    Merged! Thank you, Nathaniel!

    @1st1 1st1 removed the 3.8 label Jan 21, 2018
    @1st1 1st1 closed this as completed Jan 21, 2018
    @1st1 1st1 added the type-feature label Jan 21, 2018
    @1st1
    Copy link
    Member

    @1st1 1st1 commented Jan 22, 2018

    New changeset 3510334 by Yury Selivanov in branch 'master':
    bpo-32591: Fix PyExc_WarnFormat call (follow-up commit) (bpo-5263)
    3510334

    @1st1
    Copy link
    Member

    @1st1 1st1 commented Jan 26, 2018

    The following snippet crashes with SIGABRT:

        import asyncio
        async def f(): pass
        asyncio.gather(f())

    llvm tells that the crash is in _PyGen_Finalize/_PyErr_WarnUnawaitedCoroutine

    @1st1 1st1 reopened this Jan 26, 2018
    @njsmith
    Copy link
    Contributor Author

    @njsmith njsmith commented Jan 26, 2018

    Right, I *knew* I should be nervous about calling into Python from a C-level destructor... what's happening is:

    • Somehow, that coroutine object is managing to survive until the very, very end of the shutdown sequence (PyImport_Cleanup). I'm not sure how exactly; Yury suspects it somehow involves _asynciomodule.c holding a reference.

    • At the very end of PyImport_Cleanup, there's a final call to the cycle collector, which destroys the coroutine object

    • _PyGen_Finalize notices it's unawaited, and calls _PyErr_WarnUnawaitedCoroutine

    • _PyErr_WarnUnawaitedCoroutine attempts to call warnings._warn_unawaited_coroutine, in a very careful and defensive manner

    • But it doesn't matter how careful and defensive you are, because at this stage in the shutdown, we have sys.module = None, which freaks out the import system so badly that when we try to look up the warnings module, it doesn't even raise an error, it just abort()s the whole interpreter.

    We can get a similar crash by doing:

    import sys
    async def f(): pass
    sys.corocycle = [f]
    sys.corocycle.append(sys.corocycle)

    If you run the same code on 3.6, then it gets collected at the same time, and it issues a warning using the regular PyErr_WarnEx. It turns out that code is even *more* careful and defensive and notices that the interpreter is being shutdown, so it just skips printing the warning entirely.

    I guess what we have to do is add a similar check to _PyErr_WarnUnawaitedCoroutine.

    You can imagine how excited I am that I started working on this patch so I could make sure people get more reliable notice of unawaited coroutines (bpo-30491), and not only has that been rejected, but now I'm writing code to explicitly hide unawaited coroutine warnings. Just saying'...

    @gvanrossum
    Copy link
    Member

    @gvanrossum gvanrossum commented Jan 26, 2018

    Maybe it's better to roll this back and take a deep breath. The feature freeze should not be treated as an excuse to cram in things at the last minute. Before this week I had never heard of this proposal. And the sarcasm is a bad start of your stint as a core dev, Nathaniel.

    @1st1
    Copy link
    Member

    @1st1 1st1 commented Jan 26, 2018

    New changeset dba976b by Yury Selivanov (Nathaniel J. Smith) in branch 'master':
    bpo-32591: fix abort in _PyErr_WarnUnawaitedCoroutine during shutdown (bpo-5337)
    dba976b

    @1st1
    Copy link
    Member

    @1st1 1st1 commented Jan 26, 2018

    Nathaniel's latest PR looks good and fixes the crash.

    FWIW losing some warnings during interpreter shutdown isn't exactly a new problem, therefore I think there's nothing wrong with the new API here. For instance, I've seen asyncio's CoroWrapper losing warnings in the exact same fashion.

    So I've merged Nathaniel's PR. I think we can rollback the new API if we discover more problems during the beta-1 period.

    @1st1 1st1 closed this as completed Jan 26, 2018
    @gvanrossum
    Copy link
    Member

    @gvanrossum gvanrossum commented Jan 26, 2018

    OK. I hope both of you keep an eye on this and actively try to test and
    break it!

    @1st1
    Copy link
    Member

    @1st1 1st1 commented Jan 26, 2018

    OK. I hope both of you keep an eye on this and actively try to test and
    break it!

    That's the plan!

    @1st1
    Copy link
    Member

    @1st1 1st1 commented Jan 29, 2018

    Nathaniel,

    test_coroutines raises a bunch of Runtime and Deprecation warnings after this PR. Could you please make a PR to silence them?

    /Users/yury/dev/pydev/cpython/Lib/test/test_coroutines.py:1032: RuntimeWarning: coroutine 'CoroutineTest.test_await_12.<locals>.coro' was never awaited
    return await Awaitable()
    /Users/yury/dev/pydev/cpython/Lib/test/test_coroutines.py:37: RuntimeWarning: coroutine 'CoroutineTest.test_await_9.<locals>.bar' was never awaited
    buffer.append(coro.send(None))
    /Users/yury/dev/pydev/cpython/Lib/test/test_coroutines.py:681: RuntimeWarning: coroutine 'CoroutineTest.test_func_13.<locals>.g' was never awaited
    g().send('spam')
    /Users/yury/dev/pydev/cpython/Lib/test/test_coroutines.py:534: RuntimeWarning: coroutine 'CoroutineTest.test_func_4.<locals>.foo' was never awaited
    list(foo())
    /Users/yury/dev/pydev/cpython/Lib/test/test_coroutines.py:537: RuntimeWarning: coroutine 'CoroutineTest.test_func_4.<locals>.foo' was never awaited
    tuple(foo())
    /Users/yury/dev/pydev/cpython/Lib/test/test_coroutines.py:540: RuntimeWarning: coroutine 'CoroutineTest.test_func_4.<locals>.foo' was never awaited
    sum(foo())
    /Users/yury/dev/pydev/cpython/Lib/test/test_coroutines.py:543: RuntimeWarning: coroutine 'CoroutineTest.test_func_4.<locals>.foo' was never awaited
    iter(foo())
    /Users/yury/dev/pydev/cpython/Lib/test/test_coroutines.py:564: RuntimeWarning: coroutine 'CoroutineTest.test_func_5.<locals>.foo' was never awaited
    for el in foo(): pass
    /Users/yury/dev/pydev/cpython/Lib/test/test_coroutines.py:1984: DeprecationWarning: get_coroutine_wrapper is deprecated
    self.assertIs(sys.get_coroutine_wrapper(), wrap)
    /Users/yury/dev/pydev/cpython/Lib/test/test_coroutines.py:1991: DeprecationWarning: set_coroutine_wrapper is deprecated
    sys.set_coroutine_wrapper(None)
    /Users/yury/dev/pydev/cpython/Lib/test/test_coroutines.py:1993: DeprecationWarning: get_coroutine_wrapper is deprecated
    self.assertIsNone(sys.get_coroutine_wrapper())
    /Users/yury/dev/pydev/cpython/Lib/test/test_coroutines.py:2001: DeprecationWarning: get_coroutine_wrapper is deprecated
    self.assertIsNone(sys.get_coroutine_wrapper())
    /Users/yury/dev/pydev/cpython/Lib/test/test_coroutines.py:2003: DeprecationWarning: set_coroutine_wrapper is deprecated
    sys.set_coroutine_wrapper(1)
    /Users/yury/dev/pydev/cpython/Lib/test/test_coroutines.py:2004: DeprecationWarning: get_coroutine_wrapper is deprecated
    self.assertIsNone(sys.get_coroutine_wrapper())
    /Users/yury/dev/pydev/cpython/Lib/test/test_coroutines.py:2015: DeprecationWarning: set_coroutine_wrapper is deprecated
    sys.set_coroutine_wrapper(wrapper)
    /Users/yury/dev/pydev/cpython/Lib/test/test_coroutines.py:2024: DeprecationWarning: set_coroutine_wrapper is deprecated
    sys.set_coroutine_wrapper(None)
    /Users/yury/dev/pydev/cpython/Lib/test/test_coroutines.py:2037: DeprecationWarning: set_coroutine_wrapper is deprecated
    sys.set_coroutine_wrapper(wrap)
    /Users/yury/dev/pydev/cpython/Lib/test/test_coroutines.py:2045: DeprecationWarning: set_coroutine_wrapper is deprecated
    sys.set_coroutine_wrapper(None)

    @1st1 1st1 reopened this Jan 29, 2018
    @1st1
    Copy link
    Member

    @1st1 1st1 commented Jan 29, 2018

    Specifically "DeprecationWarning: get_coroutine_wrapper is deprecated" needs to be silenced as part of this issue.

    @njsmith
    Copy link
    Contributor Author

    @njsmith njsmith commented Jan 29, 2018

    Ah, gotcha. I'll take a look. (For posterity: the RuntimeWarnings Yury mentioned are also real, but they're from the branch Yury is testing for bpo-32605 / bpo-32703, nothing to do with this issue.)

    @njsmith
    Copy link
    Contributor Author

    @njsmith njsmith commented Jan 29, 2018

    New changeset 95e2147 by Nathaniel J. Smith in branch 'master':
    bpo-32591: silence deprecation warnings in test_coroutine (GH-5412)
    95e2147

    @1st1 1st1 closed this as completed May 28, 2018
    @Yhg1s
    Copy link
    Member

    @Yhg1s Yhg1s commented May 31, 2018

    New changeset 500a419 by T. Wouters in branch '3.6':
    [3.6] bpo-32591: fix abort in _PyErr_WarnUnawaitedCoroutine during shutdown (GH-5337) (bpo-6536)
    500a419

    @vstinner
    Copy link
    Member

    @vstinner vstinner commented Mar 31, 2020

    New changeset 8d84adc by Victor Stinner in branch 'master':
    bpo-32591: _PyErr_WarnUnawaitedCoroutine() sets source (GH-19247)
    8d84adc

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.7 expert-asyncio interpreter-core type-feature
    Projects
    None yet
    Development

    No branches or pull requests

    5 participants