Re: [Python-Dev] constant/enum type in stdlib

[Ben . Cottrell]

On Tue, 23 Nov 2010 15:15:29 +, Michael Foord wrote:
 There are still two reasonable APIs (unless you have changed your mind 
 and think that sticking with plain integers is best), of which I prefer 
 the latter:
 
 SOME_CONST = Constant('SOME_CONST', 1)
 OTHER_CONST = Constant('OTHER_CONST', 2)
 
 or:
 
 Constants = make_constants('Constants', 'SOME_CONST OTHER_CONST', start=1)
 SOME_CONST = Constants.SOME_CONST
 OTHER_CONST = Constants.OTHER_CONST

I prefer the latter too, because that makes it possible to have
'Constants' be a rendezvous point for making sure that you're
passing something valid. Perhaps using 'in':

def func(foo):
if foo not in Constants:
raise ValueError('foo must be SOME_CONST or OTHER_CONST')
...

I know this is probably not going to happen, but I would *so much*
like it if functions would start rejecting the wrong kind of 2.
Constants that are valid, integer-wise, but which aren't part of
the set of constants allowed for that argument. I'd prefer not to
think of the number of times I've made the following mistake:

s = socket.socket(socket.SOCK_DGRAM, socket.AF_INET)

~Ben
___
Python-Dev mailing list
Python-Dev@python.org
http://mail.python.org/mailman/listinfo/python-dev
Unsubscribe: 
http://mail.python.org/mailman/options/python-dev/archive%40mail-archive.com

[Python-Dev] Reference leak in thread._local

[Ben . Cottrell]

I noticed that thread._local can leak references if objects are
being stored inside the thread._local object whose destructors
might release the GIL.

The way this happens is that in Modules/threadmodule.c, in the
_ldict() function, it does things like this:

Py_CLEAR(self-dict);
Py_INCREF(ldict);
self-dict = ldict;

If the Py_CLEAR ends up taking away the last reference to an object
contained in the dict, and a thread context switch occurs during that
object's deallocation, then self-dict might not be NULL on return
from Py_CLEAR; another thread might have run, accessed something in
the same thread._local object, and caused self-dict to be set to
something else (and Py_INCREF'ed). So when we blindly do the
assignment into self-dict, we may be overwriting a valid reference,
and not properly Py_DECREFing it.

The recent change (revision 64601 to threadmodule.c) did not address
context switches during the Py_CLEAR call; only context switches
during tp_init.

The attached patch (against trunk) is my first attempt at fixing this.
It detects if self-dict has been set to something else after the
Py_CLEAR, and retries the Py_CLEAR (because _ldict really only cares
about installing the proper value of self-dict for the currently
running thread).

However, I am still uncomfortable about the fact that local_getattro
and local_setattro discard the value returned from _ldict, and instead
hand off control to the PyObject_Generic layer and trust that by the
time self-dict is actually used, it still has the correct value for
the current thread. Would it be better to, say, inline a copy of the
PyObject_Generic* functions inside local_getattro/local_setattro,
and force the operations to be done on the actual dict returned by
_ldict()?

Thanks,

~Ben
Index: Modules/threadmodule.c
===
--- Modules/threadmodule.c	(revision 66050)
+++ Modules/threadmodule.c	(working copy)
@@ -278,7 +278,9 @@
 return NULL;
 		}
 
-		Py_CLEAR(self-dict);
+		while (self-dict != NULL) {
+			Py_CLEAR(self-dict);
+		}
 		Py_INCREF(ldict);
 		self-dict = ldict; /* still borrowed */
 
@@ -297,7 +299,9 @@
 	/* The call to tp_init above may have caused another thread to run.
 	   Install our ldict again. */
 	if (self-dict != ldict) {
-		Py_CLEAR(self-dict);
+		while (self-dict != NULL) {
+			Py_CLEAR(self-dict);
+		}
 		Py_INCREF(ldict);
 		self-dict = ldict;
 	}
___
Python-Dev mailing list
Python-Dev@python.org
http://mail.python.org/mailman/listinfo/python-dev
Unsubscribe: 
http://mail.python.org/mailman/options/python-dev/archive%40mail-archive.com