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

@@ -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:

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: