Merged revisions 84344 via svnmerge from

svn+ssh://pythondev@svn.python.org/python/branches/py3k

........
  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.
........

6 files changed, 183 insertions, 92 deletions

diff --git a/Include/object.h b/Include/object.h
index 6e744bc..ef73a21 100644
--- a/Include/object.h
+++ b/Include/object.h
@@ -448,6 +448,14 @@ PyAPI_FUNC(int) PyCallable_Check(PyObject *);
 
 PyAPI_FUNC(void) PyObject_ClearWeakRefs(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 builtins.dir(obj), returning a
    list of strings.  PyObject_Dir(NULL) is like builtins.dir(),
diff --git a/Lib/_threading_local.py b/Lib/_threading_local.py
index 6e8a8d8..afdd6ea 100644
--- a/Lib/_threading_local.py
+++ b/Lib/_threading_local.py
@@ -194,6 +194,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:
@@ -203,6 +207,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 0c9f4d9..c9d0fdd 100644
--- a/Lib/test/test_threading_local.py
+++ b/Lib/test/test_threading_local.py
@@ -112,6 +112,67 @@ class BaseLocalTest:
 
         self.assertTrue(passed)
 
+    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
@@ -129,7 +190,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 4942fa2..5075c38 100644
--- a/Misc/NEWS
+++ b/Misc/NEWS
@@ -97,6 +97,9 @@ C-API
 Library
 -------
 
+- Issue #1868: Eliminate subtle timing issues in thread-local objects by
+  getting rid of the cached copy of thread-local attribute dictionary.
+
 - Issue #8797: urllib2 does a retry for Basic Authentication failure instead of
   falling into recursion.
 
diff --git a/Modules/_threadmodule.c b/Modules/_threadmodule.c
index ad55f6d..c37dc20 100644
--- a/Modules/_threadmodule.c
+++ b/Modules/_threadmodule.c
@@ -13,6 +13,7 @@
 #include "pythread.h"
 
 static PyObject *ThreadError;
+static PyObject *str_dict;
 
 
 /* Lock objects */
@@ -259,8 +260,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;
@@ -272,9 +271,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;
@@ -310,15 +309,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 *
@@ -364,7 +362,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;
@@ -380,7 +378,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;
 }
 
@@ -391,7 +388,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
@@ -437,9 +433,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,
@@ -456,14 +452,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;
 }
 
@@ -471,29 +459,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 '%U' is read-only",
+                     Py_TYPE(self)->tp_name, 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 = {
@@ -527,12 +511,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,
@@ -544,20 +528,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;
@@ -582,8 +575,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 @@ PyInit__thread(void)
     if (PyModule_AddObject(m, "_local", (PyObject *)&localtype) < 0)
         return NULL;
 
+    str_dict = PyUnicode_InternFromString("__dict__");
+    if (str_dict == NULL)
+        return NULL;
+
     /* Initialize the C thread library */
     PyThread_init_thread();
     return m;
diff --git a/Objects/object.c b/Objects/object.c
index 57ed6eb..2e6ddfa 100644
--- a/Objects/object.c
+++ b/Objects/object.c
@@ -911,7 +911,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;
@@ -971,36 +971,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) {
@@ -1023,8 +1024,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;
@@ -1056,27 +1064,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);
@@ -1098,6 +1108,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 */