[pypy-svn] pypy default: Write an obscure refcounting workaround for a common misusage

[arigo]

Author: Armin Rigo <arigo at tunes.org>
Branch: 
Changeset: r42661:5e783074ef82
Date: 2011-03-15 13:10 -0400
http://bitbucket.org/pypy/pypy/changeset/5e783074ef82/

Log:	Write an obscure refcounting workaround for a common misusage of
	PyModule_AddObject().

diff --git a/pypy/module/cpyext/test/test_cpyext.py b/pypy/module/cpyext/test/test_cpyext.py
--- a/pypy/module/cpyext/test/test_cpyext.py
+++ b/pypy/module/cpyext/test/test_cpyext.py
@@ -92,6 +92,8 @@
     self.frozen_ll2callocations = set(ll2ctypes.ALLOCATED.values())
 
 class LeakCheckingTest(object):
+    enable_leak_checking = True
+
     @staticmethod
     def cleanup_references(space):
         state = space.fromcache(RefcountState)
@@ -113,6 +115,10 @@
         # check for sane refcnts
         import gc
 
+        if not self.enable_leak_checking:
+            leakfinder.stop_tracking_allocations(check=False)
+            return False
+
         leaking = False
         state = self.space.fromcache(RefcountState)
         gc.collect()

diff --git a/pypy/module/cpyext/test/test_arraymodule.py b/pypy/module/cpyext/test/test_arraymodule.py
--- a/pypy/module/cpyext/test/test_arraymodule.py
+++ b/pypy/module/cpyext/test/test_arraymodule.py
@@ -4,6 +4,8 @@
 import sys
 
 class AppTestArrayModule(AppTestCpythonExtensionBase):
+    enable_leak_checking = False
+
     def test_basic(self):
         module = self.import_module(name='array')
         arr = module.array('i', [1,2,3])

diff --git a/pypy/module/cpyext/test/test_capsule.py b/pypy/module/cpyext/test/test_capsule.py
--- a/pypy/module/cpyext/test/test_capsule.py
+++ b/pypy/module/cpyext/test/test_capsule.py
@@ -12,6 +12,7 @@
                  if (PyErr_Occurred()) return NULL;
                  module = PyImport_ImportModule("foo");
                  PyModule_AddObject(module, "_ptr", capsule);
+                 Py_DECREF(capsule);  /* XXX <--- anti-workaround */
                  Py_DECREF(module);
                  if (PyErr_Occurred()) return NULL;
                  Py_RETURN_NONE;

diff --git a/pypy/module/cpyext/src/modsupport.c b/pypy/module/cpyext/src/modsupport.c
--- a/pypy/module/cpyext/src/modsupport.c
+++ b/pypy/module/cpyext/src/modsupport.c
@@ -665,10 +665,12 @@
 	return tmp;
 }
 
-int
-PyModule_AddObject(PyObject *m, const char *name, PyObject *o)
+/* returns -1 in case of error, 0 if a new key was added, 1 if the key
+   was already there (and replaced) */
+static int
+_PyModule_AddObject_NoConsumeRef(PyObject *m, const char *name, PyObject *o)
 {
-	PyObject *dict;
+	PyObject *dict, *prev;
 	if (!PyModule_Check(m)) {
 		PyErr_SetString(PyExc_TypeError,
 			    "PyModule_AddObject() needs module as first arg");
@@ -688,32 +690,47 @@
 			     PyModule_GetName(m));
 		return -1;
 	}
+	prev = PyDict_GetItemString(dict, name);
 	if (PyDict_SetItemString(dict, name, o))
 		return -1;
-	Py_DECREF(o);
-	return 0;
+	return prev != NULL;
+}
+
+int
+PyModule_AddObject(PyObject *m, const char *name, PyObject *o)
+{
+	int result = _PyModule_AddObject_NoConsumeRef(m, name, o);
+	/* XXX WORKAROUND for a common misusage of PyModule_AddObject:
+	   for the common case of adding a new key, we don't consume a
+	   reference, but instead just leak it away.  The issue is that
+	   people generally don't realize that this function consumes a
+	   reference, because on CPython the reference is still stored
+	   on the dictionary. */
+	if (result != 0)
+		Py_DECREF(o);
+	return result < 0 ? -1 : 0;
 }
 
 int 
 PyModule_AddIntConstant(PyObject *m, const char *name, long value)
 {
+	int result;
 	PyObject *o = PyInt_FromLong(value);
 	if (!o)
 		return -1;
-	if (PyModule_AddObject(m, name, o) == 0)
-		return 0;
+	result = _PyModule_AddObject_NoConsumeRef(m, name, o);
 	Py_DECREF(o);
-	return -1;
+	return result < 0 ? -1 : 0;
 }
 
 int 
 PyModule_AddStringConstant(PyObject *m, const char *name, const char *value)
 {
+	int result;
 	PyObject *o = PyString_FromString(value);
 	if (!o)
 		return -1;
-	if (PyModule_AddObject(m, name, o) == 0)
-		return 0;
+	result = _PyModule_AddObject_NoConsumeRef(m, name, o);
 	Py_DECREF(o);
-	return -1;
+	return result < 0 ? -1 : 0;
 }

diff --git a/pypy/module/cpyext/test/test_pycobject.py b/pypy/module/cpyext/test/test_pycobject.py
--- a/pypy/module/cpyext/test/test_pycobject.py
+++ b/pypy/module/cpyext/test/test_pycobject.py
@@ -12,6 +12,7 @@
                  if (PyErr_Occurred()) return NULL;
                  module = PyImport_ImportModule("foo");
                  PyModule_AddObject(module, "_ptr", pointer);
+                 Py_DECREF(pointer);  /* XXX <--- anti-workaround */
                  Py_DECREF(module);
                  if (PyErr_Occurred()) return NULL;
                  Py_RETURN_NONE;