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

Change contextvars C API to use PyObject #78943

Closed
1st1 opened this issue Sep 21, 2018 · 20 comments
Closed

Change contextvars C API to use PyObject #78943

1st1 opened this issue Sep 21, 2018 · 20 comments
Labels
3.7 3.8 interpreter-core type-feature

Comments

@1st1
Copy link
Member

@1st1 1st1 commented Sep 21, 2018

BPO 34762
Nosy @gvanrossum, @rhettinger, @scoder, @vstinner, @ned-deily, @ericsnowcurrently, @serhiy-storchaka, @1st1, @miss-islington
PRs
  • #9473
  • #9478
  • #9609
  • #9610
  • 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-09-27.19:35:15.549>
    created_at = <Date 2018-09-21.15:06:32.391>
    labels = ['interpreter-core', '3.8', 'type-feature', '3.7']
    title = 'Change contextvars C API to use PyObject'
    updated_at = <Date 2018-09-27.19:35:15.547>
    user = 'https://github.com/1st1'

    bugs.python.org fields:

    activity = <Date 2018-09-27.19:35:15.547>
    actor = 'yselivanov'
    assignee = 'none'
    closed = True
    closed_date = <Date 2018-09-27.19:35:15.549>
    closer = 'yselivanov'
    components = ['Interpreter Core']
    creation = <Date 2018-09-21.15:06:32.391>
    creator = 'yselivanov'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 34762
    keywords = ['patch']
    message_count = 20.0
    messages = ['325989', '325990', '325991', '325993', '325999', '326000', '326002', '326003', '326004', '326007', '326009', '326022', '326025', '326027', '326028', '326574', '326575', '326582', '326585', '326586']
    nosy_count = 9.0
    nosy_names = ['gvanrossum', 'rhettinger', 'scoder', 'vstinner', 'ned.deily', 'eric.snow', 'serhiy.storchaka', 'yselivanov', 'miss-islington']
    pr_nums = ['9473', '9478', '9609', '9610']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue34762'
    versions = ['Python 3.7', 'Python 3.8']

    @1st1
    Copy link
    Member Author

    @1st1 1st1 commented Sep 21, 2018

    Unfortunately, the current C API for PEP-567 has a flaw: it uses non-PyObject pointers.

    This causes problems with integrating with tools like Cython, where PyObject is special and a lot of machinery recognizes it and manages refcounts correctly.

    It also goes against the recent push to improve C API; one of the things we want to fix is to eliminate non-PyObject pointer types from public APIs entirely.

    Because this C API is new (landed in 3.7.0) I think it might make sense to change it in 3.7.1. I *think* this is a relatively safe (albeit annoying) change:

    1. I don't expect that a lot of people use this new C API. I googled recently if anyone uses contextvars at all, and found that some people are using the Python API. The only user of C API that I'm aware of is uvloop, which I'll be happy to fix.

    2. My current understanding is that this change won't break existing extensions compiled against Python 3.7.0, as it's just a change in pointers' types.

    3. For example, clang spits out the following *warning* if it sees mismatched pointer types (and still compiles the extension):

      warning: incompatible pointer types passing 'PyContextVar *' (aka
      'struct _pycontextvarobject *') to parameter of type 'PyObject *' (aka 'struct _object *')
      [-Wincompatible-pointer-types]

    4. This would make it slightly harder to create extension that supports both 3.7.0 and 3.7.1 and compiles without warnings. (It's possible to avoid warnings by adding some #ifdefs macro).

    If we don't change this API now, we'll likely have to either live with it, or face a very slow deprecation period.

    @1st1 1st1 added 3.7 3.8 interpreter-core labels Sep 21, 2018
    @1st1
    Copy link
    Member Author

    @1st1 1st1 commented Sep 21, 2018

    Just to add to this issue: I originally realized that something is wrong with the design when we had a super hard to track memory leak in uvloop, caused by Cython being unable to automatically manage increfs/decrefs for PyContext* pointers. So I do believe this is a serious pitfall.

    Adding Guido to the nosy list as he accepted the PEP and IMO still has a say in this.

    @vstinner
    Copy link
    Member

    @vstinner vstinner commented Sep 21, 2018

    IMHO it's not too late to change the public C API in Python 3.7.1, since it's a very new API, I don't expect many users, and I only expect compiler warnings nor hard error if existing code pass PyContextVar* instead of PyObject*.

    I agree that it's way better to use PyObject* rather than specific PyContextVar*. The C API should not leak implementation details ;-)

    @gvanrossum
    Copy link
    Member

    @gvanrossum gvanrossum commented Sep 21, 2018

    Let’s change it ASAP. It’s still up to Ned whether to hold up 3.7.1, if he
    won’t it should go into 3.7.2.

    On Fri, Sep 21, 2018 at 8:28 AM STINNER Victor <report@bugs.python.org>
    wrote:

    STINNER Victor <vstinner@redhat.com> added the comment:

    IMHO it's not too late to change the public C API in Python 3.7.1, since
    it's a very new API, I don't expect many users, and I only expect compiler
    warnings nor hard error if existing code pass PyContextVar* instead of
    PyObject*.

    I agree that it's way better to use PyObject* rather than specific
    PyContextVar*. The C API should not leak implementation details ;-)

    ----------
    nosy: +vstinner


    Python tracker <report@bugs.python.org>
    <https://bugs.python.org/issue34762\>


    --
    --Guido (mobile)

    @rhettinger
    Copy link
    Contributor

    @rhettinger rhettinger commented Sep 21, 2018

    I concur that the sooner the change is applied, the better it will be for everyone. We'll need to make a prominent notice in the release notes though.

    @vstinner
    Copy link
    Member

    @vstinner vstinner commented Sep 21, 2018

    We'll need to make a prominent notice in the release notes though.

    Something can be added at:
    https://docs.python.org/dev/whatsnew/3.7.html#notable-changes-in-python-3-7-1

    @1st1
    Copy link
    Member Author

    @1st1 1st1 commented Sep 21, 2018

    > We'll need to make a prominent notice in the release notes though.

    Something can be added at:
    https://docs.python.org/dev/whatsnew/3.7.html#notable-changes-in-python-3-7-1

    Yeah, thank you for suggestion, I'll do that. I'll also update the PEP, docs in other places, and try to spread the word.

    Now I'm just waiting for Ned to confirm if this goes into 3.7.1 or 3.7.2. It's his say.

    @ericsnowcurrently
    Copy link
    Member

    @ericsnowcurrently ericsnowcurrently commented Sep 21, 2018

    one of the things we want to fix is to eliminate non-PyObject
    pointer types from public APIs entirely.

    A notable exception is Py_buffer. [1] As well, cross-interpreter data must not be PyObject, though that isn't much of an issue (yet). :) At some point it may make sense to make _PyCrossInterpreterData [2] part of the public C-API. However, we can cross *that* bridge later. :)

    Using PyObject for contextvars makes sense (for the reasons you described) as long as they won't be shared between interpreters. I'm not super familiar with the contextvars C-API, but it looks like cross-interpreter issues are *not* a factor. If that's the case then there's nothing further to discuss. :)

    [1] https://docs.python.org/3/c-api/buffer.html#buffer-structure
    [2] https://github.com/python/cpython/blob/master/Include/internal/pystate.h#L137

    @1st1
    Copy link
    Member Author

    @1st1 1st1 commented Sep 21, 2018

    > one of the things we want to fix is to eliminate non-PyObject
    pointer types from public APIs entirely.

    A notable exception is Py_buffer. [1]

    Right, because Py_buffer isn't a PyObject at all :)

    Using PyObject for contextvars makes sense (for the reasons you described) as long as they won't be shared between interpreters.

    Yeah, PyContext, PyContextVar, and PyContextToken aren't supposed to be shared between sub-interpreters directly. Context is essentially a mapping of Context Variables to arbitrary Python Objects, so sharing it transparently isn't possible.

    @scoder
    Copy link
    Contributor

    @scoder scoder commented Sep 21, 2018

    because Py_buffer isn't a PyObject at all :)

    It owns a PyObject reference to the buffer owner, though.

    @ned-deily
    Copy link
    Member

    @ned-deily ned-deily commented Sep 21, 2018

    Since we've already delayed 3.7.1rc cutoff for a few days, let's get it into 3.7.1 if you have the time, Yury.

    @1st1
    Copy link
    Member Author

    @1st1 1st1 commented Sep 21, 2018

    New changeset 2ec872b by Yury Selivanov in branch 'master':
    bpo-34762: Fix contextvars C API to use PyObject* pointer types. (GH-9473)
    2ec872b

    @miss-islington
    Copy link
    Contributor

    @miss-islington miss-islington commented Sep 21, 2018

    New changeset 187f2dd by Miss Islington (bot) in branch '3.7':
    bpo-34762: Fix contextvars C API to use PyObject* pointer types. (GH-9473)
    187f2dd

    @1st1
    Copy link
    Member Author

    @1st1 1st1 commented Sep 21, 2018

    PEP update: python/peps@977a94d

    @1st1
    Copy link
    Member Author

    @1st1 1st1 commented Sep 21, 2018

    Fixed in master and 3.7. Thanks!

    @1st1 1st1 closed this as completed Sep 21, 2018
    @1st1 1st1 added the type-feature label Sep 21, 2018
    @serhiy-storchaka
    Copy link
    Member

    @serhiy-storchaka serhiy-storchaka commented Sep 27, 2018

    Perhaps this change caused new compiler warnings:

    In file included from ./Include/pytime.h:6:0,
                     from ./Include/Python.h:68,
                     from /home/serhiy/py/cpython/Modules/_asynciomodule.c:1:
    /home/serhiy/py/cpython/Modules/_asynciomodule.c: In function ‘_asyncio_Task___init___impl’:
    ./Include/object.h:895:14: warning: assignment from incompatible pointer type [-Wincompatible-pointer-types]
             (op) = (op2);                           \
                  ^
    /home/serhiy/py/cpython/Modules/_asynciomodule.c:1954:5: note: in expansion of macro ‘Py_XSETREF’
         Py_XSETREF(self->task_context, PyContext_CopyCurrent());
         ^~~~~~~~~~
    /home/serhiy/py/cpython/Modules/_decimal/_decimal.c: In function ‘init_current_context’:
    /home/serhiy/py/cpython/Modules/_decimal/_decimal.c:1503:44: warning: passing argument 1 of ‘PyContextVar_Set’ from incompatible pointer type [-Wincompatible-pointer-types]
         PyContextToken *tok = PyContextVar_Set(current_context_var, tl_context);
                                                ^~~~~~~~~~~~~~~~~~~
    In file included from ./Include/Python.h:116:0,
                     from /home/serhiy/py/cpython/Modules/_decimal/_decimal.c:29:
    ./Include/context.h:63:24: note: expected ‘PyObject * {aka struct _object *}’ but argument is of type ‘PyContextVar * {aka struct _pycontextvarobject *}’
     PyAPI_FUNC(PyObject *) PyContextVar_Set(PyObject *var, PyObject *value);
                            ^~~~~~~~~~~~~~~~
    /home/serhiy/py/cpython/Modules/_decimal/_decimal.c:1503:27: warning: initialization from incompatible pointer type [-Wincompatible-pointer-types]
         PyContextToken *tok = PyContextVar_Set(current_context_var, tl_context);
                               ^~~~~~~~~~~~~~~~
    /home/serhiy/py/cpython/Modules/_decimal/_decimal.c: In function ‘current_context’:
    /home/serhiy/py/cpython/Modules/_decimal/_decimal.c:1517:26: warning: passing argument 1 of ‘PyContextVar_Get’ from incompatible pointer type [-Wincompatible-pointer-types]
         if (PyContextVar_Get(current_context_var, NULL, &tl_context) < 0) {
                              ^~~~~~~~~~~~~~~~~~~
    In file included from ./Include/Python.h:116:0,
                     from /home/serhiy/py/cpython/Modules/_decimal/_decimal.c:29:
    ./Include/context.h:56:17: note: expected ‘PyObject * {aka struct _object *}’ but argument is of type ‘PyContextVar * {aka struct _pycontextvarobject *}’
     PyAPI_FUNC(int) PyContextVar_Get(
                     ^~~~~~~~~~~~~~~~
    /home/serhiy/py/cpython/Modules/_decimal/_decimal.c: In function ‘PyDec_SetCurrentContext’:
    /home/serhiy/py/cpython/Modules/_decimal/_decimal.c:1564:44: warning: passing argument 1 of ‘PyContextVar_Set’ from incompatible pointer type [-Wincompatible-pointer-types]
         PyContextToken *tok = PyContextVar_Set(current_context_var, v);
                                                ^~~~~~~~~~~~~~~~~~~
    In file included from ./Include/Python.h:116:0,
                     from /home/serhiy/py/cpython/Modules/_decimal/_decimal.c:29:
    ./Include/context.h:63:24: note: expected ‘PyObject * {aka struct _object *}’ but argument is of type ‘PyContextVar * {aka struct _pycontextvarobject *}’
     PyAPI_FUNC(PyObject *) PyContextVar_Set(PyObject *var, PyObject *value);
                            ^~~~~~~~~~~~~~~~
    /home/serhiy/py/cpython/Modules/_decimal/_decimal.c:1564:27: warning: initialization from incompatible pointer type [-Wincompatible-pointer-types]
         PyContextToken *tok = PyContextVar_Set(current_context_var, v);
                               ^~~~~~~~~~~~~~~~
    /home/serhiy/py/cpython/Modules/_decimal/_decimal.c: In function ‘PyInit__decimal’:
    /home/serhiy/py/cpython/Modules/_decimal/_decimal.c:5542:25: warning: assignment from incompatible pointer type [-Wincompatible-pointer-types]
         current_context_var = PyContextVar_New("decimal_context", NULL);
                             ^

    @1st1
    Copy link
    Member Author

    @1st1 1st1 commented Sep 27, 2018

    Right, I'll make a PR.

    @1st1
    Copy link
    Member Author

    @1st1 1st1 commented Sep 27, 2018

    New changeset 994269c by Yury Selivanov in branch 'master':
    bpo-34762: Update PyContext* to PyObject* in asyncio and decimal (GH-9609)
    994269c

    @1st1
    Copy link
    Member Author

    @1st1 1st1 commented Sep 27, 2018

    New changeset 24cb7de by Yury Selivanov in branch '3.7':
    [3.7] bpo-34762: Update PyContext* refs to PyObject* in asyncio and decimal (GH-9610)
    24cb7de

    @1st1
    Copy link
    Member Author

    @1st1 1st1 commented Sep 27, 2018

    Thank you Serhiy, for re-opening this! I've pushed fixes to 3.7 and master branches.

    @1st1 1st1 closed this as completed Sep 27, 2018
    @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 3.8 interpreter-core type-feature
    Projects
    None yet
    Development

    No branches or pull requests

    9 participants