Merge pull request #62 from zopefoundation/jam-systemerror

Avoid raising a SystemError when clearing slots if setstate() failed.

CHANGES.rst

@@ -4,7 +4,9 @@
 4.2.4 (unreleased)
 ------------------
 
-- Nothing changed yet.
+- Avoid raising a ``SystemError: error return without exception set``
+  when loading an object with slots whose jar generates an exception
+  (such as a ZODB ``POSKeyError``) in ``setstate``.
 
 
 4.2.3 (2017-03-08)

persistent/cPersistence.c

@@ -101,7 +101,7 @@ unghostify(cPersistentObject *self)
         {
             /* Create a node in the ring for this unghostified object. */
             self->cache->non_ghost_count++;
-            self->cache->total_estimated_size += 
+            self->cache->total_estimated_size +=
                 _estimated_size_in_bytes(self->estimated_size);
             ring_add(&self->cache->ring_home, &self->ring);
             Py_INCREF(self);
@@ -152,13 +152,14 @@ static void
 ghostify(cPersistentObject *self)
 {
     PyObject **dictptr, *slotnames;
+    PyObject *errtype, *errvalue, *errtb;
 
     /* are we already a ghost? */
     if (self->state == cPersistent_GHOST_STATE)
         return;
 
     /* Is it ever possible to not have a cache? */
-    if (self->cache == NULL) 
+    if (self->cache == NULL)
     {
         self->state = cPersistent_GHOST_STATE;
         return;
@@ -177,7 +178,7 @@ ghostify(cPersistentObject *self)
     /* If we're ghostifying an object, we better have some non-ghosts. */
     assert(self->cache->non_ghost_count > 0);
     self->cache->non_ghost_count--;
-    self->cache->total_estimated_size -= 
+    self->cache->total_estimated_size -=
         _estimated_size_in_bytes(self->estimated_size);
     ring_del(&self->ring);
     self->state = cPersistent_GHOST_STATE;
@@ -195,6 +196,12 @@ ghostify(cPersistentObject *self)
      *   override __new__ ) */
     if (Py_TYPE(self)->tp_new == Pertype.tp_new)
     {
+        /* later we might clear an AttributeError but
+         * if we have a pending exception that still needs to be
+         * raised so that we don't generate a SystemError.
+         */
+        PyErr_Fetch(&errtype, &errvalue, &errtb);
+
         slotnames = pickle_slotnames(Py_TYPE(self));
         if (slotnames && slotnames != Py_None)
         {
@@ -235,6 +242,7 @@ ghostify(cPersistentObject *self)
             }
         }
         Py_XDECREF(slotnames);
+        PyErr_Restore(errtype, errvalue, errtb);
     }
 
     /* We remove the reference to the just ghosted object that the ring
@@ -262,7 +270,7 @@ changed(cPersistentObject *self)
         if (meth == NULL)
             return -1;
         arg = PyTuple_New(1);
-        if (arg == NULL) 
+        if (arg == NULL)
         {
             Py_DECREF(meth);
             return -1;
@@ -371,7 +379,7 @@ pickle_slotnames(PyTypeObject *cls)
             return NULL;
         if (n)
             slotnames = Py_None;
-        
+
         Py_INCREF(slotnames);
         return slotnames;
     }
@@ -594,7 +602,7 @@ pickle___setstate__(PyObject *self, PyObject *state)
         int len;
 
         dict = _PyObject_GetDictPtr(self);
-        
+
         if (!dict)
         {
             PyErr_SetString(PyExc_TypeError,

persistent/tests/test_persistence.py

@@ -16,6 +16,9 @@ import unittest
 
 import platform
 import sys
+
+from persistent._compat import copy_reg
+
 py_impl = getattr(platform, 'python_implementation', lambda: None)
 _is_pypy3 = py_impl() == 'PyPy' and sys.version_info[0] > 2
 _is_jython = py_impl() == 'Jython'
@@ -78,22 +81,22 @@ class _Persistent_Base(object):
                 self.called = 0
             def register(self,ob):
                 self.called += 1
-                raise NotImplementedError
+                raise NotImplementedError()
             def setstate(self,ob):
-                raise NotImplementedError
+                raise NotImplementedError()
 
         jar = _BrokenJar()
         jar._cache = self._makeCache(jar)
         return jar
 
-    def _makeOneWithJar(self, klass=None):
+    def _makeOneWithJar(self, klass=None, broken_jar=False):
         from persistent.timestamp import _makeOctets
         OID = _makeOctets('\x01' * 8)
         if klass is not None:
             inst = klass()
         else:
             inst = self._makeOne()
-        jar = self._makeJar()
+        jar = self._makeJar() if not broken_jar else self._makeBrokenJar()
         jar._cache.new_ghost(OID, inst) # assigns _p_jar, _p_oid
         return inst, jar, OID
 
@@ -1401,6 +1404,26 @@ class _Persistent_Base(object):
         self.assertEqual(list(jar._loaded), [])
         self.assertEqual(list(jar._registered), [])
 
+    def test_p_invalidate_with_slots_broken_jar(self):
+        # If jar.setstate() raises a POSKeyError (or any error)
+        # clearing an object with unset slots doesn't result in a
+        # SystemError, the original error is propagated
+
+        class Derived(self._getTargetClass()):
+            __slots__ = ('slot1',)
+
+        # Pre-cache in __slotnames__; cpersistent goes directly for this
+        # and avoids a call to copy_reg. (If it calls the python code in
+        # copy_reg, the pending exception will be immediately propagated by
+        # copy_reg, not by us.)
+        copy_reg._slotnames(Derived)
+
+        inst, jar, OID = self._makeOneWithJar(Derived, broken_jar=True)
+        inst._p_invalidate()
+        self.assertEqual(inst._p_status, 'ghost')
+        self.assertRaises(NotImplementedError, inst._p_activate)
+
+
     def test__p_invalidate_from_sticky(self):
         inst, jar, OID = self._makeOneWithJar()
         inst._p_activate() # XXX