[issue34762] Change contextvars C API to use PyObject

[Yury Selivanov]

Yury Selivanov  added the comment:

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

--
stage: patch review -> resolved
status: open -> closed

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue34762] Change contextvars C API to use PyObject

[Yury Selivanov]

Yury Selivanov  added the comment:


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


--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue34762] Change contextvars C API to use PyObject

[Yury Selivanov]

Change by Yury Selivanov :


--
pull_requests: +9008

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue34762] Change contextvars C API to use PyObject

[Yury Selivanov]

Yury Selivanov  added the comment:


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


--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue34762] Change contextvars C API to use PyObject

[Yury Selivanov]

Change by Yury Selivanov :


--
pull_requests: +9007
stage: resolved -> patch review

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue34762] Change contextvars C API to use PyObject

[Yury Selivanov]

Yury Selivanov  added the comment:

Right, I'll make a PR.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue34762] Change contextvars C API to use PyObject

[Serhiy Storchaka]

Serhiy Storchaka  added the comment:

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, _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);
 ^

--
nosy: +serhiy.storchaka
status: closed -> open

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue34762] Change contextvars C API to use PyObject

[Yury Selivanov]

Yury Selivanov  added the comment:

Fixed in master and 3.7.  Thanks!

--
resolution:  -> fixed
stage: patch review -> resolved
status: open -> closed
type:  -> enhancement

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue34762] Change contextvars C API to use PyObject

[Yury Selivanov]

Yury Selivanov  added the comment:

PEP update: 
https://github.com/python/peps/commit/977a94d1a79b71336568119eb689b277d2ffec80

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue34762] Change contextvars C API to use PyObject

[miss-islington]

miss-islington  added the comment:


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


--
nosy: +miss-islington

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue34762] Change contextvars C API to use PyObject

[miss-islington]

Change by miss-islington :


--
pull_requests: +8890

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue34762] Change contextvars C API to use PyObject

[Yury Selivanov]

Yury Selivanov  added the comment:


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


--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue34762] Change contextvars C API to use PyObject

[Ned Deily]

Ned Deily  added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue34762] Change contextvars C API to use PyObject

[Stefan Behnel]

Stefan Behnel  added the comment:

> because Py_buffer isn't a PyObject at all :)

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

--
nosy: +scoder

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue34762] Change contextvars C API to use PyObject

[Yury Selivanov]

Yury Selivanov  added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue34762] Change contextvars C API to use PyObject

[Eric Snow]

Eric Snow  added the comment:

> 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

--
nosy: +eric.snow

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue34762] Change contextvars C API to use PyObject

[Yury Selivanov]

Yury Selivanov  added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue34762] Change contextvars C API to use PyObject

[STINNER Victor]

STINNER Victor  added the comment:

> 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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue34762] Change contextvars C API to use PyObject

[Raymond Hettinger]

Raymond Hettinger  added the comment:

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.

--
nosy: +rhettinger

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue34762] Change contextvars C API to use PyObject

[Guido van Rossum]

Guido van Rossum  added the comment:

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 
wrote:

>
> STINNER Victor  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 
> 
> ___
>
-- 
--Guido (mobile)

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue34762] Change contextvars C API to use PyObject

[STINNER Victor]

STINNER Victor  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 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue34762] Change contextvars C API to use PyObject

[Yury Selivanov]

Yury Selivanov  added the comment:

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.

--
nosy: +gvanrossum

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue34762] Change contextvars C API to use PyObject

[Yury Selivanov]

Change by Yury Selivanov :


--
keywords: +patch
pull_requests: +8886
stage:  -> patch review

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue34762] Change contextvars C API to use PyObject

[Yury Selivanov]

New submission from Yury Selivanov :

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.

--
components: Interpreter Core
messages: 325989
nosy: ned.deily, yselivanov
priority: normal
severity: normal
status: open
title: Change contextvars C API to use PyObject
versions: Python 3.7, Python 3.8

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com