Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

LinuxDriver: Exit if thread dies #3431

Merged
merged 5 commits into from
Oct 11, 2023
Merged

LinuxDriver: Exit if thread dies #3431

Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
a0b7516
LinuxDriver: Exit if thread dies
Sep 29, 2023
054b9f0
LinuxDriver: Handle any exception from input thread
Oct 6, 2023
646dcad
LinuxDriver: Simpler traceback construction in input thread handler
Oct 6, 2023
dda3bb3
LinuxDriver: Catch exception from run_input_thread() in _run_input_th…
Oct 10, 2023
afcd5de
LinuxDriver: Remove unneeded import: Callable
Oct 11, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 31 additions & 4 deletions src/textual/drivers/linux_driver.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need Callable ?


import rich.repr
import rich.traceback

from .. import events, log
from .._xterm_parser import XTermParser
Expand All @@ -23,6 +24,25 @@
from ..app import App


class ExceptionHandlingThread(Thread):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

"""
Thread that passes any exception from `target` to `exception_handler`

WARNING: `exception_handler` is called inside the thread. You can
schedule calls via `App.call_later`.
"""

def __init__(self, *args, exception_handler, **kwargs) -> None:
super().__init__(*args, **kwargs)
self._exception_handler: Callable = exception_handler

def run(self) -> None:
try:
super().run()
except BaseException as error:
self._exception_handler(error)


@rich.repr.auto(angular=True)
class LinuxDriver(Driver):
"""Powers display and input for Linux / MacOS"""
Expand Down Expand Up @@ -164,7 +184,10 @@ def on_terminal_resize(signum, stack) -> None:
self.write("\x1b[?25l") # Hide cursor
self.write("\033[?1003h\n")
self.flush()
self._key_thread = Thread(target=self.run_input_thread)
self._key_thread = ExceptionHandlingThread(
target=self.run_input_thread,
exception_handler=self._handle_input_thread_exception,
)
send_size_event()
self._key_thread.start()
self._request_terminal_sync_mode_support()
Expand Down Expand Up @@ -264,7 +287,11 @@ def more_data() -> bool:
unicode_data = decode(read(fileno, 1024))
for event in feed(unicode_data):
self.process_event(event)
except Exception as error:
log(error)
finally:
selector.close()

def _handle_input_thread_exception(self, error):
self._app.call_later(
self._app.panic,
rich.traceback.Traceback(),
)