From a874235dff32bd77125034cfd9542a91b68efb03 Mon Sep 17 00:00:00 2001 From: K900 Date: Tue, 19 Oct 2021 15:42:27 +0300 Subject: [PATCH 1/2] nixos/lib/test-driver: clean up threads correctly The current implementation just forks off a thread to read QEMU's stdout and lets it exist forever. This, however, makes the interpreter shutdown racy, as the thread could still be running and writing out buffered stdout when the main thread exits (and since it's using the low level API, the worker thread does not get cleaned up by the atexit hooks installed by `threading`, either). So, instead of doing that, let's create a real `threading.Thread` object, and also explicitly `join` it along with the other stuff when cleaning up. --- nixos/lib/test-driver/test-driver.py | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/nixos/lib/test-driver/test-driver.py b/nixos/lib/test-driver/test-driver.py index e659b0c04f50..f2e7bf3c1d5b 100755 --- a/nixos/lib/test-driver/test-driver.py +++ b/nixos/lib/test-driver/test-driver.py @@ -6,7 +6,7 @@ from xml.sax.saxutils import XMLGenerator from colorama import Style import queue import io -import _thread +import threading import argparse import atexit import base64 @@ -409,6 +409,7 @@ class Machine: pid: Optional[int] = None monitor: Optional[socket.socket] = None shell: Optional[socket.socket] = None + serial_thread: Optional[threading.Thread] booted: bool = False connected: bool = False @@ -444,6 +445,8 @@ class Machine: self.cleanup_statedir() self.state_dir.mkdir(mode=0o700, exist_ok=True) + self.serial_thread = None + @staticmethod def create_startcommand(args: Dict[str, str]) -> StartCommand: rootlog.warning( @@ -921,7 +924,8 @@ class Machine: self.last_lines.put(line) self.log_serial(line) - _thread.start_new_thread(process_serial_output, ()) + self.serial_thread = threading.Thread(target=process_serial_output) + self.serial_thread.start() self.wait_for_monitor_prompt() @@ -1021,9 +1025,12 @@ class Machine: assert self.process assert self.shell assert self.monitor + assert self.serial_thread + self.process.terminate() self.shell.close() self.monitor.close() + self.serial_thread.join() class VLan: From 9611041ef54b09eb764ed72d95c3111ce52fe984 Mon Sep 17 00:00:00 2001 From: K900 Date: Tue, 19 Oct 2021 16:29:19 +0300 Subject: [PATCH 2/2] nixos/lib/test-driver: set defaults in __init__ instead of class level Just a minor code style nit to avoid potential footguns --- nixos/lib/test-driver/test-driver.py | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/nixos/lib/test-driver/test-driver.py b/nixos/lib/test-driver/test-driver.py index f2e7bf3c1d5b..3ee8b3227c61 100755 --- a/nixos/lib/test-driver/test-driver.py +++ b/nixos/lib/test-driver/test-driver.py @@ -405,14 +405,14 @@ class Machine: keep_vm_state: bool allow_reboot: bool - process: Optional[subprocess.Popen] = None - pid: Optional[int] = None - monitor: Optional[socket.socket] = None - shell: Optional[socket.socket] = None + process: Optional[subprocess.Popen] + pid: Optional[int] + monitor: Optional[socket.socket] + shell: Optional[socket.socket] serial_thread: Optional[threading.Thread] - booted: bool = False - connected: bool = False + booted: bool + connected: bool # Store last serial console lines for use # of wait_for_console_text last_lines: Queue = Queue() @@ -445,8 +445,15 @@ class Machine: self.cleanup_statedir() self.state_dir.mkdir(mode=0o700, exist_ok=True) + self.process = None + self.pid = None + self.monitor = None + self.shell = None self.serial_thread = None + self.booted = False + self.connected = False + @staticmethod def create_startcommand(args: Dict[str, str]) -> StartCommand: rootlog.warning(