[Zope3-checkins] CVS: ZODB4/src/persistence - persistenceAPI.h:1.3 persistence.c:1.17

Jeremy Hylton jeremy@zope.com
Tue, 20 May 2003 15:01:39 -0400


Update of /cvs-repository/ZODB4/src/persistence
In directory cvs.zope.org:/tmp/cvs-serv18494/persistence

Modified Files:
	persistenceAPI.h persistence.c 
Log Message:
Refactor persistence api to use _p_changed only to mark an object as changed.

Use _p_deactivate() to turn an object into a ghost, and use the
keyword argument force=1 if you want to turn a modified object into a
ghost.  Several occurrences of the old interface have been updated.

This refactoring uncovered a number of subtle bugs in the persistence
C API.  The two chief problems were that the load function in the C
API struct did not set the state and that the functions return 0 for
error and 1 for success.  Regardless of whether these APIs are doing
the right thing, fix the code to use them correctly.

One downside of the new API is the C objects (BTrees) that override
_p_deactivate() have to deal with all the cruft for keyword
arguments.  Since BTrees only add a single line of extra code to
_p_deactivate(), it seems useful to provide a hook in the persistence
framework for this purpose.

Also:

If an object is in the changed state, don't call register() on its
data manager a second time.

Ignore state changes that result from setstate() being called.

Don't load an object's state to call __setstate__().

In BTrees check module, if an object has an oid, print that along with
its id().



=== ZODB4/src/persistence/persistenceAPI.h 1.2 => 1.3 ===
--- ZODB4/src/persistence/persistenceAPI.h:1.2	Wed Dec 25 09:12:13 2002
+++ ZODB4/src/persistence/persistenceAPI.h	Tue May 20 15:01:39 2003
@@ -38,25 +38,27 @@
 #define PyPersist_IS_STICKY(O) \
     ((O)->po_state == STICKY || (O)->po_state == CHANGED)
 
-#define PyPersist_CHANGED(O) \
-    PyPersist_C_API->reg_mgr((PyPersistObject *)(O))
+#define PyPersist_CHANGED(O) 				\
+    (PyPersist_C_API->reg_mgr((PyPersistObject *)(O)) ?	\
+     ((PyPersistObject *)(O))->po_state = CHANGED, 1 : 0)
 
 #define PyPersist_SetATime(O) \
     PyPersist_C_API->set_atime((PyPersistObject *)(O))
 
 /* Macros for compatibility with ZODB 3 C extensions. */
 
-#define PER_USE_OR_RETURN(O, R) \
-{ \
-    if (((O)->po_state == GHOST) \
-	&& (!PyPersist_C_API->load((PyPersistObject *)(O)))) { \
-        (O)->po_state = STICKY; \
-	return (R); \
-    } else if ((O)->po_state == UPTODATE) \
-	(O)->po_state = STICKY; \
+#define PER_USE_OR_RETURN(O, R) 				\
+{								\
+    if ((O)->po_state == GHOST) {				\
+	if (!PyPersist_C_API->load((PyPersistObject *)(O)))	\
+	    return (R);						\
+	(O)->po_state = STICKY;					\
+    } else if ((O)->po_state == UPTODATE) 			\
+	(O)->po_state = STICKY;					\
 }
 
-#define PER_CHANGED(O) PyPersist_C_API->reg_mgr((PyPersistObject *)(O))
+#define PER_CHANGED(O) \
+        PyPersist_C_API->reg_mgr((PyPersistObject *)(O)) ? -1 : 0
 
 #define PER_ALLOW_DEACTIVATION(O) \
 { \
@@ -70,10 +72,17 @@
 	(O)->po_state = STICKY; \
 }
 
-#define PER_USE(O) \
-    ((((PyPersistObject *)(O))->po_state != GHOST) \
-     || (PyPersist_C_API->load((PyPersistObject *)(O))) \
-     ? ((((PyPersistObject *)(O))->po_state == UPTODATE) \
-	? (((PyPersistObject *)(O))->po_state = STICKY) : 1) : 0)
+/* Macro to load object and mark sticky as needed.
+
+   If the object is in the UPTODATE state, the mark it sticky.
+   If the object is in the GHOST state, load it and mark it sticky.
+ */
+
+#define PER_USE(O) 						\
+    (((PyPersistObject *)(O))->po_state != GHOST ?		\
+     (((PyPersistObject *)(O))->po_state == UPTODATE ?		\
+      ((PyPersistObject *)(O))->po_state = STICKY, 1 : 1) :	\
+     (PyPersist_C_API->load((PyPersistObject *)(O)) ?		\
+      ((PyPersistObject *)(O))->po_state = STICKY, 1 : 0))
 
 #define PER_ACCESSED(O) PyPersist_C_API->set_atime((PyPersistObject *)O)


=== ZODB4/src/persistence/persistence.c 1.16 => 1.17 ===
--- ZODB4/src/persistence/persistence.c:1.16	Wed May  7 09:10:53 2003
+++ ZODB4/src/persistence/persistence.c	Tue May 20 15:01:39 2003
@@ -43,7 +43,10 @@
     PyObject *meth, *arg, *result;
 
     if (!self->po_dm)
-	return 0;
+	return 1;
+    /* If the object is in the CHANGED state, then it is already registered. */
+    if (self->po_state == CHANGED)
+	return 1;
     if (!s_register)
 	s_register = PyString_InternFromString("register");
     meth = PyObject_GetAttr((PyObject *)self->po_dm, s_register);
@@ -60,12 +63,11 @@
     Py_DECREF(arg);
     Py_DECREF(meth);
     if (result) {
-    if (self->po_state == UPTODATE || self->po_state == STICKY)
-	self->po_state = CHANGED;
+	if (self->po_state == UPTODATE || self->po_state == STICKY)
+	    self->po_state = CHANGED;
 	Py_DECREF(result);
 	return 1;
-    }
-    else
+    } else
 	return 0;
 }
 
@@ -77,6 +79,7 @@
 {
     static PyObject *s_setstate = NULL;
     PyObject *meth, *arg, *result;
+    enum PyPersist_State state;
 
     if (self->po_dm == NULL)
 	return 0;
@@ -94,7 +97,13 @@
     Py_INCREF(self);
     PyTuple_SET_ITEM(arg, 0, (PyObject *)self);
 
+    /* set state to CHANGED while setstate() call is in progress
+       to prevent a recursive call to _PyPersist_Load().
+    */
+    state = self->po_state;
+    self->po_state = CHANGED;
     result = PyObject_Call(meth, arg, NULL);
+    self->po_state = state;
 
     Py_DECREF(arg);
     Py_DECREF(meth);
@@ -240,16 +249,46 @@
 }
 
 static PyObject *
-persist_deactivate(PyPersistObject *self)
+persist_deactivate(PyPersistObject *self, PyObject *args, PyObject *keywords)
 {
-    if (self->po_state == UPTODATE && self->po_dm && self->po_oid) {
-	PyObject **pdict = _PyObject_GetDictPtr((PyObject *)self);
-	if (pdict && *pdict) {
-	    Py_DECREF(*pdict);
-	    *pdict = NULL;
+    int ghostify = 1;
+    PyObject *force = NULL;
+
+    if (args && PyTuple_GET_SIZE(args) > 0) {
+	PyErr_SetString(PyExc_TypeError, 
+			"_p_deactivate takes not positional arguments");
+	return NULL;
+    }
+    if (keywords) {
+	int size = PyDict_Size(keywords);
+	force = PyDict_GetItemString(keywords, "force");
+	if (force)
+	    size--;
+	if (size) {
+	    PyErr_SetString(PyExc_TypeError, 
+			    "_p_deactivate only accepts keyword arg force");
+	    return NULL;
 	}
-	self->po_state = GHOST;
     }
+
+    if (self->po_dm && self->po_oid) {
+	ghostify = self->po_state == UPTODATE;
+	if (!ghostify && force) {
+	    if (PyObject_IsTrue(force))
+		ghostify = 1;
+	    if (PyErr_Occurred())
+		return NULL;
+	}
+	if (ghostify) {
+	    PyObject **pdict = _PyObject_GetDictPtr((PyObject *)self);
+	    if (pdict && *pdict) {
+		Py_DECREF(*pdict);
+		*pdict = NULL;
+	    }
+	    self->po_state = GHOST;
+	}
+    }
+
     Py_INCREF(Py_None);
     return Py_None;
 }
@@ -293,13 +332,16 @@
 #define CHANGED_NONE 0
 #define CHANGED_FALSE 1
 #define CHANGED_TRUE 2
-#define CHANGED_DELETE 3
 
 static int
 persist_set_state(PyPersistObject *self, PyObject *v)
 {
+    int newstate, bool;
 
-    int newstate;
+    if (!v) {
+	PyErr_SetString(PyExc_TypeError, "can't delete _p_changed");
+	return -1;
+    }
     
     /* If the object isn't registered with a data manager, setting its
        state is meaningless.
@@ -307,14 +349,10 @@
     if (!self->po_dm || !self->po_oid)
 	return 0;
 
-    if (v == Py_None)
-	newstate = CHANGED_NONE;
-    else if (v == NULL)
-	newstate = CHANGED_DELETE;
-    else if (PyObject_IsTrue(v)) 
-	newstate = CHANGED_TRUE;
-    else 
-	newstate = CHANGED_FALSE;
+    bool = PyObject_IsTrue(v);
+    if (PyErr_Occurred())
+	return -1;
+    newstate = bool ? CHANGED_TRUE : CHANGED_FALSE;
 
     /* XXX I think the cases below cover all the transitions of
        interest.  We should really extend the interface / documentation
@@ -344,13 +382,6 @@
 	 */
 	if (self->po_state == CHANGED || self->po_state == STICKY)
 	    self->po_state = UPTODATE;
-    } else if (newstate == CHANGED_DELETE) {
-	/* Force the object to UPTODATE state to guarantee that
-	   call_p_deactivate() will turn it into a ghost.
-	*/
-	self->po_state = UPTODATE;
-	if (!call_p_deactivate(self, 0))
-	    return -1;
     } else if (self->po_state == UPTODATE) {
 	/* The final case is for CHANGED_NONE, which is only
 	   meaningful when the object is already in the up-to-date state. 
@@ -407,7 +438,7 @@
 */
 
 static int
-persist_checkattr(const char *s)
+persist_check_getattr(const char *s)
 {
     if (*s++ != '_')
 	return 1;
@@ -420,16 +451,21 @@
     }
     else if (*s == '_') {
 	s++;
-	if (*s == 'd') {
+	switch (*s) {
+	case 'd':
 	    s++;
 	    if (!strncmp(s, "ict__", 5))
 		return 0; /* __dict__ */
 	    if (!strncmp(s, "el__", 4))
 		return 0; /* __del__ */
 	    return 1;
+	case 'c':
+	    return strncmp(s, "class__", 7);
+	case 's':
+	    return strncmp(s, "setstate__", 10);
+	default:
+	    return 1;
 	}
-	else if (!strncmp(s, "class__", 7))
-	    return 0; /* __class__ */
     }
     return 1;
 }
@@ -452,7 +488,7 @@
        underscore.
     */
 
-    if (persist_checkattr(s_name)) {
+    if (persist_check_getattr(s_name)) {
 	if (self->po_state == GHOST) {
 	    /* Prevent the object from being registered as changed.
 
@@ -501,6 +537,31 @@
    1 : state loaded, attribute name is normal
 */
 
+/* Returns true if the object state must be loaded in setattr.
+
+   If any attribute other than _p_*, _v_*, or __dict__ is set,
+   the object must be unghostified.
+*/
+
+static int
+persist_check_setattr(const char *s)
+{
+    assert(s && *s);
+    if (*s++ != '_')
+	return 1;
+    switch (*s++) {
+    case 'p':
+    case 'v':
+	return *s != '_';
+	break;
+    case '_':
+	return strcmp(s, "dict__");
+	break;
+    default:
+	return 1;
+    }
+}
+
 static int
 persist_setattr_prep(PyPersistObject *self, PyObject *name, PyObject *value)
 {
@@ -515,16 +576,7 @@
        and excludes _p_ and _v_ attributes from the pickle.
     */
 
-    /* If any attribute other than an _p_ or _v_ attribute or __dict__ is 
-       being assigned to, make sure that the object state is loaded.  
-
-       Implement with simple check on s_name[0] to avoid multiple strncmp()
-       calls for all attribute names that don't start with an underscore.
-    */
-    if ((s_name[0] != '_')
-	|| ((strncmp(s_name, "_p_", 3) != 0)
-	    && (strncmp(s_name, "_v_", 3) != 0)
-	    && (strcmp(s_name, "__dict__") != 0))) {
+    if (persist_check_setattr(s_name)) {
 	if (self->po_state == GHOST) {
 	    if (self->po_dm == NULL || self->po_oid == NULL) {
 		PyErr_SetString(PyExc_TypeError,
@@ -533,6 +585,7 @@
 	    }
 	    if (!_PyPersist_Load((PyPersistObject *)self))
 		return -1;
+	    self->po_state = UPTODATE;
 	}
 	/* If the object is marked as UPTODATE then it must be
 	   registered as modified.  If it was just unghosted, it
@@ -541,6 +594,8 @@
 	   If it's in the changed state, it should already be registered.
 	   
 	   XXX What if it's in the sticky state?
+
+	   XXX It looks like these two cases could be collapsed somehow.
 	*/
 	if (self->po_state == UPTODATE && self->po_dm &&
 	    !_PyPersist_RegisterDataManager((PyPersistObject *)self))
@@ -721,7 +776,7 @@
     {"__getstate__", (PyCFunction)persist_getstate, METH_NOARGS, },
     {"__setstate__", persist_setstate, METH_O, },
     {"_p_activate", (PyCFunction)persist_activate, METH_NOARGS, },
-    {"_p_deactivate", (PyCFunction)persist_deactivate, METH_NOARGS, },
+    {"_p_deactivate", (PyCFunction)persist_deactivate, METH_KEYWORDS, },
     {"_p_setattr", (PyCFunction)persist_p_setattr, METH_VARARGS, },
     {"_p_delattr", (PyCFunction)persist_p_delattr, METH_O, },
     {NULL}