LinuxDriver: Exit if thread dies #3431

rndusr · 2023-09-29T13:58:00Z

If run_input_thread() dies, the whole application keeps running but becomes
unresponsive. It has to be killed by the user, leaving the terminal in an
unusable state.

This commit terminates the application instead and prints a traceback.

If `run_input_thread()` dies, the whole application keeps running but becomes unresponsive. It has to be killed by the user, leaving the terminal in an unusable state. This commit terminates the application instead and prints a traceback.

willmcgugan · 2023-09-29T13:59:22Z

And what exception were you getting?

rndusr · 2023-09-29T14:10:45Z

Sorry, I should've explained my motivation better.

I'm trying to add support for Alt keybindings (#3327). Every time I change the
code, run textual keys and it turns out I made mistake, I have to manually
kill textual keys in another terminal, and run reset to get the terminal
back into a defined state.

This commit fixes my personal situation, but I made a PR because I think it is
generally a good idea to escalate if a vital thread dies.

willmcgugan · 2023-10-03T15:38:31Z

src/textual/drivers/linux_driver.py

-            log(error)
+            self._app.exit(
+                return_code=1,
+                message=rich.traceback.Traceback.from_exception(


I think it makes more sense to call app.panic here. But bear in mind that neither exit or panic are threadsafe. It's probably wise to use app.call_later to ensure that panic is called in the main thread.

BTW I don't think you'll need all those parameters to Traceback. If you construct Traceback() in a handler it will detect the traceback automatically.

willmcgugan · 2023-10-03T15:39:21Z

This commit fixes my personal situation, but I made a PR because I think it is generally a good idea to escalate if a vital thread dies.

Got it!

rndusr · 2023-10-06T09:44:17Z

I think it's better use an exception handler. I think this is the better approach because any exception from run_input_thread() is now properly handled and run_input_thread() doesn't have to catch it and deal with application shutdown at all. What do you think?

I've used self._app.exit() because self._app.panic() exits with 0, indicating success. But that's probably a different issue with App.panic(). I'm calling self._app.panic() now.

I'm not sure which docstring style is used and how to reference arguments, methods, etc. I couldn't find anything in CONTRIBUTING.

willmcgugan · 2023-10-09T10:15:55Z

src/textual/drivers/linux_driver.py

@@ -23,6 +24,25 @@
    from ..app import App


+class ExceptionHandlingThread(Thread):


Extending Thread doesn't seem warranted here. Could we not wrap self.run_input_thread with a try except?

That's what ExceptionHandlingThread is doing. You can't catch the exception normally because it is called by Thread.start().

Or do you mean we could wrap the body of run_input_thread() in try ... except? That would work, but it makes run_input_thread() more complex than it needs to be and do things (terminating the application) it shouldn't care about. It also means that, if anyone ever would add code above the try ... except, any exception raised by that new code would be ignored. I think separating the functionality of the input thread and the termination of the application if that input thread raises makes maintenance much easier.

We could also pass a new method as the Thread target (e.g. _handle_run_input_thread()) that only catches exceptions from run_input_thread() and terminates the application in that case. That would make the separation of functionality more clear without an additional class.

We could also set threading.exceptionhook to a function that terminates the application. But that would affect all threads in the entire application. I don't think that's a good idea because developers might want to set that themselves. Breaking textual by configuring exception in your application would a pretty annoying bug. We would also have to bump the minimum Python version to 3.8.

The benefit of ExceptionHandlingThread is that it could be used by all textual threads to always terminate gracefully without affecting application developers' exception handling. I didn't look into using ExceptionHandlingThread for other threads because textual's code base is a bit daunting and I'm not sure what I should consider.

If you really don't like the new class, I'd recommend the _handle_run_input_thread() wrapper method. The other solutions wouldn't really solve the issue for good.

I ws proposing a wrapper. I would just call it _run_input_thread.

…read()

rndusr · 2023-10-10T17:04:21Z

OK, I've added the _run_input_thread() method. I don't understand why, but exceptions raised in the finally: block in run_input_thread() are still ignored. Let me know if I should squash my commits.

willmcgugan · 2023-10-11T09:24:38Z

src/textual/drivers/linux_driver.py

@@ -9,9 +9,10 @@
 import tty
 from codecs import getincrementaldecoder
 from threading import Event, Thread
-from typing import TYPE_CHECKING, Any
+from typing import TYPE_CHECKING, Any, Callable


Do we need Callable ?

rndusr · 2023-10-11T09:34:19Z

No. Nice catch. Fixed it. Looks like I've become dependent on my linting tools. Is there a reason you're not using something like ruff/isort/flake8?

willmcgugan · 2023-10-11T09:36:34Z

We generally rely on the IDE to catch those things.

rndusr · 2023-10-11T10:04:50Z

Sorry for the OT, but relying on the IDE doesn't seem to work very well: ``` $ flake8 --select=F4 src src/textual/_parser.py:168:5: F401 'time' imported but unused src/textual/_tree_sitter.py:4:5: F401 'tree_sitter.Language' imported but unused src/textual/_tree_sitter.py:4:5: F401 'tree_sitter.Parser' imported but unused src/textual/_tree_sitter.py:4:5: F401 'tree_sitter.Tree' imported but unused src/textual/_tree_sitter.py:5:5: F401 'tree_sitter.binding.Query' imported but unused src/textual/_tree_sitter.py:6:5: F401 'tree_sitter_languages.get_language' imported but unused src/textual/_tree_sitter.py:6:5: F401 'tree_sitter_languages.get_parser' imported but unused src/textual/_types.py:3:1: F401 'typing_extensions.Literal' imported but unused src/textual/_types.py:3:1: F401 'typing_extensions.SupportsIndex' imported but unused src/textual/_types.py:3:1: F401 'typing_extensions.get_args' imported but unused src/textual/_types.py:3:1: F401 'typing_extensions.runtime_checkable' imported but unused src/textual/app.py:103:5: F401 'textual_dev.client.DevtoolsClient' imported but unused src/textual/css/_style_properties.py:50:5: F401 '.styles.Styles' imported but unused src/textual/drivers/_input_reader_linux.py:7:1: F401 'textual.log' imported but unused src/textual/drivers/linux_driver.py:17:1: F401 '..log' imported but unused src/textual/drivers/linux_driver.py:260:22: F402 import 'events' from line 17 shadowed by loop variable src/textual/message_pump.py:29:1: F401 '._types.CallbackType' imported but unused src/textual/widget.py:11:1: F401 'operator.attrgetter' imported but unused ```

willmcgugan · 2023-10-11T10:17:42Z

Most of those are erroneous. The ones that aren't don't impact the project at all. There is a place for linters, but this rule just creates busy work with no return.

LinuxDriver: Exit if thread dies

a0b7516

If `run_input_thread()` dies, the whole application keeps running but becomes unresponsive. It has to be killed by the user, leaving the terminal in an unusable state. This commit terminates the application instead and prints a traceback.

willmcgugan requested changes Oct 3, 2023

View reviewed changes

LinuxDriver: Handle any exception from input thread

054b9f0

LinuxDriver: Simpler traceback construction in input thread handler

646dcad

willmcgugan requested changes Oct 9, 2023

View reviewed changes

LinuxDriver: Catch exception from run_input_thread() in _run_input_th…

dda3bb3

…read()

willmcgugan reviewed Oct 11, 2023

View reviewed changes

LinuxDriver: Remove unneeded import: Callable

afcd5de

willmcgugan approved these changes Oct 11, 2023

View reviewed changes

willmcgugan merged commit 08b49b1 into Textualize:main Oct 11, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

LinuxDriver: Exit if thread dies #3431

LinuxDriver: Exit if thread dies #3431

rndusr commented Sep 29, 2023

willmcgugan commented Sep 29, 2023

rndusr commented Sep 29, 2023

willmcgugan Oct 3, 2023

willmcgugan commented Oct 3, 2023

rndusr commented Oct 6, 2023

willmcgugan Oct 9, 2023

rndusr Oct 10, 2023

willmcgugan Oct 10, 2023

rndusr commented Oct 10, 2023 via email

willmcgugan Oct 11, 2023

rndusr commented Oct 11, 2023 via email

willmcgugan commented Oct 11, 2023

rndusr commented Oct 11, 2023 via email

willmcgugan commented Oct 11, 2023

		@@ -23,6 +24,25 @@
		from ..app import App


		class ExceptionHandlingThread(Thread):

LinuxDriver: Exit if thread dies #3431

LinuxDriver: Exit if thread dies #3431

Conversation

rndusr commented Sep 29, 2023

willmcgugan commented Sep 29, 2023

rndusr commented Sep 29, 2023

willmcgugan Oct 3, 2023

Choose a reason for hiding this comment

willmcgugan commented Oct 3, 2023

rndusr commented Oct 6, 2023

willmcgugan Oct 9, 2023

Choose a reason for hiding this comment

rndusr Oct 10, 2023

Choose a reason for hiding this comment

willmcgugan Oct 10, 2023

Choose a reason for hiding this comment

rndusr commented Oct 10, 2023 via email

willmcgugan Oct 11, 2023

Choose a reason for hiding this comment

rndusr commented Oct 11, 2023 via email

willmcgugan commented Oct 11, 2023

rndusr commented Oct 11, 2023 via email

willmcgugan commented Oct 11, 2023