From c5f5d55816b553e825c9ab3d8a0b249f7eb7dff7 Mon Sep 17 00:00:00 2001 From: Ivan Pozdeev Date: Tue, 10 Apr 2018 15:23:30 +0300 Subject: [PATCH 01/14] Fix race conditions for non-threaded Tcl when passing data to/from it --- Modules/_tkinter.c | 73 +++++++++++++++++++++++++++------------------- 1 file changed, 43 insertions(+), 30 deletions(-) diff --git a/Modules/_tkinter.c b/Modules/_tkinter.c index 444c268c0b4738..66120d9daadb0f 100644 --- a/Modules/_tkinter.c +++ b/Modules/_tkinter.c @@ -216,6 +216,9 @@ static PyThreadState *tcl_tstate = NULL; #define ENTER_OVERLAP \ Py_END_ALLOW_THREADS +#define LEAVE_OVERLAP \ + Py_BEGIN_ALLOW_THREADS + #define LEAVE_OVERLAP_TCL \ tcl_tstate = NULL; if(tcl_lock)PyThread_release_lock(tcl_lock); } @@ -239,6 +242,7 @@ static PyThreadState *tcl_tstate = NULL; #define ENTER_TCL #define LEAVE_TCL #define ENTER_OVERLAP +#define LEAVE_OVERLAP #define LEAVE_OVERLAP_TCL #define ENTER_PYTHON #define LEAVE_PYTHON @@ -1644,24 +1648,28 @@ Tkapp_Call(PyObject *selfptr, PyObject *args) #endif { + ENTER_TCL + ENTER_OVERLAP + objv = Tkapp_CallArgs(args, objStore, &objc); - if (!objv) - return NULL; - ENTER_TCL + if (objv) { - i = Tcl_EvalObjv(self->interp, objc, objv, flags); + LEAVE_OVERLAP - ENTER_OVERLAP + i = Tcl_EvalObjv(self->interp, objc, objv, flags); - if (i == TCL_ERROR) - Tkinter_Error(selfptr); - else - res = Tkapp_CallResult(self); + ENTER_OVERLAP - LEAVE_OVERLAP_TCL + if (i == TCL_ERROR) + Tkinter_Error(selfptr); + else + res = Tkapp_CallResult(self); - Tkapp_CallDeallocArgs(objv, objStore, objc); + Tkapp_CallDeallocArgs(objv, objStore, objc); + + } + LEAVE_OVERLAP_TCL } return res; } @@ -1952,19 +1960,20 @@ SetVar(PyObject *self, PyObject *args, int flags) if (!PyArg_ParseTuple(args, "O&O:setvar", varname_converter, &name1, &newValue)) return NULL; - /* XXX Acquire tcl lock??? */ - newval = AsObj(newValue); - if (newval == NULL) - return NULL; ENTER_TCL - ok = Tcl_SetVar2Ex(Tkapp_Interp(self), name1, NULL, - newval, flags); ENTER_OVERLAP - if (!ok) - Tkinter_Error(self); - else { - res = Py_None; - Py_INCREF(res); + newval = AsObj(newValue); + if (newval) { + LEAVE_OVERLAP + ok = Tcl_SetVar2Ex(Tkapp_Interp(self), name1, NULL, + newval, flags); + ENTER_OVERLAP + if (!ok) + Tkinter_Error(self); + else { + res = Py_None; + Py_INCREF(res); + } } LEAVE_OVERLAP_TCL break; @@ -1974,16 +1983,20 @@ SetVar(PyObject *self, PyObject *args, int flags) return NULL; CHECK_STRING_LENGTH(name1); CHECK_STRING_LENGTH(name2); - /* XXX must hold tcl lock already??? */ - newval = AsObj(newValue); ENTER_TCL - ok = Tcl_SetVar2Ex(Tkapp_Interp(self), name1, name2, newval, flags); ENTER_OVERLAP - if (!ok) - Tkinter_Error(self); - else { - res = Py_None; - Py_INCREF(res); + newval = AsObj(newValue); + if (newval) { + LEAVE_OVERLAP + ok = Tcl_SetVar2Ex(Tkapp_Interp(self), name1, name2, + newval, flags); + ENTER_OVERLAP + if (!ok) + Tkinter_Error(self); + else { + res = Py_None; + Py_INCREF(res); + } } LEAVE_OVERLAP_TCL break; From 999914941ad4cc5da9e1124ab3338a93b9fc62be Mon Sep 17 00:00:00 2001 From: Ivan Pozdeev Date: Tue, 10 Apr 2018 19:03:42 +0300 Subject: [PATCH 02/14] Document the added complexity of TCL bracket macros more clearly --- Modules/_tkinter.c | 23 +++++++++++++---------- 1 file changed, 13 insertions(+), 10 deletions(-) diff --git a/Modules/_tkinter.c b/Modules/_tkinter.c index 66120d9daadb0f..817afc4f1b49ae 100644 --- a/Modules/_tkinter.c +++ b/Modules/_tkinter.c @@ -151,23 +151,26 @@ Copyright (C) 1994 Steen Lumholt. interpreter lock was used for this. However, this causes problems when 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. + + To solve this problem, a separate lock for Tcl is introduced. + We normally treat holding it as incompatible with holding Python's + interpreter lock. The following 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 that could call an event handler, or otherwise affect the state of a Tcl interpreter. These assume that the surrounding code has the Python - interpreter lock; inside the brackets, the Python interpreter lock has been - released and the lock for Tcl has been acquired. + interpreter lock: ENTER_TCL releases the Python lock, then acquires the Tcl + lock, and LEAVE_TCL does the reverse. Sometimes, it is necessary to have both the Python lock and the Tcl lock. - (For example, when transferring data from the Tcl interpreter result to a - Python string object.) This can be done by using different macros to close - the ENTER_TCL block: ENTER_OVERLAP reacquires the Python lock (and restores - the thread state) but doesn't release the Tcl lock; LEAVE_OVERLAP_TCL - releases the Tcl lock. + (For example, when transferring data between the Tcl interpreter and/or + objects and Python objects.) To avoid deadlocks, when acquiring, we always + acquire the Tcl lock first, then the Python lock. The additional macros for + finer lock control are: ENTER_OVERLAP acquires the Python lock (and restores + the thread state) when already holding the Tcl lock; LEAVE_OVERLAP releases + the Python lock and keeps the Tcl lock; and LEAVE_OVERLAP_TCL releases the + Tcl lock and keeps the Python lock. By contrast, ENTER_PYTHON and LEAVE_PYTHON are used in Tcl event handlers when the handler needs to use Python. Such event handlers are From 5b69c1ffb3a97632cc8529c04cdeacf53a0330b4 Mon Sep 17 00:00:00 2001 From: Ivan Pozdeev Date: Sat, 14 Apr 2018 01:27:28 +0300 Subject: [PATCH 03/14] Add NEWS entry for bpo-33257 --- .../NEWS.d/next/Library/2018-04-14-01-26-10.bpo-33257.i32qOW.rst | 1 + 1 file changed, 1 insertion(+) create mode 100644 Misc/NEWS.d/next/Library/2018-04-14-01-26-10.bpo-33257.i32qOW.rst diff --git a/Misc/NEWS.d/next/Library/2018-04-14-01-26-10.bpo-33257.i32qOW.rst b/Misc/NEWS.d/next/Library/2018-04-14-01-26-10.bpo-33257.i32qOW.rst new file mode 100644 index 00000000000000..d8611ec82b046f --- /dev/null +++ b/Misc/NEWS.d/next/Library/2018-04-14-01-26-10.bpo-33257.i32qOW.rst @@ -0,0 +1 @@ +Fixed race conditions in Tkinter when passing data to/from nonthreaded Tcl From 55494f4f5ff69a0f178e2806b0eb91cc5911a17f Mon Sep 17 00:00:00 2001 From: Ivan Pozdeev Date: Sun, 15 Apr 2018 19:59:21 +0300 Subject: [PATCH 04/14] * Tkapp_CallArgs, Tkapp_CallResult, AsObj require both locks * Tkapp_CallDeallocArgs requires Tcl lock --- Modules/_tkinter.c | 32 ++++++++++++++++++++++++-------- 1 file changed, 24 insertions(+), 8 deletions(-) diff --git a/Modules/_tkinter.c b/Modules/_tkinter.c index 817afc4f1b49ae..486fafa6c9474a 100644 --- a/Modules/_tkinter.c +++ b/Modules/_tkinter.c @@ -233,6 +233,13 @@ static PyThreadState *tcl_tstate = NULL; { PyThreadState *tstate = PyEval_SaveThread(); \ if(tcl_lock)PyThread_acquire_lock(tcl_lock, 1); tcl_tstate = tstate; } +#define ENTER_PYTHON_OVERLAP \ +{ PyThreadState *tstate = tcl_tstate; tcl_tstate = NULL; PyEval_RestoreThread((tstate)); } + +#define LEAVE_PYTHON_OVERLAP \ +{ PyThreadState *tstate = PyEval_SaveThread(); tcl_tstate = tstate; } + + #define CHECK_TCL_APPARTMENT \ if (((TkappObject *)self)->threaded && \ ((TkappObject *)self)->thread_id != Tcl_GetCurrentThread()) { \ @@ -249,6 +256,8 @@ static PyThreadState *tcl_tstate = NULL; #define LEAVE_OVERLAP_TCL #define ENTER_PYTHON #define LEAVE_PYTHON +#define ENTER_PYTHON_OVERLAP +#define LEAVE_PYTHON_OVERLAP #define CHECK_TCL_APPARTMENT #endif @@ -1551,17 +1560,17 @@ Tkapp_CallProc(Tkapp_CallEvent *e, int flags) Tcl_Obj **objv; int objc; int i; - ENTER_PYTHON + ENTER_PYTHON_OVERLAP objv = Tkapp_CallArgs(e->args, objStore, &objc); if (!objv) { PyErr_Fetch(e->exc_type, e->exc_value, e->exc_tb); *(e->res) = NULL; } - LEAVE_PYTHON + LEAVE_PYTHON_OVERLAP if (!objv) goto done; i = Tcl_EvalObjv(e->self->interp, objc, objv, e->flags); - ENTER_PYTHON + ENTER_PYTHON_OVERLAP if (i == TCL_ERROR) { *(e->res) = NULL; *(e->exc_type) = NULL; @@ -1573,7 +1582,7 @@ Tkapp_CallProc(Tkapp_CallEvent *e, int flags) else { *(e->res) = Tkapp_CallResult(e->self); } - LEAVE_PYTHON + LEAVE_PYTHON_OVERLAP Tkapp_CallDeallocArgs(objv, objStore, objc); done: @@ -1669,7 +1678,11 @@ Tkapp_Call(PyObject *selfptr, PyObject *args) else res = Tkapp_CallResult(self); + LEAVE_OVERLAP + Tkapp_CallDeallocArgs(objv, objStore, objc); + + ENTER_OVERLAP } LEAVE_OVERLAP_TCL @@ -2492,15 +2505,18 @@ PythonCmd(ClientData clientData, Tcl_Interp *interp, int argc, char *argv[]) if (res == NULL) return PythonCmd_Error(interp); + ENTER_TCL + ENTER_OVERLAP obj_res = AsObj(res); + if (obj_res) { + Tcl_SetObjResult(interp, obj_res); + rv = TCL_OK; + } + LEAVE_OVERLAP_TCL if (obj_res == NULL) { Py_DECREF(res); return PythonCmd_Error(interp); } - else { - Tcl_SetObjResult(interp, obj_res); - rv = TCL_OK; - } Py_DECREF(res); From 24f813ad92d7cb6384d3e95e156e44c6472e139c Mon Sep 17 00:00:00 2001 From: Ivan Pozdeev Date: Sat, 12 May 2018 03:49:15 +0300 Subject: [PATCH 05/14] Regrtest bpo-33257 --- Lib/tkinter/test/test_tkinter/test_threads.py | 144 ++++++++++++++++++ 1 file changed, 144 insertions(+) create mode 100644 Lib/tkinter/test/test_tkinter/test_threads.py diff --git a/Lib/tkinter/test/test_tkinter/test_threads.py b/Lib/tkinter/test/test_tkinter/test_threads.py new file mode 100644 index 00000000000000..2dd18fa0f2b5f4 --- /dev/null +++ b/Lib/tkinter/test/test_tkinter/test_threads.py @@ -0,0 +1,144 @@ +#!/usr/bin/python3 + +import sys +import time +import threading +import random +import tkinter + +import unittest +class TestThreads(unittest.TestCase): + def test_threads(self): + import subprocess + p = subprocess.Popen([sys.executable, __file__, "run"], + stdout=subprocess.PIPE, + stderr=subprocess.PIPE) + self.addCleanup(p.stdout.close) + self.addCleanup(p.stderr.close) + try: + #Test code is designed to complete in a few seconds + stdout, stderr = p.communicate(timeout=10) + except subprocess.TimeoutExpired: + p.kill() + stdout, stderr = p.communicate() + self.fail("Test code hang. Stderr: " + repr(stderr)) + rc = p.returncode + self.assertTrue(rc == 0, + "Nonzero exit status: " + str(rc) + "; stderr:" + repr(stderr)) + self.assertTrue(len(stderr) == 0, "stderr: " + repr(stderr)) + + + +running = True + +class Track(threading.Thread): + """ + Calculates coordinates for a ballistic track + with random angle and velocity + and fires the callback to draw each consecutive segment. + """ + def __init__(self, track_number, draw_callback): + """ + :param track_number: ordinal + :param draw_callback: fn(track_number, x, y) + that draws the extention of the specified track + to the specified point. The callback must keep track + of any previous coordinates itself. + """ + threading.Thread.__init__(self) + #self.setDaemon(True) + self.track_number = track_number + self.draw_callback = draw_callback + + def run(self): + #starting point for the track + y = 0.0001 #height + x = 999.0 #range + #initial velocities + yVel = 400. + random.random() * 200. + xVel = -200. - random.random() * 150. + # Stop drawing when the track hits the ground + while y > 0 and running: + #How long to sleep, in seconds, between track updates + time.sleep(0.01) #realism: >1 Fun: 0.01 + + yVel -= 9.8 #gravity, more or less + xVel *= .9998 #air resistance + + y += yVel + x += xVel + + if running: + self.draw_callback(self.track_number, x, y) + + +class App: + """ + The main app logic + """ + def __init__(self, window): + self.track_no = 0 #last index of track added + self.track_coordinates = {} #coords of track tips + self.threads = [] + + self.window = window + self.frame = tkinter.Frame(window) + self.frame.pack() + self.graph = tkinter.Canvas(self.frame) + self.graph.pack() + + self.t_cleanup = threading.Thread(target=self.tf_cleanup) + self.window.after(0, self.add_tracks, 5) + self.window.after(1000, self.t_cleanup.start) + + def add_tracks(self,depth): + if depth>0: self.window.after(5, self.add_tracks, depth-1) + tracks_to_add = 40 + start_no = self.track_no + self.track_no += tracks_to_add + for self.track_no in range(start_no, self.track_no): + #self.window.after(t, self.add_tracks) + #self.track_no += 1 #Make a new track number + #if (self.track_no > 40): + t = Track(self.track_no, self.draw_track_segment) + self.threads.append(t) + t.start() + + def tf_cleanup(self): + global running + running = False + for t in self.threads: t.join() + self.window.after(0,self.window.destroy) + + def draw_track_segment(self, track_no, x, y): + # x & y are virtual coordinates for the purpose of simulation. + #To convert them to screen coordinates, + # we scale them down so the graphs fit into the window, and move the origin. + # Y is also inverted because in canvas, the UL corner is the origin. + newsx, newsy = (250.+x/100., 150.-y/100.) + + try: + (oldsx, oldsy) = self.track_coordinates[track_no] + except KeyError: + pass + else: + self.graph.create_line(oldsx, oldsy, + newsx, newsy) + self.track_coordinates[track_no] = (newsx, newsy) + + def go(self): + self.window.mainloop() + +tests_gui = (TestThreads,) + +if __name__=='__main__': + import sys,os + if sys.argv[1:]==['run']: + if os.name == 'nt': + import ctypes + #the bug causes crashes + ctypes.windll.kernel32.SetErrorMode(3) + App(tkinter.Tk()).go() + else: + from test.support import run_unittest + run_unittest(*tests_gui) From d70c6596f452fc5990d82d9fbf63b33c6f36a045 Mon Sep 17 00:00:00 2001 From: Ivan Pozdeev Date: Sun, 15 Apr 2018 20:09:05 +0300 Subject: [PATCH 06/14] * PythonCmd_Error doesn't use the arg and thus can do without Tcl lock --- Modules/_tkinter.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/Modules/_tkinter.c b/Modules/_tkinter.c index 486fafa6c9474a..ee7c0f5bf3ac88 100644 --- a/Modules/_tkinter.c +++ b/Modules/_tkinter.c @@ -2462,7 +2462,7 @@ typedef struct { } PythonCmd_ClientData; static int -PythonCmd_Error(Tcl_Interp *interp) +PythonCmd_Error(void) { errorInCmd = 1; PyErr_Fetch(&excInCmd, &valInCmd, &trbInCmd); @@ -2490,20 +2490,20 @@ PythonCmd(ClientData clientData, Tcl_Interp *interp, int argc, char *argv[]) /* Create argument list (argv1, ..., argvN) */ if (!(arg = PyTuple_New(argc - 1))) - return PythonCmd_Error(interp); + return PythonCmd_Error(); for (i = 0; i < (argc - 1); i++) { PyObject *s = fromTclString(argv[i + 1]); if (!s || PyTuple_SetItem(arg, i, s)) { Py_DECREF(arg); - return PythonCmd_Error(interp); + return PythonCmd_Error(); } } res = PyEval_CallObject(func, arg); Py_DECREF(arg); if (res == NULL) - return PythonCmd_Error(interp); + return PythonCmd_Error(); ENTER_TCL ENTER_OVERLAP @@ -2515,7 +2515,7 @@ PythonCmd(ClientData clientData, Tcl_Interp *interp, int argc, char *argv[]) LEAVE_OVERLAP_TCL if (obj_res == NULL) { Py_DECREF(res); - return PythonCmd_Error(interp); + return PythonCmd_Error(); } Py_DECREF(res); From 77daeccf2f53202cf1831d4877f2526de12f2ead Mon Sep 17 00:00:00 2001 From: Ivan Pozdeev Date: Wed, 25 Apr 2018 01:57:25 +0300 Subject: [PATCH 07/14] Make all Tcl lock operations into macros for easier tracking --- Modules/_tkinter.c | 38 ++++++++++++++++++++------------------ 1 file changed, 20 insertions(+), 18 deletions(-) diff --git a/Modules/_tkinter.c b/Modules/_tkinter.c index ee7c0f5bf3ac88..e118c02e09c9ad 100644 --- a/Modules/_tkinter.c +++ b/Modules/_tkinter.c @@ -210,12 +210,24 @@ static PyThreadState *tcl_tstate = NULL; #endif #define ENTER_TCL \ - { PyThreadState *tstate = PyThreadState_Get(); Py_BEGIN_ALLOW_THREADS \ - if(tcl_lock)PyThread_acquire_lock(tcl_lock, 1); tcl_tstate = tstate; + { PyThreadState *tstate = PyThreadState_Get();\ + ENTER_TCL_CUSTOM_TSTATE(tstate) + +#define ENTER_TCL_CUSTOM_TSTATE(tstate) \ + Py_BEGIN_ALLOW_THREADS\ + if (tcl_lock)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} +#define LEAVE_TCL_WITH_BUSYWAIT(result) \ + tcl_tstate = NULL; \ + if (tcl_lock)PyThread_release_lock(tcl_lock);\ + if (result == 0)\ + Sleep(Tkinter_busywaitinterval);\ + Py_END_ALLOW_THREADS + + #define ENTER_OVERLAP \ Py_END_ALLOW_THREADS @@ -250,7 +262,9 @@ static PyThreadState *tcl_tstate = NULL; #else #define ENTER_TCL +#define ENTER_TCL_CUSTOM_TSTATE(tstate) #define LEAVE_TCL +#define LEAVE_TCL_WITH_BUSYWAIT(result) #define ENTER_OVERLAP #define LEAVE_OVERLAP #define LEAVE_OVERLAP_TCL @@ -3081,15 +3095,9 @@ Tkapp_MainLoop(PyObject *selfptr, PyObject *args) LEAVE_TCL } else { - Py_BEGIN_ALLOW_THREADS - if(tcl_lock)PyThread_acquire_lock(tcl_lock, 1); - tcl_tstate = tstate; + ENTER_TCL_CUSTOM_TSTATE(tstate) result = Tcl_DoOneEvent(TCL_DONT_WAIT); - tcl_tstate = NULL; - if(tcl_lock)PyThread_release_lock(tcl_lock); - if (result == 0) - Sleep(Tkinter_busywaitinterval); - Py_END_ALLOW_THREADS + LEAVE_TCL_WITH_BUSYWAIT(result) } #else result = Tcl_DoOneEvent(0); @@ -3595,17 +3603,11 @@ EventHook(void) } #endif #if defined(WITH_THREAD) || defined(MS_WINDOWS) - Py_BEGIN_ALLOW_THREADS - if(tcl_lock)PyThread_acquire_lock(tcl_lock, 1); - tcl_tstate = event_tstate; + ENTER_TCL_CUSTOM_TSTATE(event_tstate) result = Tcl_DoOneEvent(TCL_DONT_WAIT); - tcl_tstate = NULL; - if(tcl_lock)PyThread_release_lock(tcl_lock); - if (result == 0) - Sleep(Tkinter_busywaitinterval); - Py_END_ALLOW_THREADS + LEAVE_TCL_WITH_BUSYWAIT(result) #else result = Tcl_DoOneEvent(0); #endif From b642cace73e29521c73d9a75461a5c7a23eef030 Mon Sep 17 00:00:00 2001 From: Ivan Pozdeev Date: Wed, 16 May 2018 05:29:54 +0300 Subject: [PATCH 08/14] Make Tcl lock reentrant --- Modules/_tkinter.c | 39 +++++++++++++++++++++++++++++++++------ 1 file changed, 33 insertions(+), 6 deletions(-) diff --git a/Modules/_tkinter.c b/Modules/_tkinter.c index e118c02e09c9ad..add86faebd2b23 100644 --- a/Modules/_tkinter.c +++ b/Modules/_tkinter.c @@ -200,6 +200,9 @@ Copyright (C) 1994 Steen Lumholt. */ static PyThread_type_lock tcl_lock = 0; +/* Only the thread holding the lock can modify these */ +static unsigned long tcl_lock_thread_ident = 0; +static unsigned int tcl_lock_reentry_count = 0; #ifdef TCL_THREADS static Tcl_ThreadDataKey state_key; @@ -209,20 +212,43 @@ typedef PyThreadState *ThreadSpecificData; static PyThreadState *tcl_tstate = NULL; #endif +#define ACQUIRE_TCL_LOCK \ +if (tcl_lock) {\ + if (tcl_lock_thread_ident == PyThread_get_thread_ident()) {\ + tcl_lock_reentry_count++; \ + if(!tcl_lock_reentry_count) \ + Py_FatalError("Tcl lock reentry count overflow"); \ + } else { \ + PyThread_acquire_lock(tcl_lock, 1); \ + tcl_lock_thread_ident = PyThread_get_thread_ident(); \ + }\ +} + +#define RELEASE_TCL_LOCK \ +if (tcl_lock){\ + if (tcl_lock_reentry_count) { \ + tcl_lock_reentry_count--; \ + } else { \ + tcl_lock_thread_ident = 0; \ + PyThread_release_lock(tcl_lock); \ + }\ +} + #define ENTER_TCL \ { PyThreadState *tstate = PyThreadState_Get();\ ENTER_TCL_CUSTOM_TSTATE(tstate) #define ENTER_TCL_CUSTOM_TSTATE(tstate) \ Py_BEGIN_ALLOW_THREADS\ - if (tcl_lock)PyThread_acquire_lock(tcl_lock, 1); tcl_tstate = tstate; + ACQUIRE_TCL_LOCK; tcl_tstate = tstate; #define LEAVE_TCL \ - tcl_tstate = NULL; if(tcl_lock)PyThread_release_lock(tcl_lock); Py_END_ALLOW_THREADS} + tcl_tstate = NULL; \ + RELEASE_TCL_LOCK; Py_END_ALLOW_THREADS} #define LEAVE_TCL_WITH_BUSYWAIT(result) \ tcl_tstate = NULL; \ - if (tcl_lock)PyThread_release_lock(tcl_lock);\ + RELEASE_TCL_LOCK;\ if (result == 0)\ Sleep(Tkinter_busywaitinterval);\ Py_END_ALLOW_THREADS @@ -235,15 +261,16 @@ static PyThreadState *tcl_tstate = NULL; Py_BEGIN_ALLOW_THREADS #define LEAVE_OVERLAP_TCL \ - tcl_tstate = NULL; if(tcl_lock)PyThread_release_lock(tcl_lock); } + tcl_tstate = NULL; RELEASE_TCL_LOCK; } #define ENTER_PYTHON \ { PyThreadState *tstate = tcl_tstate; tcl_tstate = NULL; \ - if(tcl_lock)PyThread_release_lock(tcl_lock); PyEval_RestoreThread((tstate)); } + RELEASE_TCL_LOCK; PyEval_RestoreThread((tstate)); } #define LEAVE_PYTHON \ { PyThreadState *tstate = PyEval_SaveThread(); \ - if(tcl_lock)PyThread_acquire_lock(tcl_lock, 1); tcl_tstate = tstate; } + ACQUIRE_TCL_LOCK;\ + tcl_tstate = tstate; } #define ENTER_PYTHON_OVERLAP \ { PyThreadState *tstate = tcl_tstate; tcl_tstate = NULL; PyEval_RestoreThread((tstate)); } From bc95fb3e764c22a3c268be95f4d1f8c4c93295df Mon Sep 17 00:00:00 2001 From: Ivan Pozdeev Date: Sun, 29 Apr 2018 17:12:43 +0300 Subject: [PATCH 09/14] Don't release Tcl lock in Tcl custom commands --- Modules/_tkinter.c | 28 +++++++--------------------- 1 file changed, 7 insertions(+), 21 deletions(-) diff --git a/Modules/_tkinter.c b/Modules/_tkinter.c index add86faebd2b23..b85e6086ae6800 100644 --- a/Modules/_tkinter.c +++ b/Modules/_tkinter.c @@ -264,18 +264,9 @@ if (tcl_lock){\ tcl_tstate = NULL; RELEASE_TCL_LOCK; } #define ENTER_PYTHON \ - { PyThreadState *tstate = tcl_tstate; tcl_tstate = NULL; \ - RELEASE_TCL_LOCK; PyEval_RestoreThread((tstate)); } - -#define LEAVE_PYTHON \ - { PyThreadState *tstate = PyEval_SaveThread(); \ - ACQUIRE_TCL_LOCK;\ - tcl_tstate = tstate; } - -#define ENTER_PYTHON_OVERLAP \ { PyThreadState *tstate = tcl_tstate; tcl_tstate = NULL; PyEval_RestoreThread((tstate)); } -#define LEAVE_PYTHON_OVERLAP \ +#define LEAVE_PYTHON \ { PyThreadState *tstate = PyEval_SaveThread(); tcl_tstate = tstate; } @@ -297,8 +288,6 @@ if (tcl_lock){\ #define LEAVE_OVERLAP_TCL #define ENTER_PYTHON #define LEAVE_PYTHON -#define ENTER_PYTHON_OVERLAP -#define LEAVE_PYTHON_OVERLAP #define CHECK_TCL_APPARTMENT #endif @@ -1601,17 +1590,17 @@ Tkapp_CallProc(Tkapp_CallEvent *e, int flags) Tcl_Obj **objv; int objc; int i; - ENTER_PYTHON_OVERLAP + ENTER_PYTHON objv = Tkapp_CallArgs(e->args, objStore, &objc); if (!objv) { PyErr_Fetch(e->exc_type, e->exc_value, e->exc_tb); *(e->res) = NULL; } - LEAVE_PYTHON_OVERLAP + LEAVE_PYTHON if (!objv) goto done; i = Tcl_EvalObjv(e->self->interp, objc, objv, e->flags); - ENTER_PYTHON_OVERLAP + ENTER_PYTHON if (i == TCL_ERROR) { *(e->res) = NULL; *(e->exc_type) = NULL; @@ -1623,7 +1612,7 @@ Tkapp_CallProc(Tkapp_CallEvent *e, int flags) else { *(e->res) = Tkapp_CallResult(e->self); } - LEAVE_PYTHON_OVERLAP + LEAVE_PYTHON Tkapp_CallDeallocArgs(objv, objStore, objc); done: @@ -2525,8 +2514,8 @@ PythonCmd(ClientData clientData, Tcl_Interp *interp, int argc, char *argv[]) ENTER_PYTHON /* TBD: no error checking here since we know, via the - * Tkapp_CreateCommand() that the client data is a two-tuple - */ + * Tkapp_CreateCommand() that the client data is a two-tuple + */ func = data->func; /* Create argument list (argv1, ..., argvN) */ @@ -2546,14 +2535,11 @@ PythonCmd(ClientData clientData, Tcl_Interp *interp, int argc, char *argv[]) if (res == NULL) return PythonCmd_Error(); - ENTER_TCL - ENTER_OVERLAP obj_res = AsObj(res); if (obj_res) { Tcl_SetObjResult(interp, obj_res); rv = TCL_OK; } - LEAVE_OVERLAP_TCL if (obj_res == NULL) { Py_DECREF(res); return PythonCmd_Error(); From 0511128f520f75f77d7f3eb9ab2d49ee26cefbaa Mon Sep 17 00:00:00 2001 From: Ivan Pozdeev Date: Wed, 16 May 2018 08:35:01 +0300 Subject: [PATCH 10/14] Document locking changes for handlers --- Modules/_tkinter.c | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/Modules/_tkinter.c b/Modules/_tkinter.c index b85e6086ae6800..8b9120ac0b2f2e 100644 --- a/Modules/_tkinter.c +++ b/Modules/_tkinter.c @@ -173,16 +173,16 @@ Copyright (C) 1994 Steen Lumholt. Tcl lock and keeps the Python lock. By contrast, ENTER_PYTHON and LEAVE_PYTHON are used in Tcl event - handlers when the handler needs to use Python. Such event handlers are - entered while the lock for Tcl is held; the event handler presumably needs - to use Python. ENTER_PYTHON releases the lock for Tcl and acquires - the Python interpreter lock, restoring the appropriate thread state, and - LEAVE_PYTHON releases the Python interpreter lock and re-acquires the lock - for Tcl. It is okay for ENTER_TCL/LEAVE_TCL pairs to be contained inside - the code between ENTER_PYTHON and LEAVE_PYTHON. - - These locks expand to several statements and brackets; they should not be - used in branches of if statements and the like. + handlers when the handler needs to use Python. Such event handlers + are entered while the lock for Tcl is held; the event handler + presumably needs to use Python. ENTER_PYTHON acquires the Python + interpreter lock, restoring the appropriate thread state, and + LEAVE_PYTHON releases the Python interpreter lock. Tcl lock is not + released because that would lead to a race condition: another, + unrelated call that uses these macros may start (but not finish) in + the meantime, and a wrong Tcl stack frame will be on top when the original + handler regains the lock. Since a handler's Python payload may need to make + Tkinter calls itself, the Tcl lock is made reentrant. If Tcl is threaded, this approach won't work anymore. The Tcl interpreter is only valid in the thread that created it, and all Tk activity must happen in this From a15b10c289f9bbef4d90c7d24e6abdd49907ca6d Mon Sep 17 00:00:00 2001 From: Ivan Pozdeev Date: Thu, 17 May 2018 14:52:58 +0300 Subject: [PATCH 11/14] cosmetic changes for readability --- Modules/_tkinter.c | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/Modules/_tkinter.c b/Modules/_tkinter.c index 8b9120ac0b2f2e..af3a63d48e49b8 100644 --- a/Modules/_tkinter.c +++ b/Modules/_tkinter.c @@ -165,8 +165,8 @@ Copyright (C) 1994 Steen Lumholt. Sometimes, it is necessary to have both the Python lock and the Tcl lock. (For example, when transferring data between the Tcl interpreter and/or - objects and Python objects.) To avoid deadlocks, when acquiring, we always - acquire the Tcl lock first, then the Python lock. The additional macros for + objects and Python objects.) To avoid deadlocks, when acquiring, we always + acquire the Tcl lock first, then the Python lock. The additional macros for finer lock control are: ENTER_OVERLAP acquires the Python lock (and restores the thread state) when already holding the Tcl lock; LEAVE_OVERLAP releases the Python lock and keeps the Tcl lock; and LEAVE_OVERLAP_TCL releases the @@ -204,6 +204,7 @@ static PyThread_type_lock tcl_lock = 0; static unsigned long tcl_lock_thread_ident = 0; static unsigned int tcl_lock_reentry_count = 0; + #ifdef TCL_THREADS static Tcl_ThreadDataKey state_key; typedef PyThreadState *ThreadSpecificData; @@ -212,9 +213,10 @@ typedef PyThreadState *ThreadSpecificData; static PyThreadState *tcl_tstate = NULL; #endif + #define ACQUIRE_TCL_LOCK \ if (tcl_lock) {\ - if (tcl_lock_thread_ident == PyThread_get_thread_ident()) {\ + if (tcl_lock_thread_ident == PyThread_get_thread_ident()) { \ tcl_lock_reentry_count++; \ if(!tcl_lock_reentry_count) \ Py_FatalError("Tcl lock reentry count overflow"); \ @@ -234,12 +236,13 @@ if (tcl_lock){\ }\ } + #define ENTER_TCL \ - { PyThreadState *tstate = PyThreadState_Get();\ + { PyThreadState *tstate = PyThreadState_Get(); \ ENTER_TCL_CUSTOM_TSTATE(tstate) #define ENTER_TCL_CUSTOM_TSTATE(tstate) \ - Py_BEGIN_ALLOW_THREADS\ + Py_BEGIN_ALLOW_THREADS \ ACQUIRE_TCL_LOCK; tcl_tstate = tstate; #define LEAVE_TCL \ From 8f51be1e41539bf214d520e962700da10a0b62a9 Mon Sep 17 00:00:00 2001 From: Ivan Pozdeev Date: Wed, 16 May 2018 19:16:09 +0300 Subject: [PATCH 12/14] replace test case --- Lib/lib-tk/test/test_tkinter/test_threads.py | 117 ++++++++++++++ Lib/tkinter/test/test_tkinter/test_threads.py | 144 ------------------ 2 files changed, 117 insertions(+), 144 deletions(-) create mode 100644 Lib/lib-tk/test/test_tkinter/test_threads.py delete mode 100644 Lib/tkinter/test/test_tkinter/test_threads.py diff --git a/Lib/lib-tk/test/test_tkinter/test_threads.py b/Lib/lib-tk/test/test_tkinter/test_threads.py new file mode 100644 index 00000000000000..92667da2a4320f --- /dev/null +++ b/Lib/lib-tk/test/test_tkinter/test_threads.py @@ -0,0 +1,117 @@ +from __future__ import print_function +import Tkinter as tkinter +import random +import string +import sys +import threading +import time + +import unittest + + +class TestThreads(unittest.TestCase): + def setUp(self): + self.stderr = "" + + def test_threads(self): + import subprocess + p = subprocess.Popen([sys.executable, __file__, "run"], + stdout=subprocess.PIPE, + stderr=subprocess.STDOUT) + + rt = threading.Thread(target=reader_thread, args=(p.stdout, self.stderr)) + rt.start() + + try: + t = time.time() + # Test code is designed to complete in a few seconds + while time.time() - t < 10: + if p.poll() is not None: break + time.sleep(0.2) + else: + p.kill() + setattr(self, 'killed', True) + finally: + p.stdout.close() + rt.join() + rc = p.returncode + if hasattr(self, 'killed'): + self.fail("Test code hang. Stderr: " + repr(self.stderr)) + self.assertTrue(rc == 0, + "Nonzero exit status: " + str(rc) + "; stderr:" + repr(self.stderr)) + self.assertTrue(len(self.stderr) == 0, "stderr: " + repr(self.stderr)) + + +def reader_thread(stream, output): + while True: + s = stream.read() + if not s: break + output += s + + +running = True + + +class EventThread(threading.Thread): + def __init__(self, target): + threading.Thread.__init__(self) + self.target = target + + def run(self): + while running: + time.sleep(0.02) + c = random.choice(string.ascii_letters) + self.target.event_generate(c) + + +class Main(object): + def __init__(self): + self.root = tkinter.Tk() + self.root.bind('', dummy_handler) + self.threads = [] + + self.t_cleanup = threading.Thread(target=self.tf_stop) + + def go(self): + self.root.after(0, self.add_threads) + self.root.after(500, self.stop) + self.root.protocol("WM_DELETE_WINDOW", self.stop) + self.root.mainloop() + self.t_cleanup.join() + + def stop(self): + self.t_cleanup.start() + + def tf_stop(self): + global running + running = False + for t in self.threads: t.join() + self.root.after(0, self.root.destroy) + + def add_threads(self): + for _ in range(20): + t = EventThread(self.root) + self.threads.append(t) + t.start() + + +def dummy_handler(event): + pass + + +tests_gui = (TestThreads,) + +if __name__ == '__main__': + import os + + if sys.argv[1:] == ['run']: + if os.name == 'nt': + import ctypes + + # the bug causes crashes + ctypes.windll.kernel32.SetErrorMode(3) + Main().go() + else: + from test.support import run_unittest + + run_unittest(*tests_gui) diff --git a/Lib/tkinter/test/test_tkinter/test_threads.py b/Lib/tkinter/test/test_tkinter/test_threads.py deleted file mode 100644 index 2dd18fa0f2b5f4..00000000000000 --- a/Lib/tkinter/test/test_tkinter/test_threads.py +++ /dev/null @@ -1,144 +0,0 @@ -#!/usr/bin/python3 - -import sys -import time -import threading -import random -import tkinter - -import unittest -class TestThreads(unittest.TestCase): - def test_threads(self): - import subprocess - p = subprocess.Popen([sys.executable, __file__, "run"], - stdout=subprocess.PIPE, - stderr=subprocess.PIPE) - self.addCleanup(p.stdout.close) - self.addCleanup(p.stderr.close) - try: - #Test code is designed to complete in a few seconds - stdout, stderr = p.communicate(timeout=10) - except subprocess.TimeoutExpired: - p.kill() - stdout, stderr = p.communicate() - self.fail("Test code hang. Stderr: " + repr(stderr)) - rc = p.returncode - self.assertTrue(rc == 0, - "Nonzero exit status: " + str(rc) + "; stderr:" + repr(stderr)) - self.assertTrue(len(stderr) == 0, "stderr: " + repr(stderr)) - - - -running = True - -class Track(threading.Thread): - """ - Calculates coordinates for a ballistic track - with random angle and velocity - and fires the callback to draw each consecutive segment. - """ - def __init__(self, track_number, draw_callback): - """ - :param track_number: ordinal - :param draw_callback: fn(track_number, x, y) - that draws the extention of the specified track - to the specified point. The callback must keep track - of any previous coordinates itself. - """ - threading.Thread.__init__(self) - #self.setDaemon(True) - self.track_number = track_number - self.draw_callback = draw_callback - - def run(self): - #starting point for the track - y = 0.0001 #height - x = 999.0 #range - #initial velocities - yVel = 400. + random.random() * 200. - xVel = -200. - random.random() * 150. - # Stop drawing when the track hits the ground - while y > 0 and running: - #How long to sleep, in seconds, between track updates - time.sleep(0.01) #realism: >1 Fun: 0.01 - - yVel -= 9.8 #gravity, more or less - xVel *= .9998 #air resistance - - y += yVel - x += xVel - - if running: - self.draw_callback(self.track_number, x, y) - - -class App: - """ - The main app logic - """ - def __init__(self, window): - self.track_no = 0 #last index of track added - self.track_coordinates = {} #coords of track tips - self.threads = [] - - self.window = window - self.frame = tkinter.Frame(window) - self.frame.pack() - self.graph = tkinter.Canvas(self.frame) - self.graph.pack() - - self.t_cleanup = threading.Thread(target=self.tf_cleanup) - self.window.after(0, self.add_tracks, 5) - self.window.after(1000, self.t_cleanup.start) - - def add_tracks(self,depth): - if depth>0: self.window.after(5, self.add_tracks, depth-1) - tracks_to_add = 40 - start_no = self.track_no - self.track_no += tracks_to_add - for self.track_no in range(start_no, self.track_no): - #self.window.after(t, self.add_tracks) - #self.track_no += 1 #Make a new track number - #if (self.track_no > 40): - t = Track(self.track_no, self.draw_track_segment) - self.threads.append(t) - t.start() - - def tf_cleanup(self): - global running - running = False - for t in self.threads: t.join() - self.window.after(0,self.window.destroy) - - def draw_track_segment(self, track_no, x, y): - # x & y are virtual coordinates for the purpose of simulation. - #To convert them to screen coordinates, - # we scale them down so the graphs fit into the window, and move the origin. - # Y is also inverted because in canvas, the UL corner is the origin. - newsx, newsy = (250.+x/100., 150.-y/100.) - - try: - (oldsx, oldsy) = self.track_coordinates[track_no] - except KeyError: - pass - else: - self.graph.create_line(oldsx, oldsy, - newsx, newsy) - self.track_coordinates[track_no] = (newsx, newsy) - - def go(self): - self.window.mainloop() - -tests_gui = (TestThreads,) - -if __name__=='__main__': - import sys,os - if sys.argv[1:]==['run']: - if os.name == 'nt': - import ctypes - #the bug causes crashes - ctypes.windll.kernel32.SetErrorMode(3) - App(tkinter.Tk()).go() - else: - from test.support import run_unittest - run_unittest(*tests_gui) From 6323978b0a0989ec0398ac54429b15d6f5bc4542 Mon Sep 17 00:00:00 2001 From: Ivan Pozdeev Date: Thu, 17 May 2018 22:09:05 +0300 Subject: [PATCH 13/14] code style --- Modules/_tkinter.c | 37 +++++++++++++++++++------------------ 1 file changed, 19 insertions(+), 18 deletions(-) diff --git a/Modules/_tkinter.c b/Modules/_tkinter.c index af3a63d48e49b8..9764693738610c 100644 --- a/Modules/_tkinter.c +++ b/Modules/_tkinter.c @@ -215,7 +215,7 @@ static PyThreadState *tcl_tstate = NULL; #define ACQUIRE_TCL_LOCK \ -if (tcl_lock) {\ +if (tcl_lock) { \ if (tcl_lock_thread_ident == PyThread_get_thread_ident()) { \ tcl_lock_reentry_count++; \ if(!tcl_lock_reentry_count) \ @@ -223,17 +223,17 @@ if (tcl_lock) {\ } else { \ PyThread_acquire_lock(tcl_lock, 1); \ tcl_lock_thread_ident = PyThread_get_thread_ident(); \ - }\ + } \ } #define RELEASE_TCL_LOCK \ -if (tcl_lock){\ +if (tcl_lock) { \ if (tcl_lock_reentry_count) { \ tcl_lock_reentry_count--; \ } else { \ tcl_lock_thread_ident = 0; \ PyThread_release_lock(tcl_lock); \ - }\ + } \ } @@ -242,8 +242,9 @@ if (tcl_lock){\ ENTER_TCL_CUSTOM_TSTATE(tstate) #define ENTER_TCL_CUSTOM_TSTATE(tstate) \ - Py_BEGIN_ALLOW_THREADS \ - ACQUIRE_TCL_LOCK; tcl_tstate = tstate; + Py_BEGIN_ALLOW_THREADS \ + ACQUIRE_TCL_LOCK \ + tcl_tstate = tstate; #define LEAVE_TCL \ tcl_tstate = NULL; \ @@ -251,9 +252,9 @@ if (tcl_lock){\ #define LEAVE_TCL_WITH_BUSYWAIT(result) \ tcl_tstate = NULL; \ - RELEASE_TCL_LOCK;\ - if (result == 0)\ - Sleep(Tkinter_busywaitinterval);\ + RELEASE_TCL_LOCK \ + if (result == 0) \ + Sleep(Tkinter_busywaitinterval); \ Py_END_ALLOW_THREADS @@ -264,7 +265,7 @@ if (tcl_lock){\ Py_BEGIN_ALLOW_THREADS #define LEAVE_OVERLAP_TCL \ - tcl_tstate = NULL; RELEASE_TCL_LOCK; } + tcl_tstate = NULL; RELEASE_TCL_LOCK } #define ENTER_PYTHON \ { PyThreadState *tstate = tcl_tstate; tcl_tstate = NULL; PyEval_RestoreThread((tstate)); } @@ -2015,7 +2016,7 @@ SetVar(PyObject *self, PyObject *args, int flags) if (newval) { LEAVE_OVERLAP ok = Tcl_SetVar2Ex(Tkapp_Interp(self), name1, NULL, - newval, flags); + newval, flags); ENTER_OVERLAP if (!ok) Tkinter_Error(self); @@ -2038,7 +2039,7 @@ SetVar(PyObject *self, PyObject *args, int flags) if (newval) { LEAVE_OVERLAP ok = Tcl_SetVar2Ex(Tkapp_Interp(self), name1, name2, - newval, flags); + newval, flags); ENTER_OVERLAP if (!ok) Tkinter_Error(self); @@ -2517,8 +2518,8 @@ PythonCmd(ClientData clientData, Tcl_Interp *interp, int argc, char *argv[]) ENTER_PYTHON /* TBD: no error checking here since we know, via the - * Tkapp_CreateCommand() that the client data is a two-tuple - */ + * Tkapp_CreateCommand() that the client data is a two-tuple + */ func = data->func; /* Create argument list (argv1, ..., argvN) */ @@ -2539,14 +2540,14 @@ PythonCmd(ClientData clientData, Tcl_Interp *interp, int argc, char *argv[]) return PythonCmd_Error(); obj_res = AsObj(res); - if (obj_res) { - Tcl_SetObjResult(interp, obj_res); - rv = TCL_OK; - } if (obj_res == NULL) { Py_DECREF(res); return PythonCmd_Error(); } + else { + Tcl_SetObjResult(interp, obj_res); + rv = TCL_OK; + } Py_DECREF(res); From fa06251551dc188aee246a92863d834e0c000265 Mon Sep 17 00:00:00 2001 From: Ivan Pozdeev Date: Fri, 18 May 2018 05:09:07 +0300 Subject: [PATCH 14/14] alter NEWS to include the 2nd fix --- .../next/Library/2018-04-14-01-26-10.bpo-33257.i32qOW.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Misc/NEWS.d/next/Library/2018-04-14-01-26-10.bpo-33257.i32qOW.rst b/Misc/NEWS.d/next/Library/2018-04-14-01-26-10.bpo-33257.i32qOW.rst index d8611ec82b046f..b061786ae808af 100644 --- a/Misc/NEWS.d/next/Library/2018-04-14-01-26-10.bpo-33257.i32qOW.rst +++ b/Misc/NEWS.d/next/Library/2018-04-14-01-26-10.bpo-33257.i32qOW.rst @@ -1 +1 @@ -Fixed race conditions in Tkinter when passing data to/from nonthreaded Tcl +Fixed race conditions in Tkinter with nonthreaded Tcl