From 4679c708bc4b01c53adfa1f241ee27338bb5b2ac Mon Sep 17 00:00:00 2001 From: Philipp Wagner Date: Fri, 16 Oct 2026 10:12:46 +0200 Subject: [PATCH] Fix UDP listener defects (#115) (#122) * fix: let ReadUDPbyUCXMessageThread own its DatagramSocket so interrupt() closes it (#115) The field `socket` was a java.net.Socket that was never assigned; the real DatagramSocket lived in a local variable of run(). interrupt() therefore closed nothing and terminateConnection() would have thrown a NullPointerException. The receive loop kept running in two-second timeout steps until the user-disconnect flag was set. The field is now the DatagramSocket, run() binds it when no pre-bound socket was handed in, the loop ends on the interrupt flag or a closed socket, and both interrupt() and terminateConnection() are null-safe. A package-private constructor accepts a pre-bound socket so loopback tests can listen on an ephemeral port; the public constructor used by ChatController is unchanged. Co-Authored-By: Claude Fable 5.1 * fix: stop ReadUDPbyUCXMessageThread truncating datagrams after a short packet (#115) A DatagramPacket keeps the length of the last datagram it received, so a long RadioInfo packet following a short one was cut to the short one's length and failed to parse. The length is reset to the buffer size before every receive, the same way ReadUDPByWintestThread does it. The loopback test sends a short and then a long packet on port 0 and checks that both frequencies arrive. Co-Authored-By: Claude Fable 5.1 * fix: give PstRotatorClient's port constructor a callback, decode AirScout as UTF-8 (#115) The explicit-port constructor left the thread-status callback null, so start() and the first received packet threw an NPE; the default constructor now delegates to it. The AirScout listener decoded replies with the platform charset although the inquirer sends UTF-8. PROJECT_CONTEXT listed PSTRotator as TCP; the client speaks UDP. Co-Authored-By: Claude Opus 5.5 * fix: decode UCX datagrams as UTF-8 and tighten the interrupt test (#115) Review follow-up. UCXLog sends UTF-8 XML, but the listener decoded with the platform charset before handing a String to the parser, so the XML header's encoding was ignored. The port-in-use fallback now sets its receive timeout before the first receive(). The interrupt test waits 1 s, below the 2 s receive timeout, so it fails if interrupt() stops closing the socket; the QRG latches now count one value each. Co-Authored-By: Claude Opus 5.5 --------- Co-authored-by: Claude Fable 5.1 --- .../controller/PstRotatorClient.java | 9 +- .../ReadUDPbyAirScoutMessageThread.java | 4 +- .../controller/ReadUDPbyUCXMessageThread.java | 67 +++++++++-- ...ReadUDPbyUCXMessageThreadLoopbackTest.java | 108 ++++++++++++++++++ docs/PROJECT_CONTEXT.md | 2 +- github_docs/de-Changelog.md | 2 + github_docs/en-Changelog.md | 2 + 7 files changed, 175 insertions(+), 19 deletions(-) create mode 100644 core/src/test/java/kst4contest/controller/ReadUDPbyUCXMessageThreadLoopbackTest.java diff --git a/core/src/main/java/kst4contest/controller/PstRotatorClient.java b/core/src/main/java/kst4contest/controller/PstRotatorClient.java index 81dcdb00..ed7a832f 100644 --- a/core/src/main/java/kst4contest/controller/PstRotatorClient.java +++ b/core/src/main/java/kst4contest/controller/PstRotatorClient.java @@ -37,19 +37,16 @@ public class PstRotatorClient implements Runnable { * @param listener Callback für den Chatcontroller */ public PstRotatorClient(String host, int remotePort, PstRotatorEventListener listener, ThreadStatusCallback callBack) { - this.callBackToController = callBack; - this.host = host; - this.remotePort = remotePort; // Laut Manual antwortet PSTRotator oft auf Port+1. // Wir binden uns also standardmäßig auf remotePort + 1. - this.localPort = remotePort + 1; - this.listener = listener; + this(host, remotePort, remotePort + 1, listener, callBack); } /** * alternative constructor for seting the remote port explicitely */ - public PstRotatorClient(String host, int remotePort, int localPort, PstRotatorEventListener listener) { + public PstRotatorClient(String host, int remotePort, int localPort, PstRotatorEventListener listener, ThreadStatusCallback callBack) { + this.callBackToController = callBack; this.host = host; this.remotePort = remotePort; this.localPort = localPort; diff --git a/core/src/main/java/kst4contest/controller/ReadUDPbyAirScoutMessageThread.java b/core/src/main/java/kst4contest/controller/ReadUDPbyAirScoutMessageThread.java index 2187b290..8eadf1ce 100644 --- a/core/src/main/java/kst4contest/controller/ReadUDPbyAirScoutMessageThread.java +++ b/core/src/main/java/kst4contest/controller/ReadUDPbyAirScoutMessageThread.java @@ -6,6 +6,7 @@ import java.net.DatagramSocket; import java.net.InetSocketAddress; import java.net.SocketException; import java.net.SocketTimeoutException; +import java.nio.charset.StandardCharsets; import java.util.Comparator; import java.util.List; @@ -136,7 +137,8 @@ public class ReadUDPbyAirScoutMessageThread extends Thread { String received = new String( packet.getData(), packet.getOffset(), - packet.getLength() + packet.getLength(), + StandardCharsets.UTF_8 ).trim(); if (received.contains( diff --git a/core/src/main/java/kst4contest/controller/ReadUDPbyUCXMessageThread.java b/core/src/main/java/kst4contest/controller/ReadUDPbyUCXMessageThread.java index 05208720..61e246ca 100644 --- a/core/src/main/java/kst4contest/controller/ReadUDPbyUCXMessageThread.java +++ b/core/src/main/java/kst4contest/controller/ReadUDPbyUCXMessageThread.java @@ -2,6 +2,7 @@ package kst4contest.controller; import java.io.*; import java.net.*; +import java.nio.charset.StandardCharsets; import java.sql.SQLException; import java.util.Arrays; @@ -32,7 +33,12 @@ import kst4contest.model.Band; */ public class ReadUDPbyUCXMessageThread extends Thread { private BufferedReader reader; - private Socket socket; + /** + * The listening socket. It is owned by this thread: {@link #run()} binds it + * (unless a pre-bound socket was handed in) and {@link #interrupt()} closes + * it, which is the only way to wake a blocking {@code receive()}. + */ + private volatile DatagramSocket socket; private ChatController client; private int udpPortNr = 12060; private ThreadStatusCallback callBackToController; @@ -49,6 +55,22 @@ public class ReadUDPbyUCXMessageThread extends Thread { this.callBackToController = callback; } + /** + * Creates a listener on a socket that is already bound. Tests use this to + * listen on an ephemeral loopback port; the application binds the configured + * port itself through the public constructor. + * + * @param boundSocket bound datagram socket this thread takes ownership of + * @param client controller that receives the parsed packets + * @param callback thread-status sink + */ + ReadUDPbyUCXMessageThread(DatagramSocket boundSocket, ChatController client, ThreadStatusCallback callback) { + this.socket = boundSocket; + this.udpPortNr = boundSocket.getLocalPort(); + this.client = client; + this.callBackToController = callback; + } + @Override public void interrupt() { super.interrupt(); @@ -138,30 +160,40 @@ public class ReadUDPbyUCXMessageThread extends Thread { ThreadStateMessage threadStateMessage = new ThreadStateMessage(this.ThreadNickName, true, "initialized", false); callBackToController.onThreadStatus(ThreadNickName,threadStateMessage); - DatagramSocket socket = null; - boolean running; byte[] buf = new byte[1777]; DatagramPacket packet = new DatagramPacket(buf, buf.length); try { // socket = new DatagramSocket(12060); - socket = new DatagramSocket(udpPortNr); + if (socket == null) { + socket = new DatagramSocket(udpPortNr); + } socket.setSoTimeout(2000); //TODO try for end properly } - + catch (SocketException e) { //this will catch the repeating Sockettimeoutexception...nothing to do // e.printStackTrace(); } - while (true) { - + /* + * interrupt() sets the flag and closes the socket; the closed socket makes + * the blocking receive() throw, and the flag ends the loop. + */ + while (!isInterrupted()) { + boolean timeOutIndicator = false; - + // packet = new DatagramPacket(buf, buf.length); //TODO: Changed that due to memory leak, check if all works (seems like that) // DatagramPacket packet = new DatagramPacket(SRPDefinitions.BYTE_BUFFER_MAX_LENGTH); //TODO: Changed that due to memory leak, check if all works (seems like that) try { + /* + * A DatagramPacket remembers the length of the last datagram it + * received; without this reset a long packet after a short one is + * truncated to the short one's length (#115). + */ + packet.setLength(packet.getData().length); socket.receive(packet); } catch (SocketTimeoutException e2) { @@ -171,6 +203,9 @@ public class ReadUDPbyUCXMessageThread extends Thread { // e2.printStackTrace(); } catch (IOException e) { + if (isInterrupted() || socket == null || socket.isClosed()) { + break; + } // TODO Auto-generated catch block e.printStackTrace(); } catch (NullPointerException nE) { @@ -189,8 +224,9 @@ public class ReadUDPbyUCXMessageThread extends Thread { socket = new DatagramSocket(null); socket.setReuseAddress(true); socket.bind(new InetSocketAddress(client.getChatPreferences().getLogsynch_ucxUDPWkdCallListenerPort())); - socket.receive(packet); + packet.setLength(packet.getData().length); socket.setSoTimeout(3000); + socket.receive(packet); } catch (Exception ex) { System.out.println("ReadUDPByUCXMsgTh: Could not solve that. Program Restart needed."); throw new RuntimeException(ex); @@ -203,7 +239,7 @@ public class ReadUDPbyUCXMessageThread extends Thread { InetAddress address = packet.getAddress(); int port = packet.getPort(); // packet = new DatagramPacket(buf, buf.length, address, port); - String received = new String(packet.getData(), packet.getOffset(), packet.getLength()); + String received = new String(packet.getData(), packet.getOffset(), packet.getLength(), StandardCharsets.UTF_8); received = received.trim(); @@ -236,6 +272,12 @@ public class ReadUDPbyUCXMessageThread extends Thread { } + // Covers an interrupt() that arrived before the socket was bound. + DatagramSocket listeningSocket = socket; + if (listeningSocket != null && !listeningSocket.isClosed()) { + listeningSocket.close(); + } + } public String processUCXUDPMessage(String udpPacketToProcess) { @@ -563,7 +605,10 @@ public class ReadUDPbyUCXMessageThread extends Thread { ThreadStateMessage threadStateMessage = new ThreadStateMessage(this.ThreadNickName, false, "terminated", false); callBackToController.onThreadStatus(ThreadNickName,threadStateMessage); - this.socket.close(); + DatagramSocket listeningSocket = this.socket; + if (listeningSocket != null && !listeningSocket.isClosed()) { + listeningSocket.close(); + } return true; diff --git a/core/src/test/java/kst4contest/controller/ReadUDPbyUCXMessageThreadLoopbackTest.java b/core/src/test/java/kst4contest/controller/ReadUDPbyUCXMessageThreadLoopbackTest.java new file mode 100644 index 00000000..9047d687 --- /dev/null +++ b/core/src/test/java/kst4contest/controller/ReadUDPbyUCXMessageThreadLoopbackTest.java @@ -0,0 +1,108 @@ +package kst4contest.controller; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +import java.net.DatagramPacket; +import java.net.DatagramSocket; +import java.net.InetAddress; +import java.net.InetSocketAddress; +import java.nio.charset.StandardCharsets; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.TimeUnit; + +import kst4contest.model.ChatPreferences; +import kst4contest.observe.DirectUiDispatcher; +import kst4contest.observe.SimpleValue; +import org.junit.jupiter.api.Test; + +/** + * Loopback tests for the UCXLog/DXLog UDP listener. Every socket is bound to + * port 0 and every datagram goes to the loopback address, so the tests neither + * collide with a running logger nor send anything to a broadcast address. + */ +class ReadUDPbyUCXMessageThreadLoopbackTest { + + private final ChatController controller = mock(ChatController.class); + private final ChatPreferences preferences = mock(ChatPreferences.class); + private final ThreadStatusCallback statusCallback = (threadName, message) -> { }; + + @Test + void interruptEndsTheReceiveLoopBeforeTheReceiveTimeout() throws Exception { + when(controller.getChatPreferences()).thenReturn(preferences); + ReadUDPbyUCXMessageThread listener = new ReadUDPbyUCXMessageThread(0, controller, statusCallback); + listener.setDaemon(true); + listener.start(); + // Let the thread bind its socket and block in receive() before interrupting it. + Thread.sleep(200); + + /* + * The receive timeout is 2 s. Waiting only 1 s proves that interrupt() + * closed the socket, rather than the loop ending on the next timeout. + */ + listener.interrupt(); + listener.join(1_000); + + assertFalse(listener.isAlive(), "listener must stop once interrupted"); + } + + /** + * A DatagramPacket remembers the length of the last datagram it received. + * Without resetting it before every receive(), a long RadioInfo packet that + * follows a short one is cut to the short one's length and fails to parse. + * The QRG probe needs no database: RadioInfo only sets the first-category + * frequency through the UI dispatcher. + */ + @Test + void longDatagramAfterShortOneIsProcessedCompletely() throws Exception { + when(controller.getChatPreferences()).thenReturn(preferences); + when(controller.uiDispatcher()).thenReturn(new DirectUiDispatcher()); + when(preferences.isTrxSynch_ucxLogUDPListenerEnabled()).thenReturn(true); + SimpleValue qrg = new SimpleValue<>(""); + when(preferences.getMYQRGFirstCat()).thenReturn(qrg); + CountDownLatch firstQrg = new CountDownLatch(1); + CountDownLatch secondQrg = new CountDownLatch(1); + qrg.addListener(value -> { + if (value.equals("144.200.00")) firstQrg.countDown(); + if (value.equals("432.200.00")) secondQrg.countDown(); + }); + + String shortPacket = "14420000CW"; + String longPacket = "" + + "CONTEST-PC-WITH-A-LONG-NAME" + + "14322000043220000" + + "SSBDL0TESTFalse" + + "011" + + "1False" + + "1"; + assertTrue(longPacket.length() > shortPacket.length()); + + InetAddress loopback = InetAddress.getLoopbackAddress(); + DatagramSocket bound = new DatagramSocket(new InetSocketAddress(loopback, 0)); + ReadUDPbyUCXMessageThread listener = new ReadUDPbyUCXMessageThread(bound, controller, statusCallback); + listener.setDaemon(true); + listener.start(); + try (DatagramSocket sender = new DatagramSocket(new InetSocketAddress(loopback, 0))) { + InetSocketAddress target = new InetSocketAddress(loopback, bound.getLocalPort()); + + send(sender, target, shortPacket); + assertTrue(firstQrg.await(5, TimeUnit.SECONDS), "short packet must be processed"); + assertEquals("144.200.00", qrg.get()); + + send(sender, target, longPacket); + assertTrue(secondQrg.await(5, TimeUnit.SECONDS), "long packet must be processed"); + assertEquals("432.200.00", qrg.get()); + } finally { + listener.interrupt(); + listener.join(2_000); + } + } + + private static void send(DatagramSocket sender, InetSocketAddress target, String payload) throws Exception { + byte[] bytes = payload.getBytes(StandardCharsets.UTF_8); + sender.send(new DatagramPacket(bytes, bytes.length, target)); + } +} diff --git a/docs/PROJECT_CONTEXT.md b/docs/PROJECT_CONTEXT.md index 5cbdeabf..c205d2f8 100644 --- a/docs/PROJECT_CONTEXT.md +++ b/docs/PROJECT_CONTEXT.md @@ -99,7 +99,7 @@ Known integration areas include: - AirScout; - UCXLog / DXLog UDP XML (`contactinfo`, `contactreplace`); - Win-Test UDP; -- PSTRotator TCP; +- PSTRotator UDP; - DXCluster; - local SQLite persistence. diff --git a/github_docs/de-Changelog.md b/github_docs/de-Changelog.md index 2f74c863..ef9e3b26 100644 --- a/github_docs/de-Changelog.md +++ b/github_docs/de-Changelog.md @@ -48,6 +48,8 @@ Mehrere Operateure an einem Rechner können jetzt eigene Rufzeichen, Locators un - **Kein Locator für die zweite Kategorie mehr:** Intern gab es eine Einstellung für einen eigenen Locator der zweiten Chat-Kategorie, die weder in der Oberfläche noch in der `preferences.xml` erschien und deshalb immer auf ihrem eingebauten Standardwert stand. Beim Verbindungsaufbau wurde dieser Standardwert mit dem eigenen Locator verglichen und eine Warnung ausgegeben, dass der zweite Locator ignoriert werde. ON4KST verwendet ohnehin einen Locator pro TCP-Sitzung; die Einstellung und die Warnung sind entfernt. Damit ist [Issue #104](https://github.com/praktimarc/kst4contest/issues/104) behoben. - **Linux-Beta-Pakete lassen sich auf das stabile Paket aktualisieren:** DEB- und RPM-Pakete eines Beta-Tags trugen dieselbe Versionsnummer wie das folgende stabile Paket, sodass `dnf` und `dpkg` die Aktualisierung als gleiche Version ablehnten. Beta-Pakete erhalten jetzt eine Vorab-Release-Nummer (`1.50.0-0.beta.001`) unter der des stabilen Pakets (`1.50.0-1`), und der Release-Lauf bricht ab, wenn der Tag nicht zur `composePackageVersion` passt. Damit ist [Issue #113](https://github.com/praktimarc/kst4contest/issues/113) behoben. +- **UCXLog-Pakete werden nicht mehr abgeschnitten:** Nach einem kurzen UDP-Paket las der Empfänger für UCXLog-kompatible Logprogramme jedes längere Folgepaket nur bis zur Länge des kurzen, sodass Logeinträge unvollständig ankommen konnten. Außerdem schließt das Trennen den Socket des Empfängers jetzt sofort, statt bis zum nächsten Timeout zu warten. UCXLog-Pakete und Antworten von AirScout werden jetzt immer als UTF-8 dekodiert statt im Standardzeichensatz des Systems, sodass Umlaute in Stationsnamen und Operator-Rufzeichen unter Windows nicht mehr verstümmelt ankommen. Damit ist [Issue #115](https://github.com/praktimarc/kst4contest/issues/115) behoben. + ### Hinweise zur Aktualisierung - Bestehende Installationen werden **nicht** verändert: `preferences.xml` und `praktiKST.db` bleiben genau dort liegen, wo sie sind, und werden zum Profil **Default**. Es wird keine Datei verschoben oder kopiert. diff --git a/github_docs/en-Changelog.md b/github_docs/en-Changelog.md index 52d34c49..72e333f0 100644 --- a/github_docs/en-Changelog.md +++ b/github_docs/en-Changelog.md @@ -48,6 +48,8 @@ Several operators sharing one computer can now use their own callsigns, locators - **No second-category locator any more:** internally there was a setting for a separate locator of the second chat category that appeared neither in the user interface nor in `preferences.xml`, so it always kept its compiled-in default. During login that default was compared with the station's locator and a warning was emitted that the second locator is ignored. ON4KST uses one locator per TCP session anyway; the setting and the warning are removed. This fixes [Issue #104](https://github.com/praktimarc/kst4contest/issues/104). - **Linux beta packages upgrade to the stable package:** DEB and RPM packages of a beta tag carried the same version number as the following stable package, so `dnf` and `dpkg` refused the upgrade as an equal version. Beta packages now carry a pre-release number (`1.50.0-0.beta.001`) below the stable package (`1.50.0-1`), and the release run aborts when the tag does not match `composePackageVersion`. This fixes [Issue #113](https://github.com/praktimarc/kst4contest/issues/113). +- **UCXLog packets are no longer cut off:** after a short UDP packet, the listener for UCXLog-compatible loggers read every longer following packet only up to the length of the short one, so log entries could arrive incomplete. In addition, disconnecting now closes the listener's socket immediately instead of waiting for the next timeout. UCXLog packets and AirScout replies are now always decoded as UTF-8 instead of the system default, so umlauts in station names and operator callsigns no longer turn into garbage on Windows. This fixes [Issue #115](https://github.com/praktimarc/kst4contest/issues/115). + ### Upgrade notes - Existing installations are **not** modified: `preferences.xml` and `praktiKST.db` stay exactly where they are and become the **Default** profile. No file is moved or copied.