1

I'm using threading to run a long task, but I ran into an issue. The thread just hung while trying to set an IntVar after I clicked the close button. It doesn't even error. I don't want to use a daemon thread because the function is a critical part of the program, which might have consequences if it stops midway through (it deals with a bunch of files).

Here's an oversimplified version of my program, meant to demonstrate my issue.

import tkinter as tk
import tkinter.ttk as ttk
import threading

class Window(tk.Tk):
    def __init__(this, *args, **kwargs) -> None:
        super().__init__(*args, **kwargs)
        
        this.threads = []
        this.var = tk.IntVar(value=0)
        
        this.label = ttk.Label(textvariable=this.var)
        this.button = ttk.Button(text='Start counter', command=this.startCounter)
        
        this.label.pack()
        this.button.pack()
        
        this.stop = False
        
        this.protocol("WM_DELETE_WINDOW", this.close)
        
    def startCounter(this):
        thread = threading.Thread(target=this.counter)
        this.threads.append(thread)
        thread.start()
        
    def counter(this):
        while True:
            if this.stop:
                print(f'self.stop = ')
                break
            this.var.set(this.var.get() + 1)
        
    def close(this):
        print('Stopping threads')
        this.stop = True
        
        this.waitThreads()
        
        print('Stopped threads')
        this.destroy()
    
    def waitThreads(this):
        for thread in this.threads:
            thread.join()

Window().mainloop()

My program is using an InVar for a progress bar, not a counter, this was just the best way I could demonstrate the issue.

I tried a bunch of different methods to stop all threads, but none of them worked (that was before I knew what the issue was). For some reason in my actual program, if I log the var and the value of the var before the stop check, it's able to stop. I could not reproduce that with my test script.

I'm expecting the set var line to move on, or error instead of just hang.

Why is it hanging, and how can I fix this? I want to be able to safely stop the thread(s), and I don't want to use a daemon.

  • what about capturing the event user closes de window, so before destroying your program `this.close` method is called? See https://stackoverflow.com/questions/111155/how-do-i-handle-the-window-close-event-in-tkinter – mamg2909 Feb 05 '23 at 10:09

2 Answers2

0

you have a race condition, a deadlock, and an undefined behavior in your application ... that's how simple it is to mess up a small code snippet when multithreading.

the tk interpreter isn't threadsafe, and shouldn't be called from different threads, notably the event_generate function is threadsafe and should be used for instructing GUI changes, incrementing the variable from another thread is likely going to crash the interpreter, it's also a race condition, and the results will be wrong, increments should only happen in the main thread, by generating an event from the other thread.

lastly you need to make your threads drop the GIL momentarily, this can be done by a small sleep time.sleep(0.0000001).

import tkinter as tk
import tkinter.ttk as ttk
import threading
import time

class Window(tk.Tk):
    def __init__(this, *args, **kwargs) -> None:
        super().__init__(*args, **kwargs)

        this.threads = []
        this.var = tk.IntVar(value=0)

        this.label = ttk.Label(textvariable=this.var)
        this.button = ttk.Button(text='Start counter', command=this.startCounter)
        this.bind("<<increment_counter>>",this.increment_var)
        this.label.pack()
        this.button.pack()

        this.stop = False

        this.protocol("WM_DELETE_WINDOW", this.close)

    def startCounter(this):
        thread = threading.Thread(target=this.counter)
        this.threads.append(thread)
        thread.start()

    def increment_var(this, event):
        this.var.set(this.var.get() + 1)

    def counter(this):
        while True:
            time.sleep(0.0000001)  # drop the GIL momentarily
            if this.stop:
                print(f'self.stop = ')
                break
            this.event_generate("<<increment_counter>>")  # all increments happen in main thread

    def close(this):
        print('Stopping threads')
        this.stop = True

        this.waitThreads()

        print('Stopped threads')
        this.destroy()

    def waitThreads(this):
        for thread in this.threads:
            thread.join()

Window().mainloop()

note that the first argument of a method is by convention called self in python, calling it this will confuse a lot of linters, parsers, other coders, autocorrect IDEs and documentation generators. please don't do that to everyone and use self instead of this.

Ahmed AEK
  • 8,584
  • 2
  • 7
  • 23
  • I wouldn't agree with the statement --*only the event_generate function is threadsafe*--, cause you can do different things but you have to take care of things. However, `event_generate` is the easiest way to not mess up. In addition `waitThreads` will probably still lead to the described behavior in the question. While this might be an improvement to the code, it does not necessarily solve the issue. Also I'm not really sure where you do solve any `race condition` if the term applies here anyway. – Thingamabobs Feb 05 '23 at 11:33
  • @Thingamabobs the increment happen only in the main thread, which guarantees no race condition will happen on the increment, the deadlock is slightly more involved, as it involves tkinter not releasing the gil ... the above code seems to reliably fix it, from testing, unless the problem is elsewhere ? i also editted the wording to prevent confusion on the use of events. – Ahmed AEK Feb 05 '23 at 12:54
  • You seem to assume the outsourced task is as short timed as a print statement, I'd doubt that, read and write operation on files can take some time, depending on what OP is in fact doing. Without testing you code, I would assume that a long operating task will still *hang* the window, till the threads join. Seems intuitive for me. – Thingamabobs Feb 05 '23 at 12:54
  • @Thingamabobs the main problem here is that the thread is not terminating, so the GUI gets stuck indefinitely in deadlock, yes, in my edit the GUI will hang until the thread terinates, but it is not stuck in a deadlock, removing the wait will be enough to turn it into your expected behavior, but without my edit this won't work. – Ahmed AEK Feb 05 '23 at 12:59
  • That's why I wrote "While this might be an improvement to the code, t does not necessarily solve the issue.". At least if I do interpret the question, especially the title, in the right way. – Thingamabobs Feb 05 '23 at 13:02
0

I have a few suggestions to make your code work safer.

  1. Use a costume defined event. This will place an event in the event queue of tkinters event-loop.
  2. Have a thread limit, too many threads might mess things up an become unreachable.
  3. Use a threading primitive to signal the termination of your threads, like threading.Event()
  4. Instead of joining the threads within the event-loop of tkinter, make sure they are done after your app has been terminated. This will lead to a better user-experience.

In addition I would propose:

  • Using self instead of this because of convention
  • Using a set() instead of a list()

import tkinter as tk
import tkinter.ttk as ttk
import threading
import time

class Window(tk.Tk):
    def __init__(self, *args, **kwargs) -> None:
        super().__init__(*args, **kwargs)
        
        self.threads = set()
        self.var = tk.IntVar(value=0)
        
        self.label = ttk.Label(self, textvariable=self.var)
        self.button = ttk.Button(
            self, text='Start counter', command=self.startCounter)
        
        self.label.pack()
        self.button.pack()
        
        self.stop = threading.Event()
        self.event_generate('<<UpdateLabel>>')
        self.bind('<<UpdateLabel>>', self.update_counter)
        self.protocol("WM_DELETE_WINDOW", self.close)

    def update_counter(self, event):
        self.var.set(self.var.get() + 1)
        
    def startCounter(self):
        if threading.active_count()+1 <= THREAD_LIMIT:
            t = threading.Thread(target=self.counter)
            self.threads.add(t)
            t.start()
        else:
            print('too many threads might messing things up')
        
    def counter(self):
        current = threading.current_thread()
        while not self.stop.is_set():
            time.sleep(1) #simulate task
            if not self.stop.is_set():
                #if application still exists
                self.event_generate('<<UpdateLabel>>')
            else:
                self.threads.discard(current)
                break
        self.threads.discard(current)
        
    def close(self):
        print('Stopping threads')
        self.stop.set()
        print('Stop event is set')
        self.destroy()

if __name__ == '__main__':
    COUNT = threading.active_count()
    THREAD_LIMIT = COUNT + 7
    window = Window()
    window.mainloop()
    print('mainloop ended')
    while threading.active_count() > COUNT: #simulate join
        time.sleep(0.1)
        print(threading.active_count())
    print('all threads ended, mainthread ends now')
Thingamabobs
  • 7,274
  • 5
  • 21
  • 54
  • How would I do the same thing with a for loop instead of a while loop? – ego-lay atman-bay Feb 06 '23 at 17:06
  • @ego-layatman-bay if you would consider to show a sign of appreciation for the time I invest in helping you by voting or even accepting the answer if it's the solution and explanation that fulfills your need. I'll take the time to make another example with a for loop. – Thingamabobs Feb 06 '23 at 17:52
  • I appreciate your work, I just haven't fully tested it, and I haven't decided if it's the solution yet (many "solutions" I find on stack overflow don't always work with my programs). – ego-lay atman-bay Feb 06 '23 at 18:07
  • Also, I ran into an issue that it would just get stuck on `self.event_generate()` anyway, even when using the `Event()` for stopping the thread, and your snippet to wait for the threads after the window closes. – ego-lay atman-bay Feb 06 '23 at 18:42
  • @ego-layatman-bay What does "stuck" mean? Does the example code not work for you? You might have done a mistake by implementing the logic in your project. Also what is missing besides a for loop that can fairly easy implemented with that code – Thingamabobs Feb 06 '23 at 19:32
  • ok, so here's my actual program, where it's just hanging. https://github.com/ego-lay-atman-bay/mkt-explorer/blob/threading/src/main.py#L401 The might also give some context as to what I'm dealing with. (you can't really test it, unless you have the resources it's reading) – ego-lay atman-bay Feb 06 '23 at 20:01
  • after some more testing, it seems to just be updating a label that is making the program hang, updating a progress bar is just fine. – ego-lay atman-bay Feb 07 '23 at 03:55