From 0abc8536f079b53b4e0f996d17a3b89a0723ad17 Mon Sep 17 00:00:00 2001 From: Timothy Allen Date: Sat, 10 Oct 2026 10:59:34 +0200 Subject: [PATCH] Answer an invalid request with an error reply An unknown command ended the server loop, so one bad request stopped the service for every client. The server now sends {"status": "ERROR", "message": ...} and continues. A request with no command, or one that is not a JSON object, gets the same reply. The server tests stopped the server with an unknown command. They now stop it through accept(). --- bmspy/server.py | 18 ++++++++---- tests/test_server.py | 69 +++++++++++++++++++++++++++----------------- 2 files changed, 56 insertions(+), 31 deletions(-) diff --git a/bmspy/server.py b/bmspy/server.py index da276d2..42ca4ff 100755 --- a/bmspy/server.py +++ b/bmspy/server.py @@ -409,9 +409,12 @@ def main(): print("socket ERROR: {}".format(e)) continue - client = request_data["client"] or "unknown" + if not isinstance(request_data, dict): + request_data = {} + client = request_data.get("client") or "unknown" + command = request_data.get("command") - match request_data["command"]: + match command: case "REGISTER": connected_clients.append(client) send_response( @@ -463,10 +466,15 @@ def main(): send_response(connection, result, client, debug) case _: - debugger( - "socket: invalid request from {}".format(request_data["client"]) + # Answer and continue: one bad request must not end the + # server for every other client. + debugger("socket: invalid request from {}".format(client)) + send_response( + connection, + {"status": "ERROR", "message": "unknown command: {!r}".format(command)}, + client, + debug, ) - break except KeyboardInterrupt: if connection: diff --git a/tests/test_server.py b/tests/test_server.py index cd4efc8..421be6f 100644 --- a/tests/test_server.py +++ b/tests/test_server.py @@ -6,6 +6,8 @@ import subprocess import sys import threading +from unittest.mock import patch + import pytest import serial @@ -317,6 +319,22 @@ class TestReadRequestErrors: class TestServerMain: """Test the server main() function by running it in a thread and sending real socket commands.""" + @pytest.fixture(autouse=True) + def _stoppable_accept(self): + """Let a test end main(). The server itself has no command that stops it.""" + self._stopping = stopping = threading.Event() + original_accept = socket_module.socket.accept + + def accept_or_stop(sock): + connection, address = original_accept(sock) + if stopping.is_set(): + connection.close() + raise KeyboardInterrupt + return connection, address + + with patch.object(socket_module.socket, "accept", accept_or_stop): + yield + def _make_server_thread(self, sock_path: str, ready_event: threading.Event, stop_event: threading.Event, **kwargs): """Run server main() in a thread with mocked serial and collect_data.""" @@ -392,22 +410,25 @@ class TestServerMain: return json.loads(resp_data) def _stop_server(self, sock_path: str, thread: threading.Thread) -> None: - """End main(), so that its thread and its patches do not outlive the test. + """End main(), so that its threads and its patches do not outlive the test. - An invalid command breaks the server loop. No response follows. + One more connection wakes accept(), which then stops the server. """ + self._stopping.set() if thread.is_alive(): sock = socket_module.socket(socket_module.AF_UNIX, socket_module.SOCK_STREAM) try: sock.connect(sock_path) - payload = json.dumps({"command": "STOP", "client": "test"}).encode() - sock.sendall(struct.pack("!I", len(payload)) + payload) except OSError: pass # The server ended between the check and the connection. finally: sock.close() thread.join(timeout=5) assert not thread.is_alive() + for client_thread in threading.enumerate(): + if client_thread.name == "client": + client_thread.join(timeout=5) + assert not client_thread.is_alive() def test_register_command(self, tmp_path): """Test REGISTER command via server main().""" @@ -958,35 +979,31 @@ class TestServerMain: pass self._stop_server(sock_path, t) - def test_invalid_command_breaks_loop(self, tmp_path, capsys): - """Test that an invalid command logs an error.""" + @pytest.mark.parametrize("request_data,command", [ + ({"command": "INVALID", "client": "test"}, "'INVALID'"), + ({"client": "test"}, "None"), + (["GET"], "None"), + ], ids=["unknown-command", "no-command", "not-an-object"]) + def test_invalid_request_gets_an_error_and_the_server_continues( + self, tmp_path, capsys, request_data, command, + ): + """One bad request must not end the server for every other client.""" sock_path = str(tmp_path / "server_invalid.sock") ready = threading.Event() - stop = threading.Event() - - t = self._make_server_thread(sock_path, ready, stop) + t = self._make_server_thread(sock_path, ready, threading.Event()) t.start() ready.wait(timeout=2) - import time as _time - _time.sleep(0.2) - try: - # Send invalid command - note: server breaks on invalid, so this may fail - import socket as _s, struct, json - sock = _s.socket(_s.AF_UNIX, _s.SOCK_STREAM) - for _ in range(20): - try: - sock.connect(sock_path) - break - except (OSError, ConnectionRefusedError): - _time.sleep(0.05) - payload = json.dumps({"command": "INVALID", "client": "test"}).encode() - sock.sendall(struct.pack("!I", len(payload)) + payload) - sock.close() - except Exception: - pass + assert self._send_command(sock_path, request_data) == { + "status": "ERROR", "message": "unknown command: " + command, + } + assert t.is_alive() + # The next client still gets its answer. + response = self._send_command(sock_path, {"command": "GET", "client": "test"}) + assert "ttyUSB0" in response finally: self._stop_server(sock_path, t) + assert "invalid request from" in capsys.readouterr().out class TestSendResponseDebug: