From a408350a08d4aadaec7573648ab91651c32a90df Mon Sep 17 00:00:00 2001 From: Antoine Pitrou Date: Sat, 28 Aug 2010 18:29:13 +0000 Subject: Merged revisions 84344 via svnmerge from svn+ssh://pythondev@svn.python.org/python/branches/py3k MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ........ r84344 | antoine.pitrou | 2010-08-28 20:17:03 +0200 (sam., 28 août 2010) | 4 lines Issue #1868: Eliminate subtle timing issues in thread-local objects by getting rid of the cached copy of thread-local attribute dictionary. ........ --- Include/object.h | 7 +++ Lib/_threading_local.py | 8 +++ Lib/test/test_threading_local.py | 62 ++++++++++++++++++++- Misc/NEWS | 3 ++ Modules/threadmodule.c | 81 +++++++++++++--------------- Objects/object.c | 113 ++++++++++++++++++++++----------------- 6 files changed, 182 insertions(+), 92 deletions(-) diff --git a/Include/object.h b/Include/object.h index dffe0f8..b368946 100644 --- a/Include/object.h +++ b/Include/object.h @@ -492,6 +492,13 @@ PyAPI_FUNC(void) PyObject_ClearWeakRefs(PyObject *); /* A slot function whose address we need to compare */ extern int _PyObject_SlotCompare(PyObject *, PyObject *); +/* Same as PyObject_Generic{Get,Set}Attr, but passing the attributes + dict as the last parameter. */ +PyAPI_FUNC(PyObject *) +_PyObject_GenericGetAttrWithDict(PyObject *, PyObject *, PyObject *); +PyAPI_FUNC(int) +_PyObject_GenericSetAttrWithDict(PyObject *, PyObject *, + PyObject *, PyObject *); /* PyObject_Dir(obj) acts like Python __builtin__.dir(obj), returning a diff --git a/Lib/_threading_local.py b/Lib/_threading_local.py index e953597..09a3515 100644 --- a/Lib/_threading_local.py +++ b/Lib/_threading_local.py @@ -195,6 +195,10 @@ class local(_localbase): lock.release() def __setattr__(self, name, value): + if name == '__dict__': + raise AttributeError( + "%r object attribute '__dict__' is read-only" + % self.__class__.__name__) lock = object.__getattribute__(self, '_local__lock') lock.acquire() try: @@ -204,6 +208,10 @@ class local(_localbase): lock.release() def __delattr__(self, name): + if name == '__dict__': + raise AttributeError( + "%r object attribute '__dict__' is read-only" + % self.__class__.__name__) lock = object.__getattribute__(self, '_local__lock') lock.acquire() try: diff --git a/Lib/test/test_threading_local.py b/Lib/test/test_threading_local.py index 35c0889..4c9f296 100644 --- a/Lib/test/test_threading_local.py +++ b/Lib/test/test_threading_local.py @@ -125,6 +125,67 @@ class BaseLocalTest: self.assertRaises(TypeError, cls, a=1) self.assertRaises(TypeError, cls, 1) + def _test_one_class(self, c): + self._failed = "No error message set or cleared." + obj = c() + e1 = threading.Event() + e2 = threading.Event() + + def f1(): + obj.x = 'foo' + obj.y = 'bar' + del obj.y + e1.set() + e2.wait() + + def f2(): + try: + foo = obj.x + except AttributeError: + # This is expected -- we haven't set obj.x in this thread yet! + self._failed = "" # passed + else: + self._failed = ('Incorrectly got value %r from class %r\n' % + (foo, c)) + sys.stderr.write(self._failed) + + t1 = threading.Thread(target=f1) + t1.start() + e1.wait() + t2 = threading.Thread(target=f2) + t2.start() + t2.join() + # The test is done; just let t1 know it can exit, and wait for it. + e2.set() + t1.join() + + self.assertFalse(self._failed, self._failed) + + def test_threading_local(self): + self._test_one_class(self._local) + + def test_threading_local_subclass(self): + class LocalSubclass(self._local): + """To test that subclasses behave properly.""" + self._test_one_class(LocalSubclass) + + def _test_dict_attribute(self, cls): + obj = cls() + obj.x = 5 + self.assertEqual(obj.__dict__, {'x': 5}) + with self.assertRaises(AttributeError): + obj.__dict__ = {} + with self.assertRaises(AttributeError): + del obj.__dict__ + + def test_dict_attribute(self): + self._test_dict_attribute(self._local) + + def test_dict_attribute_subclass(self): + class LocalSubclass(self._local): + """To test that subclasses behave properly.""" + self._test_dict_attribute(LocalSubclass) + class ThreadLocalTest(unittest.TestCase, BaseLocalTest): _local = _thread._local @@ -142,7 +203,6 @@ class ThreadLocalTest(unittest.TestCase, BaseLocalTest): gc.collect() self.assertIs(wr(), None) - class PyThreadingLocalTest(unittest.TestCase, BaseLocalTest): _local = _threading_local.local diff --git a/Misc/NEWS b/Misc/NEWS index deceec4..fe04f43 100644 --- a/Misc/NEWS +++ b/Misc/NEWS @@ -305,6 +305,9 @@ Core and Builtins Library ------- +- Issue #1868: Eliminate subtle timing issues in thread-local objects by + getting rid of the cached copy of thread-local attribute dictionary. + - Issue #9125: Add recognition of 'except ... as ...' syntax to parser module. Extension Modules diff --git a/Modules/threadmodule.c b/Modules/threadmodule.c index a89158e..53a833c 100644 --- a/Modules/threadmodule.c +++ b/Modules/threadmodule.c @@ -14,6 +14,7 @@ #include "pythread.h" static PyObject *ThreadError; +static PyObject *str_dict; static long nb_threads = 0; /* Lock objects */ @@ -264,8 +265,6 @@ typedef struct { PyObject *key; PyObject *args; PyObject *kw; - /* The current thread's local dict (necessary for tp_dictoffset) */ - PyObject *dict; PyObject *weakreflist; /* List of weak references to self */ /* A {localdummy weakref -> localdict} dict */ PyObject *dummies; @@ -277,9 +276,9 @@ typedef struct { static PyObject *_ldict(localobject *self); static PyObject *_localdummy_destroyed(PyObject *meth_self, PyObject *dummyweakref); -/* Create and register the dummy for the current thread, as well as the - corresponding local dict */ -static int +/* Create and register the dummy for the current thread. + Returns a borrowed reference of the corresponding local dict */ +static PyObject * _local_create_dummy(localobject *self) { PyObject *tdict, *ldict = NULL, *wr = NULL; @@ -315,15 +314,14 @@ _local_create_dummy(localobject *self) goto err; Py_CLEAR(dummy); - Py_CLEAR(self->dict); - self->dict = ldict; - return 0; + Py_DECREF(ldict); + return ldict; err: Py_XDECREF(ldict); Py_XDECREF(wr); Py_XDECREF(dummy); - return -1; + return NULL; } static PyObject * @@ -369,7 +367,7 @@ local_new(PyTypeObject *type, PyObject *args, PyObject *kw) if (self->wr_callback == NULL) goto err; - if (_local_create_dummy(self) < 0) + if (_local_create_dummy(self) == NULL) goto err; return (PyObject *)self; @@ -385,7 +383,6 @@ local_traverse(localobject *self, visitproc visit, void *arg) Py_VISIT(self->args); Py_VISIT(self->kw); Py_VISIT(self->dummies); - Py_VISIT(self->dict); return 0; } @@ -396,7 +393,6 @@ local_clear(localobject *self) Py_CLEAR(self->args); Py_CLEAR(self->kw); Py_CLEAR(self->dummies); - Py_CLEAR(self->dict); Py_CLEAR(self->wr_callback); /* Remove all strong references to dummies from the thread states */ if (self->key @@ -442,9 +438,9 @@ _ldict(localobject *self) dummy = PyDict_GetItem(tdict, self->key); if (dummy == NULL) { - if (_local_create_dummy(self) < 0) + ldict = _local_create_dummy(self); + if (ldict == NULL) return NULL; - ldict = self->dict; if (Py_TYPE(self)->tp_init != PyBaseObject_Type.tp_init && Py_TYPE(self)->tp_init((PyObject*)self, @@ -461,14 +457,6 @@ _ldict(localobject *self) ldict = ((localdummyobject *) dummy)->localdict; } - /* The call to tp_init above may have caused another thread to run. - Install our ldict again. */ - if (self->dict != ldict) { - Py_INCREF(ldict); - Py_CLEAR(self->dict); - self->dict = ldict; - } - return ldict; } @@ -476,29 +464,25 @@ static int local_setattro(localobject *self, PyObject *name, PyObject *v) { PyObject *ldict; + int r; ldict = _ldict(self); if (ldict == NULL) return -1; - return PyObject_GenericSetAttr((PyObject *)self, name, v); -} + r = PyObject_RichCompareBool(name, str_dict, Py_EQ); + if (r == 1) { + PyErr_Format(PyExc_AttributeError, + "'%.50s' object attribute '__dict__' is read-only", + Py_TYPE(self)->tp_name); + return -1; + } + if (r == -1) + return -1; -static PyObject * -local_getdict(localobject *self, void *closure) -{ - PyObject *ldict; - ldict = _ldict(self); - Py_XINCREF(ldict); - return ldict; + return _PyObject_GenericSetAttrWithDict((PyObject *)self, name, v, ldict); } -static PyGetSetDef local_getset[] = { - {"__dict__", (getter)local_getdict, (setter)NULL, - "Local-data dictionary", NULL}, - {NULL} /* Sentinel */ -}; - static PyObject *local_getattro(localobject *, PyObject *); static PyTypeObject localtype = { @@ -532,12 +516,12 @@ static PyTypeObject localtype = { /* tp_iternext */ 0, /* tp_methods */ 0, /* tp_members */ 0, - /* tp_getset */ local_getset, + /* tp_getset */ 0, /* tp_base */ 0, /* tp_dict */ 0, /* internal use */ /* tp_descr_get */ 0, /* tp_descr_set */ 0, - /* tp_dictoffset */ offsetof(localobject, dict), + /* tp_dictoffset */ 0, /* tp_init */ 0, /* tp_alloc */ 0, /* tp_new */ local_new, @@ -549,20 +533,29 @@ static PyObject * local_getattro(localobject *self, PyObject *name) { PyObject *ldict, *value; + int r; ldict = _ldict(self); if (ldict == NULL) return NULL; + r = PyObject_RichCompareBool(name, str_dict, Py_EQ); + if (r == 1) { + Py_INCREF(ldict); + return ldict; + } + if (r == -1) + return NULL; + if (Py_TYPE(self) != &localtype) /* use generic lookup for subtypes */ - return PyObject_GenericGetAttr((PyObject *)self, name); + return _PyObject_GenericGetAttrWithDict((PyObject *)self, name, ldict); /* Optimization: just look in dict ourselves */ value = PyDict_GetItem(ldict, name); if (value == NULL) /* Fall back on generic to get __class__ and __dict__ */ - return PyObject_GenericGetAttr((PyObject *)self, name); + return _PyObject_GenericGetAttrWithDict((PyObject *)self, name, ldict); Py_INCREF(value); return value; @@ -587,8 +580,6 @@ _localdummy_destroyed(PyObject *localweakref, PyObject *dummyweakref) PyObject *ldict; ldict = PyDict_GetItem(self->dummies, dummyweakref); if (ldict != NULL) { - if (ldict == self->dict) - Py_CLEAR(self->dict); PyDict_DelItem(self->dummies, dummyweakref); } if (PyErr_Occurred()) @@ -931,6 +922,10 @@ initthread(void) nb_threads = 0; + str_dict = PyString_InternFromString("__dict__"); + if (str_dict == NULL) + return; + /* Initialize the C thread library */ PyThread_init_thread(); } diff --git a/Objects/object.c b/Objects/object.c index e4252c5..b6ad5de 100644 --- a/Objects/object.c +++ b/Objects/object.c @@ -1317,7 +1317,7 @@ _PyObject_NextNotImplemented(PyObject *self) /* Generic GetAttr functions - put these in your tp_[gs]etattro slot */ PyObject * -PyObject_GenericGetAttr(PyObject *obj, PyObject *name) +_PyObject_GenericGetAttrWithDict(PyObject *obj, PyObject *name, PyObject *dict) { PyTypeObject *tp = Py_TYPE(obj); PyObject *descr = NULL; @@ -1395,36 +1395,37 @@ PyObject_GenericGetAttr(PyObject *obj, PyObject *name) } } - /* Inline _PyObject_GetDictPtr */ - dictoffset = tp->tp_dictoffset; - if (dictoffset != 0) { - PyObject *dict; - if (dictoffset < 0) { - Py_ssize_t tsize; - size_t size; - - tsize = ((PyVarObject *)obj)->ob_size; - if (tsize < 0) - tsize = -tsize; - size = _PyObject_VAR_SIZE(tp, tsize); - - dictoffset += (long)size; - assert(dictoffset > 0); - assert(dictoffset % SIZEOF_VOID_P == 0); - } - dictptr = (PyObject **) ((char *)obj + dictoffset); - dict = *dictptr; - if (dict != NULL) { - Py_INCREF(dict); - res = PyDict_GetItem(dict, name); - if (res != NULL) { - Py_INCREF(res); - Py_XDECREF(descr); - Py_DECREF(dict); - goto done; + if (dict == NULL) { + /* Inline _PyObject_GetDictPtr */ + dictoffset = tp->tp_dictoffset; + if (dictoffset != 0) { + if (dictoffset < 0) { + Py_ssize_t tsize; + size_t size; + + tsize = ((PyVarObject *)obj)->ob_size; + if (tsize < 0) + tsize = -tsize; + size = _PyObject_VAR_SIZE(tp, tsize); + + dictoffset += (long)size; + assert(dictoffset > 0); + assert(dictoffset % SIZEOF_VOID_P == 0); } + dictptr = (PyObject **) ((char *)obj + dictoffset); + dict = *dictptr; + } + } + if (dict != NULL) { + Py_INCREF(dict); + res = PyDict_GetItem(dict, name); + if (res != NULL) { + Py_INCREF(res); + Py_XDECREF(descr); Py_DECREF(dict); + goto done; } + Py_DECREF(dict); } if (f != NULL) { @@ -1447,8 +1448,15 @@ PyObject_GenericGetAttr(PyObject *obj, PyObject *name) return res; } +PyObject * +PyObject_GenericGetAttr(PyObject *obj, PyObject *name) +{ + return _PyObject_GenericGetAttrWithDict(obj, name, NULL); +} + int -PyObject_GenericSetAttr(PyObject *obj, PyObject *name, PyObject *value) +_PyObject_GenericSetAttrWithDict(PyObject *obj, PyObject *name, + PyObject *value, PyObject *dict) { PyTypeObject *tp = Py_TYPE(obj); PyObject *descr; @@ -1494,27 +1502,29 @@ PyObject_GenericSetAttr(PyObject *obj, PyObject *name, PyObject *value) } } - dictptr = _PyObject_GetDictPtr(obj); - if (dictptr != NULL) { - PyObject *dict = *dictptr; - if (dict == NULL && value != NULL) { - dict = PyDict_New(); - if (dict == NULL) - goto done; - *dictptr = dict; - } - if (dict != NULL) { - Py_INCREF(dict); - if (value == NULL) - res = PyDict_DelItem(dict, name); - else - res = PyDict_SetItem(dict, name, value); - if (res < 0 && PyErr_ExceptionMatches(PyExc_KeyError)) - PyErr_SetObject(PyExc_AttributeError, name); - Py_DECREF(dict); - goto done; + if (dict == NULL) { + dictptr = _PyObject_GetDictPtr(obj); + if (dictptr != NULL) { + dict = *dictptr; + if (dict == NULL && value != NULL) { + dict = PyDict_New(); + if (dict == NULL) + goto done; + *dictptr = dict; + } } } + if (dict != NULL) { + Py_INCREF(dict); + if (value == NULL) + res = PyDict_DelItem(dict, name); + else + res = PyDict_SetItem(dict, name, value); + if (res < 0 && PyErr_ExceptionMatches(PyExc_KeyError)) + PyErr_SetObject(PyExc_AttributeError, name); + Py_DECREF(dict); + goto done; + } if (f != NULL) { res = f(descr, obj, value); @@ -1536,6 +1546,13 @@ PyObject_GenericSetAttr(PyObject *obj, PyObject *name, PyObject *value) return res; } +int +PyObject_GenericSetAttr(PyObject *obj, PyObject *name, PyObject *value) +{ + return _PyObject_GenericSetAttrWithDict(obj, name, value, NULL); +} + + /* Test a value used as condition, e.g., in a for or if statement. Return -1 if an error occurred */ -- cgit v0.12