From 72237bde60bc06624589bfde5ab66a5740ef3640 Mon Sep 17 00:00:00 2001 From: Kirill Smelkov Date: Wed, 16 Nov 2016 22:34:29 +0300 Subject: [PATCH] On deactivate release in-slots objects only for classes that don't override __new__ Commit fe2219f4 taught Persistent to release in-slots objects on deactivation. That however broke pure-python implementation of many things because they were using __slots__ as a place which survive deactivation. As per discussion in https://github.com/zopefoundation/persistent/pull/44 and resolution: https://github.com/zopefoundation/persistent/pull/44#issuecomment-261019084 let's try to preserve backward compatibility by not releasing slots for classes that override __new__ /proposed-by-and-helped @jimfulton --- persistent/cPersistence.c | 63 +++++++++++++++------------- persistent/persistence.py | 7 +++- persistent/tests/test_persistence.py | 30 +++++++++++++ 3 files changed, 69 insertions(+), 31 deletions(-) diff --git a/persistent/cPersistence.c b/persistent/cPersistence.c index 5a5bfc2..67cdbd2 100644 --- a/persistent/cPersistence.c +++ b/persistent/cPersistence.c @@ -190,47 +190,52 @@ ghostify(cPersistentObject *self) *dictptr = NULL; } - /* clear all slots besides _p_* */ - slotnames = pickle_slotnames(Py_TYPE(self)); - if (slotnames && slotnames != Py_None) + /* clear all slots besides _p_* + * ( for backward-compatibility reason we do this only if class does not + * override __new__ ) */ + if (Py_TYPE(self)->tp_new == Pertype.tp_new) { - int i; - - for (i = 0; i < PyList_GET_SIZE(slotnames); i++) + slotnames = pickle_slotnames(Py_TYPE(self)); + if (slotnames && slotnames != Py_None) { - PyObject *name; - char *cname; - int is_special; + int i; - name = PyList_GET_ITEM(slotnames, i); -#ifdef PY3K - if (PyUnicode_Check(name)) + for (i = 0; i < PyList_GET_SIZE(slotnames); i++) { - PyObject *converted = convert_name(name); - cname = PyBytes_AS_STRING(converted); + PyObject *name; + char *cname; + int is_special; + + name = PyList_GET_ITEM(slotnames, i); +#ifdef PY3K + if (PyUnicode_Check(name)) + { + PyObject *converted = convert_name(name); + cname = PyBytes_AS_STRING(converted); #else - if (PyBytes_Check(name)) - { - cname = PyBytes_AS_STRING(name); + if (PyBytes_Check(name)) + { + cname = PyBytes_AS_STRING(name); #endif - is_special = !strncmp(cname, "_p_", 3); + is_special = !strncmp(cname, "_p_", 3); #ifdef PY3K - Py_DECREF(converted); + Py_DECREF(converted); #endif - if (is_special) /* skip persistent */ - { - continue; + if (is_special) /* skip persistent */ + { + continue; + } } - } - /* NOTE: this skips our delattr hook */ - if (PyObject_GenericSetAttr((PyObject *)self, name, NULL) < 0) - /* delattr of non-set slot will raise AttributeError - we - * simply ignore. */ - PyErr_Clear(); + /* NOTE: this skips our delattr hook */ + if (PyObject_GenericSetAttr((PyObject *)self, name, NULL) < 0) + /* delattr of non-set slot will raise AttributeError - we + * simply ignore. */ + PyErr_Clear(); + } } + Py_XDECREF(slotnames); } - Py_XDECREF(slotnames); /* We remove the reference to the just ghosted object that the ring * holds. Note that the dictionary of oids->objects has an uncounted diff --git a/persistent/persistence.py b/persistent/persistence.py index 7ffc0df..5abd866 100644 --- a/persistent/persistence.py +++ b/persistent/persistence.py @@ -424,8 +424,11 @@ def _p_invalidate_deactivate_helper(self): if idict is not None: idict.clear() type_ = type(self) - for slotname in Persistent._slotnames(self, _v_exclude=False): - getattr(type_, slotname).__delete__(self) + # ( for backward-compatibility reason we release __slots__ only if + # class does not override __new__ ) + if type_.__new__ is Persistent.__new__: + for slotname in Persistent._slotnames(self, _v_exclude=False): + getattr(type_, slotname).__delete__(self) # Implementation detail: deactivating/invalidating # updates the size of the cache (if we have one) diff --git a/persistent/tests/test_persistence.py b/persistent/tests/test_persistence.py index 170b337..0922b64 100644 --- a/persistent/tests/test_persistence.py +++ b/persistent/tests/test_persistence.py @@ -1340,6 +1340,36 @@ def __init__(self): self.assertEqual(list(jar._loaded), []) self.assertEqual(list(jar._registered), []) + def test__p_invalidate_from_changed_w_slots_compat(self): + # check that (for backward-compatibility reason) slots are not released + # for classes where __new__ is overwritten. Attributes in __dict__ + # should be always released. + class Derived(self._getTargetClass()): + __slots__ = ('myattr1', 'myattr2', '__dict__') + def __new__(cls): + obj = cls.__base__.__new__(cls) + obj.myattr1 = 'value1' + obj.myattr2 = 'value2' + obj.foo = 'foo1' # .foo & .bar are in __dict__ + obj.bar = 'bar2' + return obj + inst, jar, OID = self._makeOneWithJar(Derived) + inst._p_activate() + inst._p_changed = True + jar._loaded = [] + jar._registered = [] + self.assertEqual(Derived.myattr1.__get__(inst), 'value1') + self.assertEqual(Derived.myattr2.__get__(inst), 'value2') + self.assertEqual(inst.__dict__, {'foo': 'foo1', 'bar': 'bar2'}) + inst._p_invalidate() + self.assertEqual(inst._p_status, 'ghost') + self.assertEqual(list(jar._loaded), []) + self.assertEqual(Derived.myattr1.__get__(inst), 'value1') + self.assertEqual(Derived.myattr2.__get__(inst), 'value2') + self.assertEqual(inst.__dict__, {}) + self.assertEqual(list(jar._loaded), []) + self.assertEqual(list(jar._registered), []) + def test__p_invalidate_from_sticky(self): inst, jar, OID = self._makeOneWithJar() inst._p_activate() # XXX