diff -r 36cd1cf5a160 Doc/whatsnew/3.3.rst --- a/Doc/whatsnew/3.3.rst Wed Jun 06 15:57:18 2012 +0200 +++ b/Doc/whatsnew/3.3.rst Wed Jun 06 12:50:57 2012 -0500 @@ -1427,6 +1427,18 @@ (Contributed by Victor Stinner in :issue:`10278`) +tkinter +------- + +The :mod:`tkinter` module is now (more) reentrant. Previously, callback +functions registered with the Tcl interpreter via its C API (e.g. by extension +modules) would deadlock the interpreter if they called Python functions which +in turn invoked tkinter methods. Even single-threaded applications could +trigger such a deadlock. tkinter now allows this form of reentrance. + +(Contributed by John Bollinger in :issue:`14390`) + + types ----- diff -r 36cd1cf5a160 Lib/test/test_tk.py --- a/Lib/test/test_tk.py Wed Jun 06 15:57:18 2012 +0200 +++ b/Lib/test/test_tk.py Wed Jun 06 12:50:57 2012 -0500 @@ -16,7 +16,7 @@ support.use_resources.append('gui') support.run_unittest( - *runtktests.get_tests(text=False, packages=['test_tkinter'])) + *runtktests.get_tests(packages=['test_tkinter'])) if __name__ == '__main__': test_main(enable_gui=True) diff -r 36cd1cf5a160 Lib/tkinter/test/test_tkinter/test_tcl_deadlock.py --- /dev/null Thu Jan 01 00:00:00 1970 +0000 +++ b/Lib/tkinter/test/test_tkinter/test_tcl_deadlock.py Wed Jun 06 12:50:57 2012 -0500 @@ -0,0 +1,125 @@ +# +# test_tcl_deadlock.py +# +# Copyright (C) 2012 John C. Bollinger. All rights reserved. +# +# Licensed to PSF under a Contributor Agreement +# + +import sys +import os +from unittest import TestCase +from test.support import captured_output, rmtree, run_unittest +from tkinter import Tcl +from multiprocessing import Process +from tempfile import mkdtemp +from distutils.core import Distribution, Extension +from distutils.command.build_ext import build_ext +from distutils.sysconfig import get_config_var +from distutils.tests.support import fixup_build_ext + +# A TestCase that tests tkinter's behavior in situations that caused deadlock +# in some versions of Python. Even though it tests tkinter, this case does not +# actually require a GUI because it relies only on tkinter's underlying Tcl +# support framework. The Tk subsystem is never initialized. +class DeadlockTest(TestCase): + + def setUp(self): + super().setUp() + self.tempdir = mkdtemp() + self.build_extension() + + def tearDown(self): + rmtree(self.tempdir) + self.tempdir = None + super().tearDown() + + # Test whether tkinter's Tcl lock is reentrant. In a mixed Python / C[++] + # program, a C callback can call Python functions. When such a callback + # does so, it will hold tkinter's Tcl lock (unlike when tkinter's internal + # C functions do the same thing). If the Python function invokes tkinter + # methods then they will try to acquire the Tcl lock again, and that attempt + # will deadlock unless the Tcl lock is reentrant. + def test_tcl_deadlock(self): + + # This is all it takes to cause a deadlock in susceptible tkinter + def try_deadlock(): + sys.path.append(self.tempdir) + import tclqueue + + # The Tk subsystem is not needed. Tcl alone is sufficient. + tcl = Tcl() + + # Queue up a call to a function whose invocation by Tcl will + # deadlock a susceptible tkinter + tclqueue.queue_call(lambda: tcl.eval('puts {callback called}')) + + # Cause the queued event to be processed. Susceptible tkinter + # implementations do not return from this call. + tcl.dooneevent() + + # This function is called in a child process; no need to unload + # tclqueue from the child or restore the child's copy of sys.path. + + child = Process(target = try_deadlock) + child.start() + + # Attempt to join the process running the deadlock code. 5 seconds + # should be plenty of time, provided the child does not deadlock. + child.join(5) + + # If the child process is still alive then it must have deadlocked + if child.is_alive(): + + # Terminate the deadlocked process. The ability to do this is the + # primary reason for using a Process instead of a Thread (or the + # main thread). + child.terminate() + + self.fail('tkinter deadlocked') + + # Builds the extension module "tclqueue" used by this test case + def build_extension(self): + source_name = 'tclqueuemodule.c' + extension_source = get_source_path(source_name) + + if extension_source == None: + self.fail("Could not build extension: source file %s not found" + % (source_name, )) + + extension = Extension( + name = 'tclqueue', + sources = [extension_source], + define_macros=[("PYTHON%d" % (sys.version_info[0],), '1')], + libraries=['tcl'] + ) + dist = Distribution({'name' : 'tkinter deadlock test', + 'ext_modules' : [extension]}) + dist.package_dir = self.tempdir + cmd = build_ext(dist) + fixup_build_ext(cmd) + cmd.build_lib = self.tempdir + cmd.build_temp = self.tempdir + cmd.ensure_finalized() + with captured_output('stdout'): + cmd.run() + +# Tries to find the specified source file, returning the full path to it if +# successful. Currently it checks the directory in which this module resides +# and the Modules/ subdirectory of this build's source directory. +def get_source_path(file): + srcdir = get_config_var('srcdir') + candidates = [ + # use an installed copy if available + os.path.join(os.path.dirname(__file__), file), + # otherwise try using a copy from build directory + os.path.join(srcdir, 'Modules', file), + ] + for path in candidates: + if os.path.exists(path): + return path + +tests_nogui = (DeadlockTest, ) + +if __name__ == "__main__": + run_unittest(*tests_nogui) diff -r 36cd1cf5a160 Misc/ACKS --- a/Misc/ACKS Wed Jun 06 15:57:18 2012 +0200 +++ b/Misc/ACKS Wed Jun 06 12:50:57 2012 -0500 @@ -106,6 +106,7 @@ Paul Boddie Matthew Boedicker David Bolen +John Bollinger Gawain Bolton Forest Bond Gregory Bond diff -r 36cd1cf5a160 Misc/NEWS --- a/Misc/NEWS Wed Jun 06 15:57:18 2012 +0200 +++ b/Misc/NEWS Wed Jun 06 12:50:57 2012 -0500 @@ -548,6 +548,10 @@ - Issue #14493: Use gvfs-open or xdg-open in webbrowser. +- Issue #14390: Avoid tkinter deadlock when Python callbacks are registered + with Tcl via its C API, and those callbacks call Python functions that in + turn call tkinter methods. + Build ----- diff -r 36cd1cf5a160 Modules/_tkinter.c --- a/Modules/_tkinter.c Wed Jun 06 15:57:18 2012 +0200 +++ b/Modules/_tkinter.c Wed Jun 06 12:50:57 2012 -0500 @@ -4,6 +4,11 @@ All Rights Reserved ******************************************************************/ +/*********************************************************** +Some portions +Copyright (C) 2012 John C. Bollinger. All rights reserved. +Licensed to PSF under a Contributor Agreement. +******************************************************************/ /* _tkinter.c -- Interface to libtk.a and libtcl.a. */ @@ -130,8 +135,15 @@ other Python threads need to run while Tcl is blocked waiting for events. To solve this problem, a separate lock for Tcl is introduced. Holding it - is incompatible with holding Python's interpreter lock. The following four - macros manipulate both locks together. + is incompatible with holding Python's interpreter lock. The Tcl lock + presents a tricky problem, however, because it is possible for a thread + holding the Tcl lock to call Python API functions which in turn invoke + Tkinter methods. In such cases, that thread will attempt to acquire the + Tcl lock while already holding it, and it will deadlock against itself + unless the locking code is reentrant. The Python API does not natively + provide reentrant locking. + + The following four macros manipulate both locks together. ENTER_TCL and LEAVE_TCL are brackets, just like Py_BEGIN_ALLOW_THREADS and Py_END_ALLOW_THREADS. They should be used whenever a call into Tcl is made @@ -171,39 +183,78 @@ In addition, for a threaded Tcl, a single global tcl_tstate won't be sufficient anymore, since multiple Tcl interpreters may simultaneously dispatch in different threads. So we use the Tcl TLS API. - */ static PyThread_type_lock tcl_lock = 0; #ifdef TCL_THREADS static Tcl_ThreadDataKey state_key; -typedef PyThreadState *ThreadSpecificData; -#define tcl_tstate (*(PyThreadState**)Tcl_GetThreadData(&state_key, sizeof(PyThreadState*))) +typedef struct { + /* The stored Python thread state for this thread: */ + PyThreadState *tcl_tstate; + /* + * The number of times this thread has acquired the Tcl lock minus the + * number of times it has released it: + */ + int lock_count; +} state_data_t; +/* + * Within the macros that follow, USE_LOCK_DATA retrieves state information + * from Tcl's TLS for this thread. In some cases (especially ENTER_PYTHON / + * LEAVE_PYTHON) it might be slightly more efficient to put USE_LOCK_DATA + * directly into the C code, but this is not done because it would disturb + * the well-established usage patterns of the other macros. + */ +#define USE_LOCK_DATA state_data_t *state_data = \ + (state_data_t *) Tcl_GetThreadData(&state_key, sizeof(state_data_t)); +#define tcl_tstate state_data->tcl_tstate +#define tcl_lock_count state_data->lock_count #else +#define USE_LOCK_DATA static PyThreadState *tcl_tstate = NULL; +static int tcl_lock_count = 0; #endif -#define ENTER_TCL \ - { PyThreadState *tstate = PyThreadState_Get(); Py_BEGIN_ALLOW_THREADS \ - if(tcl_lock)PyThread_acquire_lock(tcl_lock, 1); tcl_tstate = tstate; +#define ENTER_TCL { \ + USE_LOCK_DATA \ + PyThreadState *tstate = PyThreadState_Get(); \ + Py_BEGIN_ALLOW_THREADS \ + if (tcl_lock && (tcl_lock_count++ == 0)) \ + PyThread_acquire_lock(tcl_lock, 1); \ + tcl_tstate = tstate; #define LEAVE_TCL \ - tcl_tstate = NULL; if(tcl_lock)PyThread_release_lock(tcl_lock); Py_END_ALLOW_THREADS} + tcl_tstate = NULL; \ + if (tcl_lock && (--tcl_lock_count == 0)) \ + PyThread_release_lock(tcl_lock); \ + Py_END_ALLOW_THREADS \ +} #define ENTER_OVERLAP \ Py_END_ALLOW_THREADS #define LEAVE_OVERLAP_TCL \ - tcl_tstate = NULL; if(tcl_lock)PyThread_release_lock(tcl_lock); } - -#define ENTER_PYTHON \ - { PyThreadState *tstate = tcl_tstate; tcl_tstate = NULL; \ - if(tcl_lock)PyThread_release_lock(tcl_lock); PyEval_RestoreThread((tstate)); } - -#define LEAVE_PYTHON \ - { PyThreadState *tstate = PyEval_SaveThread(); \ - if(tcl_lock)PyThread_acquire_lock(tcl_lock, 1); tcl_tstate = tstate; } + tcl_tstate = NULL; \ + if (tcl_lock && (--tcl_lock_count == 0)) \ + PyThread_release_lock(tcl_lock); \ +} + +#define ENTER_PYTHON { \ + USE_LOCK_DATA \ + PyThreadState *tstate = tcl_tstate; \ + tcl_tstate = NULL; \ + if (tcl_lock && (--tcl_lock_count == 0)) \ + PyThread_release_lock(tcl_lock); \ + PyEval_RestoreThread(tstate); \ +} + +#define LEAVE_PYTHON { \ + USE_LOCK_DATA \ + PyThreadState *tstate = PyEval_SaveThread(); \ + if (tcl_lock && (tcl_lock_count++ == 0)) \ + PyThread_acquire_lock(tcl_lock, 1); \ + tcl_tstate = tstate; \ +} #define CHECK_TCL_APPARTMENT \ if (((TkappObject *)self)->threaded && \ @@ -2543,12 +2594,15 @@ LEAVE_TCL } else { + USE_LOCK_DATA Py_BEGIN_ALLOW_THREADS - if(tcl_lock)PyThread_acquire_lock(tcl_lock, 1); + if (tcl_lock && (tcl_lock_count++ == 0)) + PyThread_acquire_lock(tcl_lock, 1); tcl_tstate = tstate; result = Tcl_DoOneEvent(TCL_DONT_WAIT); tcl_tstate = NULL; - if(tcl_lock)PyThread_release_lock(tcl_lock); + if (tcl_lock && (--tcl_lock_count == 0)) + PyThread_release_lock(tcl_lock); if (result == 0) Sleep(Tkinter_busywaitinterval); Py_END_ALLOW_THREADS @@ -3007,14 +3061,17 @@ } #endif #if defined(WITH_THREAD) || defined(MS_WINDOWS) + USE_LOCK_DATA Py_BEGIN_ALLOW_THREADS - if(tcl_lock)PyThread_acquire_lock(tcl_lock, 1); + if (tcl_lock && (tcl_lock_count++ == 0)) + PyThread_acquire_lock(tcl_lock, 1); tcl_tstate = event_tstate; result = Tcl_DoOneEvent(TCL_DONT_WAIT); tcl_tstate = NULL; - if(tcl_lock)PyThread_release_lock(tcl_lock); + if (tcl_lock && (--tcl_lock_count == 0)) + PyThread_release_lock(tcl_lock); if (result == 0) Sleep(Tkinter_busywaitinterval); Py_END_ALLOW_THREADS diff -r 36cd1cf5a160 Modules/tclqueuemodule.c --- /dev/null Thu Jan 01 00:00:00 1970 +0000 +++ b/Modules/tclqueuemodule.c Wed Jun 06 12:50:57 2012 -0500 @@ -0,0 +1,141 @@ +/* + * tclqueuemodule.c + * + * An extension module used in some of tkinter's tests. Not part of the + * standard library. + * + * Copyright (C) 2012 John C. Bollinger. All rights reserved. + * Licensed to PSF under a Contributor Agreement. + */ + +#include +#include + +/* type definitions */ +typedef struct { + Tcl_Event tcl_event; + PyObject *callable; +} python_call_event; + +/* internal functions */ +static int call_callable(PyObject *py_callback); +static int callable_arg(PyObject *func, void *func_p); +static int handle_event(Tcl_Event *evPtr, int flags); + +/* functions implementing module methods */ +static PyObject *queue_call(PyObject *self, PyObject *args); + +/* module method definitions */ +static PyMethodDef DeadlocktestMethods[] = { + { "queue_call", queue_call, METH_VARARGS, NULL }, + { NULL, NULL, 0, NULL} +}; + +/* + * module method implementation: + * Queues a call to the specified Python Callable to be performed by the Tcl + * event loop of the Tcl interpreter associated with the current thread. + */ +static PyObject * +queue_call(PyObject *self, PyObject *args) +{ + PyObject *callback; + python_call_event *event; + + if (!PyArg_ParseTuple(args, "O&", callable_arg, &callback)) + return NULL; + + /* Create and initialize a python_call_event */ + event = (python_call_event *) ckalloc(sizeof(python_call_event)); + if (event == NULL) { + PyErr_SetString(PyExc_MemoryError, + "cannot allocate memory for a Tcl event"); + return NULL; + } + event->tcl_event.proc = handle_event; + event->callable = callback; + + /* Queue the event for THIS THREAD'S Tcl interpreter */ + Tcl_QueueEvent(&(event->tcl_event), TCL_QUEUE_HEAD); + + /* + * We need to own a reference to the callback; it will be released once + * the callback has been called by handle_event + */ + Py_INCREF(callback); + + Py_RETURN_NONE; +} + +/* + * Handles a python_call_event by calling the associated callable + */ +static int handle_event(Tcl_Event *event, int flags) +{ + python_call_event *call_event = (python_call_event *) event; + + call_callable(call_event->callable); + Py_DECREF(call_event->callable); + + return 1; +} + +static int call_callable(PyObject *callable) +{ + /* + * When this function is called via tkinter, the current thread might not + * (probably doesn't) hold the GIL + */ + PyGILState_STATE gstate = PyGILState_Ensure(); + + PyObject *result = PyObject_CallFunction(callable, NULL); + Py_XDECREF(result); + + PyGILState_Release(gstate); + + return ((result == NULL) ? 0 : 1); +} + +/* + * A Converter used with PyArg_ParseTuple() O& format to verify that an + * argument is Callable. If so, the argument is returned as-is; otherwise a + * conversion error is reported. + */ +static int callable_arg(PyObject *func, void *func_p) +{ + if (!PyCallable_Check(func)) { + PyErr_SetString(PyExc_TypeError, "argument is not a function"); + return 0; + } else { + *(PyObject **) func_p = func; + return 1; + } +} + +/* module initialization function */ +#ifdef PYTHON2 + +PyMODINIT_FUNC +inittclqueue(void) +{ + Py_InitModule("tclqueue", DeadlocktestMethods); +} + +#else + +static struct PyModuleDef tclqueuemodule = { + PyModuleDef_HEAD_INIT, + "tclqueue", + NULL, + -1, + DeadlocktestMethods +}; + +PyMODINIT_FUNC +PyInit_tclqueue(void) +{ + return PyModule_Create(&tclqueuemodule); +} + +#endif +