bczsalba / pytermgui

Python TUI framework with mouse support, modular widget system, customizable and rapid terminal markup language and more!
https://ptg.bczsalba.com
MIT License
2.21k stars 54 forks source link

[BUG] Ctrl-C doesn't stop threads #78

Closed Tired-Fox closed 2 years ago

Tired-Fox commented 2 years ago

Describe the bug When running any graphics loop, using the compositor, and you execute a keyboard interrupt the thread that draws the window keeps going. Mainly found on windows 10.

I tested this on Windows 11 and the issue doesn't exist.

To Reproduce

  1. Use a Windows 10 machine
  2. Use Windows Terminal or Alacritty
  3. Run any of the examples
  4. Press CTRL-C
  5. Program exits, but thread keeps drawing screen which traps the terminal in the window

Expected behavior Program exits cleanly back to terminal prompt

Screenshots LockedInDrawWindow The above gif shows a flash of text from the interrupt then another draw call.

LockedInTerminalInterrupt The above image shows the successful interrupt but the terminal is trapped because of a thread still being active.

System information

PyTermGUI version 6.4.0

System details:
    Python version: 3.10.5
    $TERM:          None
    $COLORTERM:     None
    Color support:  ColorSystem.STANDARD
    OS Platform:    Windows-10-10.0.19043-SP0

Possible cause On Windows 10 a keyboard interrupt does not stop child threads and only stops the main thread.

Possible solution Set thread to be a daemon. When a thread is set to be a daemon it will terminate when all non-daemon threads terminate. This means when the main thread terminates so will the child thread.

Add daemon=True to where the compositor draw thread is created

165        Thread(name="CompositorDrawLoop", target=self._draw_loop, daemon=True).start()

I have tested this on the machine that had this bug and it fixed the problem.

NOTE: I would like to add that making the different threads daemons may solve some other issues as well.

bczsalba commented 2 years ago

Will look into this, thank you!

I would like to add that making the different threads daemons may solve some other issues as well.

Do you have any specific things in mind for this?

Tired-Fox commented 2 years ago

Didn't read to deep, but it could help with issue #18.

Also thought with the ticket about SIGWINCH not working for windows (#33), there could be a daemon thread created that sends a signal when os.get_terminal_size() changes it's value. I have already started thinking about other ways of solving that ticket as well.

bczsalba commented 2 years ago

Look out for https://github.com/bczsalba/pytermgui/issues/25, AFAICR we also tried a thread-based implementation that caused the same issue. Would be grateful if you could help out!

Tired-Fox commented 2 years ago

I duel boot windows and linux, so I don't mind going into the windows side and helping.

I may look into writing some Windows API hooks, let me know if you are interested in bringing this into the project.

bczsalba commented 2 years ago

I actually have been for a long time! Specifically this API would be greatly beneficial to the library, for seemingly very little cost.

Tired-Fox commented 2 years ago

I saw that you linked that in #33. I wouldn't mind writing something that can translate between linux and windows calls to make it universal. I can start a discussion as to keep this issue about the problem listed above.

bczsalba commented 2 years ago

@Tired-Fox could you test out if daemon=True fixes the issue on Windows? It seems to not have any adverse effect on MacOS, so it should be fine on Linux too.

Tired-Fox commented 2 years ago

I can test both Linux and Windows

Tired-Fox commented 2 years ago

@bczsalba It seems that it has no adverse effects for Linux. It also works on windows with no obvious side affects.

bczsalba commented 2 years ago

Can you test if this fixed the issue? :smile:

P.S. Thank you for the testing!