[issue3710] Reference leak in thread._local
Ben Cottrell added the comment: The latest patch over in #1868 is working fine for my company in production, and solves #3710 as well. I think the only thing left to do on that patch is to make it special case "__dict__". -- ___ Python tracker <http://bugs.python.org/issue3710> ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue3710] Reference leak in thread._local
New submission from Ben Cottrell <[EMAIL PROTECTED]>: This is a copy of a message I sent to the python-dev mailing list; it was suggested in a reply that I file a bug for this issue. I'm filing it against Python 2.5 because that's where I noticed it, but it doesn't look like this code has changed much in trunk. 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()? -- components: Extension Modules files: threadmodule.c.diff keywords: patch messages: 72052 nosy: tamino severity: normal status: open title: Reference leak in thread._local type: behavior versions: Python 2.5 Added file: http://bugs.python.org/file11278/threadmodule.c.diff ___ Python tracker <[EMAIL PROTECTED]> <http://bugs.python.org/issue3710> ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue3710] Reference leak in thread._local
Ben Cottrell <[EMAIL PROTECTED]> added the comment: But then if there is a context switch during the last Py_XDECREF, then it could be the case that self->dict is not set properly on return from _ldict(). Functions like local_setattro() use _ldict() more for its side effect (setting self->dict) than for its return value. It's possible that this should be changed; see the last paragraph in my original report. ___ Python tracker <[EMAIL PROTECTED]> <http://bugs.python.org/issue3710> ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue3710] Reference leak in thread._local
Ben Cottrell <[EMAIL PROTECTED]> added the comment: The specific thing that was happening for me is that an _sqlite3.Connection object was in the dictionary. In Modules/_sqlite/connection.c, in pysqlite_connection_dealloc(), it uses Py_BEGIN_ALLOW_THREADS/Py_END_ALLOW_THREADS. So it's the call to Py_DECREF that's interesting from my point of view. I believe that if _ldict() sets self->dict to what it should be for the current thread, and then calls Py_DECREF on the old value, and then returns, then _ldict() is no longer able to guarantee that self->dict will be set to the right thing for the current thread after it returns (because if the Py_DECREF ended up deallocating something like an sqlite3 connection, then it'd have released and reacquired the GIL). Hence, in the patch I attached, the assignment into self->dict is kept as the last thing that happens before _ldict() returns, and I believe this means _ldict() can still make that guarantee. Of course, I'd be all for changing local_getattro/local_setattro to not need _ldict to make that guarantee! _ldict always *returns* the correct pointer; it would be nice to make use of that somehow. ___ Python tracker <[EMAIL PROTECTED]> <http://bugs.python.org/issue3710> ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue1868] threading.local doesn't free attrs when assigning thread exits
Ben Cottrell <[EMAIL PROTECTED]> added the comment: I like this patch, too! I think it's a much cleaner way of implementing the thread._local type. However, when I test it, I have problems with subclasses of thread._local; using the class itself seems to work. I've attached a test program that shows the issue. -- nosy: +tamino Added file: http://bugs.python.org/file11296/test1868.py ___ Python tracker <[EMAIL PROTECTED]> <http://bugs.python.org/issue1868> ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue1868] threading.local doesn't free attrs when assigning thread exits
Ben Cottrell <[EMAIL PROTECTED]> added the comment: Christian, Your patch works for me -- thanks!! I made a slight modification to your patch to allow "del" to work, and have attached my modified version. I agree that allowing subclassing makes thread._local harder to get right than it would otherwise be. There is code out there that uses that feature, though -- I'm running into it in the context of django, which (when using the sqlite database back end) keeps its sqlite connections in a subclass of thread._local. Added file: http://bugs.python.org/file11298/threading_local3.patch ___ Python tracker <[EMAIL PROTECTED]> <http://bugs.python.org/issue1868> ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
