classification
Title: Change contextvars C API to use PyObject
Type: enhancement Stage: resolved
Components: Interpreter Core Versions: Python 3.8, Python 3.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: eric.snow, gvanrossum, miss-islington, ned.deily, rhettinger, scoder, serhiy.storchaka, vstinner, yselivanov
Priority: normal Keywords: patch

Created on 2018-09-21 15:06 by yselivanov, last changed 2018-09-27 19:35 by yselivanov. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 9473 merged yselivanov, 2018-09-21 15:08
PR 9478 merged miss-islington, 2018-09-21 19:34
PR 9609 merged yselivanov, 2018-09-27 18:42
PR 9610 merged yselivanov, 2018-09-27 19:10
Messages (20)
msg325989 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2018-09-21 15:06
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.
msg325990 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2018-09-21 15:13
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.
msg325991 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-09-21 15:28
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 ;-)
msg325993 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2018-09-21 15:32
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)
msg325999 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2018-09-21 16:21
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.
msg326000 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-09-21 16:24
> 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
msg326002 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2018-09-21 16:28
>> 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.
msg326003 - (view) Author: Eric Snow (eric.snow) * (Python committer) Date: 2018-09-21 16:32
> 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
msg326004 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2018-09-21 16:37
> > 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.
msg326007 - (view) Author: Stefan Behnel (scoder) * Date: 2018-09-21 16:41
> because Py_buffer isn't a PyObject at all :)

It owns a PyObject reference to the buffer owner, though.
msg326009 - (view) Author: Ned Deily (ned.deily) * (Python committer) Date: 2018-09-21 17:32
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.
msg326022 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2018-09-21 19:34
New changeset 2ec872b31e25cee1f983fe07991fb53f3fd1cbac by Yury Selivanov in branch 'master':
bpo-34762: Fix contextvars C API to use PyObject* pointer types. (GH-9473)
https://github.com/python/cpython/commit/2ec872b31e25cee1f983fe07991fb53f3fd1cbac
msg326025 - (view) Author: miss-islington (miss-islington) Date: 2018-09-21 19:48
New changeset 187f2dd256a917c20bf55954d019fd35fb46ab08 by Miss Islington (bot) in branch '3.7':
bpo-34762: Fix contextvars C API to use PyObject* pointer types. (GH-9473)
https://github.com/python/cpython/commit/187f2dd256a917c20bf55954d019fd35fb46ab08
msg326027 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2018-09-21 19:49
PEP update: https://github.com/python/peps/commit/977a94d1a79b71336568119eb689b277d2ffec80
msg326028 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2018-09-21 19:50
Fixed in master and 3.7.  Thanks!
msg326574 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2018-09-27 17:20
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);
                         ^
msg326575 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2018-09-27 17:24
Right, I'll make a PR.
msg326582 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2018-09-27 18:55
New changeset 994269ccee5574f03cda6b018399347fc52bf330 by Yury Selivanov in branch 'master':
bpo-34762: Update PyContext* to PyObject* in asyncio and decimal (GH-9609)
https://github.com/python/cpython/commit/994269ccee5574f03cda6b018399347fc52bf330
msg326585 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2018-09-27 19:33
New changeset 24cb7de15d3a5979425b281ab4f600f7c2b401f2 by Yury Selivanov in branch '3.7':
[3.7] bpo-34762: Update PyContext* refs to PyObject* in asyncio and decimal (GH-9610)
https://github.com/python/cpython/commit/24cb7de15d3a5979425b281ab4f600f7c2b401f2
msg326586 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2018-09-27 19:35
Thank you Serhiy, for re-opening this!  I've pushed fixes to 3.7 and master branches.
History
Date User Action Args
2018-09-27 19:35:15yselivanovsetstatus: open -> closed

messages: + msg326586
stage: patch review -> resolved
2018-09-27 19:33:29yselivanovsetmessages: + msg326585
2018-09-27 19:10:30yselivanovsetpull_requests: + pull_request9008
2018-09-27 18:55:58yselivanovsetmessages: + msg326582
2018-09-27 18:42:53yselivanovsetstage: resolved -> patch review
pull_requests: + pull_request9007
2018-09-27 17:24:00yselivanovsetmessages: + msg326575
2018-09-27 17:20:06serhiy.storchakasetstatus: closed -> open
nosy: + serhiy.storchaka
messages: + msg326574

2018-09-21 19:50:34yselivanovsetstatus: open -> closed
type: enhancement
messages: + msg326028

resolution: fixed
stage: patch review -> resolved
2018-09-21 19:49:37yselivanovsetmessages: + msg326027
2018-09-21 19:48:15miss-islingtonsetnosy: + miss-islington
messages: + msg326025
2018-09-21 19:34:26miss-islingtonsetpull_requests: + pull_request8890
2018-09-21 19:34:02yselivanovsetmessages: + msg326022
2018-09-21 17:32:27ned.deilysetmessages: + msg326009
2018-09-21 16:41:56scodersetnosy: + scoder
messages: + msg326007
2018-09-21 16:37:21yselivanovsetmessages: + msg326004
2018-09-21 16:32:24eric.snowsetnosy: + eric.snow
messages: + msg326003
2018-09-21 16:28:07yselivanovsetmessages: + msg326002
2018-09-21 16:24:56vstinnersetmessages: + msg326000
2018-09-21 16:21:43rhettingersetnosy: + rhettinger
messages: + msg325999
2018-09-21 15:32:12gvanrossumsetmessages: + msg325993
2018-09-21 15:28:42vstinnersetnosy: + vstinner
messages: + msg325991
2018-09-21 15:13:45yselivanovsetnosy: + gvanrossum
messages: + msg325990
2018-09-21 15:08:15yselivanovsetkeywords: + patch
stage: patch review
pull_requests: + pull_request8886
2018-09-21 15:06:32yselivanovcreate