Fix __module__ attribute for shared types
#6845
Conversation
| @@ -1771,7 +1771,7 @@ static PyObject *__Pyx_CoroutineAwait_no_new(PyTypeObject *type, PyObject *args, | |||
| // We are applying this to all Python versions (hence the commented out version guard) | |||
| // to make the behaviour explicit. | |||
| // #if CYTHON_USE_TYPE_SPECS | |||
| static PyObject *__Pyx_CoroutineAwait_reduce_ex(__pyx_CoroutineAwaitObject *self, PyObject *arg) { | |||
| static PyObject *__Pyx_CoroutineTypes_reduce_ex(PyObject *self, PyObject *arg) { | |||
There was a problem hiding this comment.
For reasons I don't understand, changing the definition of __module__ magically made coroutines pickleable under protocols 1 and 2. Therefore I just explicitly banned it using the same function as CoroutineAwait uses.
It can't be a descriptor because that messes up `type(func).__module__`. I decided not to put it in `__dict__` because that involves instantiating `__dict__` for every instance (which we've previously decided not to do) and also deciding on the behaviour when `__dict__` is replaced. Therefore the solution seems to be to add some special casing to tp_getattro and tp_setattro. Fixes cython#6841.
| """ | ||
| >>> test_module.__module__ | ||
| 'cyfunction' | ||
| >>> type(test_module).__module__.startswith("_cython") 8000 |
There was a problem hiding this comment.
This seems to be a bit fragile locally, because for some reason I always end up with an older cached cyfunction implementation.
It works when I manually import the module alone but not when I use runtests. I'm not sure what the CI will end up doing, but I'm fairly sure this is a caching problem and will go away when the shared lib version changes.
|
I'm always wary of changing attribute access away from PyObject_GenericGetAttr due to CPython's internal optimisations.
Definitely not a change to merge at this point, the pickle changes seem risky.
|
Agree
I don't think it's as bad as it seems - the non-pickleability of coroutines cython/tests/run/test_coroutines_pep492.pyx Line 2385 in b086383
Yes. Probably worth waiting to see how many people the bug really affects for now |
| @@ -1771,7 +1771,7 @@ static PyObject *__Pyx_CoroutineAwait_no_new(PyTypeObject *type, PyObject *args, | |||
| // We are applying this to all Python versions (hence the commented out version guard) | |||
| // to make the behaviour explicit. | |||
| // #if CYTHON_USE_TYPE_SPECS | |||
| static PyObject *__Pyx_CoroutineAwait_reduce_ex(__pyx_CoroutineAwaitObject *self, PyObject *arg) { | |||
| static PyObject *__Pyx_CoroutineTypes_reduce_ex(PyObject *self, PyObject *arg) { | |||
There was a problem hiding this comment.
Now that we're renaming this function, I think it's worth stating in the function name that this is not a pickle implementation but something that intentionally prevents pickling.
| static PyObject *__Pyx_CoroutineTypes_reduce_ex(PyObject *self, PyObject *arg) { | |
| static PyObject *__Pyx_Coroutine_fail_reduce_ex(PyObject *self, PyObject *arg) { |
| {Py_tp_getattro, (void *) PyObject_GenericGetAttr}, | ||
| {Py_tp_getattro, (void *)__pyx_Coroutine_getattro}, | ||
| {Py_tp_setattro, (void *)__pyx_Coroutine_setattro}, |
There was a problem hiding this comment.
Could you run a quick micro-benchmark to see if this has any visible impact on the time it takes to look up and call .send() from Python code (e.g. from within asyncio)? Especially with all the attribute lookup optimisations in Py3.1[34]? Just so that we have a number.
It can't be a descriptor because that messes up
type(func).__module__. I decided not to put it in__dict__because that involves instantiating__dict__for every instance (which we've previously decided not to do) and also deciding on the behaviour when__dict__is replaced.Therefore the solution seems to be to add some special casing to tp_getattro and tp_setattro.
Fixes #6841.