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 <noreply@anthropic.com>

* 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 <noreply@anthropic.com>

* 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 <noreply@anthropic.com>

* 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 <noreply@anthropic.com>

---------

Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>

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;

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(

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;
 

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<String> 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 = "<RadioInfo><Freq>14420000</Freq><Mode>CW</Mode></RadioInfo>";
+		String longPacket = "<?xml version=\"1.0\" encoding=\"utf-8\"?>"
+				+ "<RadioInfo><StationName>CONTEST-PC-WITH-A-LONG-NAME</StationName>"
+				+ "<RadioNr>1</RadioNr><Freq>43220000</Freq><TXFreq>43220000</TXFreq>"
+				+ "<Mode>SSB</Mode><OpCall>DL0TEST</OpCall><IsRunning>False</IsRunning>"
+				+ "<FocusEntry>0</FocusEntry><Antenna>1</Antenna><Rotors>1</Rotors>"
+				+ "<FocusRadioNr>1</FocusRadioNr><IsStereo>False</IsStereo>"
+				+ "<ActiveRadioNr>1</ActiveRadioNr></RadioInfo>";
+		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));
+	}
+}