Mostly fix NetworkTest (#357)

* (busily) wait for client to connect in testLogin

* kick clients instead of directly disconnecting them at the end of testLogin; fixes race condition causing testLogin flakiness
tests that wait on DummyClientPacketHandler#disconnectFromServerLatch are still flaky: NetworkTest::connectClient can throw BindException

* remove unnecessary EnigmaClient::disconnect calls after DummyClientPacketHandler#disconnectFromServerLatch waits
replace NetworkTest::testTakenUsername's client1 diconnect call with kickAfterTest
repeat each test 100 times (except testUnapprovedMessage which takes 3 sec. per success)
warn about over-repeating tests that wait on DummyClientPacketHandler#disconnectFromServerLatch

* adjust client kicking in testTakenUserName

* busyAwaitConnection of first client in testTakenuserName

* comment that testTakenUsername is still flaky on CI :-[

* use CountDownLock instead of busy waiting

* require one connection waiter at a time

* try waiting for second client connection in testTakenUsername

* stop repeating testTakenUsername

* javadoc note about changing testLogin

Author: supersaiyansubtlety

enigma-server/src/main/java/org/quiltmc/enigma/network/EnigmaServer.java

@@ -87,7 +87,8 @@ public void start() throws IOException {
 		thread.start();
 	}
 
-	private void acceptClient() throws IOException {
+	@VisibleForTesting
+	Socket acceptClient() throws IOException {
 		Socket client = this.socket.accept();
 		this.clients.add(client);
 		this.unapprovedClients.add(client);
@@ -127,6 +128,8 @@ private void acceptClient() throws IOException {
 		thread.setName("Server I/O thread #" + (nextIoId++));
 		thread.setDaemon(true);
 		thread.start();
+
+		return client;
 	}
 
 	public void stop() {

enigma-server/src/test/java/org/quiltmc/enigma/network/DummyClientPacketHandler.java

@@ -1,5 +1,7 @@
 package org.quiltmc.enigma.network;
 
+import org.jspecify.annotations.NonNull;
+import org.junit.jupiter.api.Assertions;
 import org.quiltmc.enigma.api.translation.mapping.EntryChange;
 import org.quiltmc.enigma.api.translation.mapping.EntryMapping;
 import org.quiltmc.enigma.api.translation.mapping.tree.EntryTree;
@@ -10,6 +12,7 @@
 
 public class DummyClientPacketHandler implements ClientPacketHandler {
 	TestEnigmaClient client;
+	@NonNull
 	CountDownLatch disconnectFromServerLatch = new CountDownLatch(1);
 
 	@Override
@@ -21,15 +24,14 @@ public boolean applyChangeFromServer(EntryChange<?> change) {
 		return true;
 	}
 
+	@SuppressWarnings("deprecation")
 	@Override
 	public void disconnectIfConnected(String reason) {
-		if (this.client != null) {
-			this.client.disconnect();
-		}
+		Assertions.assertNotNull(this.client, "No client!");
+		this.client.disconnect();
 
-		if (this.disconnectFromServerLatch != null) {
-			this.disconnectFromServerLatch.countDown();
-		}
+		Assertions.assertNotNull(this.disconnectFromServerLatch, "No disconnection latch!");
+		this.disconnectFromServerLatch.countDown();
 	}
 
 	@Override

enigma-server/src/test/java/org/quiltmc/enigma/network/NetworkTest.java

@@ -3,37 +3,55 @@
 import org.junit.jupiter.api.AfterAll;
 import org.junit.jupiter.api.Assertions;
 import org.junit.jupiter.api.BeforeAll;
+import org.junit.jupiter.api.RepeatedTest;
 import org.junit.jupiter.api.Test;
 import org.quiltmc.enigma.TestUtil;
 import org.quiltmc.enigma.api.Enigma;
 import org.quiltmc.enigma.api.EnigmaProject;
 import org.quiltmc.enigma.api.ProgressListener;
 import org.quiltmc.enigma.api.class_provider.ClasspathClassProvider;
-import org.quiltmc.enigma.api.translation.mapping.EntryRemapper;
 import org.quiltmc.enigma.network.packet.c2s.LoginC2SPacket;
 import org.quiltmc.enigma.network.packet.c2s.MessageC2SPacket;
+import org.quiltmc.enigma.network.packet.s2c.KickS2CPacket;
 import org.quiltmc.enigma.util.Utils;
 
 import java.io.IOException;
+import java.net.BindException;
+import java.net.Socket;
 import java.nio.file.Path;
+import java.util.List;
 import java.util.concurrent.CountDownLatch;
 import java.util.concurrent.TimeUnit;
 
+/**
+ * <b>Warning</b>: Using {@link RepeatedTest @RepeatedTest} on tests that wait on
+ * {@link DummyClientPacketHandler#disconnectFromServerLatch} eventually causes
+ * {@link BindException}: {@code Address already in use: connect}.
+ *
+ * <p> It seems like all outbound sockets get exhausted and have a timeout before they become available.
+ *
+ * <p> On my (supersaiyansubtlety's) PC, the first exception comes after ~20,000 combined repetitions within ~2 minutes,
+ * after which exceptions are frequently thrown until ~2 minutes have passed without any repetitions.
+ *
+ * <p> Each {@link DummyClientPacketHandler#disconnectFromServerLatch}-waiting test succeeded with 1,000 repetitions.
+ *
+ * <p> Repetitions are set to {@value #DEFAULT_REPETITIONS} to reasonably test flakiness while avoiding the socket cap.
+ */
 public class NetworkTest {
 	private static final Path JAR = TestUtil.obfJar("complete");
 	private static final String PASSWORD = "foobar";
+	private static final int DEFAULT_REPETITIONS = 100;
+
 	private static byte[] checksum;
 	private static TestEnigmaServer server;
-	private static EntryRemapper remapper;
 
 	@BeforeAll
 	public static void startServer() throws IOException {
 		Enigma enigma = Enigma.create();
 		EnigmaProject project = enigma.openJar(JAR, new ClasspathClassProvider(), ProgressListener.createEmpty());
 
 		checksum = Utils.zipSha1(JAR);
-		remapper = project.getRemapper();
-		server = new TestEnigmaServer(checksum, PASSWORD.toCharArray(), remapper, 0);
+		server = new TestEnigmaServer(checksum, PASSWORD.toCharArray(), project.getRemapper(), 0);
 
 		server.start();
 	}
@@ -50,38 +68,63 @@ private static TestEnigmaClient connectClient(ClientPacketHandler handler) throw
 		return client;
 	}
 
-	@Test
+	private static void awaitNextConnection() throws InterruptedException {
+		Assertions.assertTrue(server.awaitNextConnection(3, TimeUnit.SECONDS), "Client did not connect");
+	}
+
+	/**
+	 * <b>Never</b> directly {@linkplain EnigmaClient#disconnect() disconnect} clients after tests; <b>always</b>
+	 * either manually {@link EnigmaServer#kick(Socket, String) kick} them (like this method does) or wait for them to
+	 * be disconnected using {@link DummyClientPacketHandler#disconnectFromServerLatch}.
+	 *
+	 * <p> Directly {@linkplain EnigmaClient#disconnect() disconnecting} a client creates a race condition:
+	 * The {@code "disconnect.disconnected"} {@link EnigmaServer#kick(Socket, String) kick} call in
+	 * {@link EnigmaServer}'s client threads sends a {@link KickS2CPacket} that can be received by
+	 * <em>other</em> test's clients.
+	 */
+	static void kickAfterTest(Socket clientSocket) {
+		server.kick(clientSocket, "test complete");
+	}
+
+	/**
+	 * When making changes to this test, <em>always</em> test with 100,000 repetitions.
+	 */
+	@RepeatedTest(DEFAULT_REPETITIONS)
 	public void testLogin() throws IOException, InterruptedException {
-		var handler = new DummyClientPacketHandler();
-		var client = connectClient(handler);
+		final var handler = new DummyClientPacketHandler();
+
+		server.queueConnectionWait();
+
+		final TestEnigmaClient client = connectClient(handler);
 		handler.client = client;
 
-		Assertions.assertFalse(server.getClients().isEmpty());
-		Assertions.assertFalse(server.getUnapprovedClients().isEmpty());
+		awaitNextConnection();
 
+		Assertions.assertNotEquals(0, handler.disconnectFromServerLatch.getCount(), "The client was disconnected by the server");
+
+		final Socket clientSocket = server.getClients().get(0);
+		final CountDownLatch changeConfirmation = server.waitChangeConfirmation(clientSocket, 1);
 		client.sendPacket(new LoginC2SPacket(checksum, PASSWORD.toCharArray(), "alice"));
-		var confirmed = server.waitChangeConfirmation(server.getClients().get(0), 1)
-				.await(3, TimeUnit.SECONDS);
+		final boolean confirmed = changeConfirmation.await(3, TimeUnit.SECONDS);
 
-		Assertions.assertNotEquals(0, handler.disconnectFromServerLatch.getCount(), "The client was disconnected by the server");
 		Assertions.assertTrue(confirmed, "Timed out waiting for the change confirmation");
-		client.disconnect();
+
+		kickAfterTest(clientSocket);
 	}
 
-	@Test
+	@RepeatedTest(DEFAULT_REPETITIONS)
 	public void testInvalidUsername() throws IOException, InterruptedException {
 		var handler = new DummyClientPacketHandler();
 		var client = connectClient(handler);
 		handler.client = client;
 
 		client.sendPacket(new LoginC2SPacket(checksum, PASSWORD.toCharArray(), "<span style=\"color: lavender\">eve</span>"));
-		var disconnected = handler.disconnectFromServerLatch.await(3, TimeUnit.SECONDS);
+		boolean disconnected = handler.disconnectFromServerLatch.await(3, TimeUnit.SECONDS);
 
 		Assertions.assertTrue(disconnected, "Timed out waiting for the server to kick the client");
-		client.disconnect();
 	}
 
-	@Test
+	@RepeatedTest(DEFAULT_REPETITIONS)
 	public void testWrongPassword() throws IOException, InterruptedException {
 		var handler = new DummyClientPacketHandler();
 		var client = connectClient(handler);
@@ -91,31 +134,43 @@ public void testWrongPassword() throws IOException, InterruptedException {
 		var disconnected = handler.disconnectFromServerLatch.await(3, TimeUnit.SECONDS);
 
 		Assertions.assertTrue(disconnected, "Timed out waiting for the server to kick the client");
-		client.disconnect();
 	}
 
+	// FIXME this test is flaky when run from workflows/build.yml
 	@Test
 	public void testTakenUsername() throws IOException, InterruptedException {
-		var packet = new LoginC2SPacket(checksum, PASSWORD.toCharArray(), "alice");
+		final var packet = new LoginC2SPacket(checksum, PASSWORD.toCharArray(), "alice");
 
-		var handler = new DummyClientPacketHandler();
-		var client = connectClient(handler);
-		handler.client = client;
-		client.sendPacket(packet);
+		final var handler = new DummyClientPacketHandler();
+
+		server.queueConnectionWait();
 
-		var handler2 = new DummyClientPacketHandler();
-		var client2 = connectClient(handler2);
+		final TestEnigmaClient client1 = connectClient(handler);
+		handler.client = client1;
+		client1.sendPacket(packet);
+
+		awaitNextConnection();
+
+		final var handler2 = new DummyClientPacketHandler();
+
+		server.queueConnectionWait();
+
+		final TestEnigmaClient client2 = connectClient(handler2);
 		handler2.client = client2;
 
+		awaitNextConnection();
+
 		client2.sendPacket(packet);
-		var disconnected = handler2.disconnectFromServerLatch.await(3, TimeUnit.SECONDS);
+		final boolean disconnected = handler2.disconnectFromServerLatch.await(3, TimeUnit.SECONDS);
 
 		Assertions.assertTrue(disconnected, "Timed out waiting for the server to kick the client");
-		client.disconnect();
-		client2.disconnect();
+
+		for (final Socket clientSocket : List.copyOf(server.getClients())) {
+			kickAfterTest(clientSocket);
+		}
 	}
 
-	@Test
+	@RepeatedTest(DEFAULT_REPETITIONS)
 	public void testWrongChecksum() throws IOException, InterruptedException {
 		var handler = new DummyClientPacketHandler();
 		var client = connectClient(handler);
@@ -126,9 +181,9 @@ public void testWrongChecksum() throws IOException, InterruptedException {
 		var disconnected = handler.disconnectFromServerLatch.await(3, TimeUnit.SECONDS);
 
 		Assertions.assertTrue(disconnected, "Timed out waiting for the server to kick the client");
-		client.disconnect();
 	}
 
+	// no repeats because successes take at least 3 seconds
 	@Test
 	public void testUnapprovedMessage() throws IOException, InterruptedException {
 		var handler = new DummyClientPacketHandler();

enigma-server/src/test/java/org/quiltmc/enigma/network/TestEnigmaClient.java

@@ -2,6 +2,7 @@
 
 import org.quiltmc.enigma.network.packet.Packet;
 
+import java.net.Socket;
 import java.util.concurrent.CountDownLatch;
 
 public class TestEnigmaClient extends EnigmaClient {
@@ -24,4 +25,13 @@ public void sendPacket(Packet<ServerPacketHandler> packet) {
 			this.packetSentLatch.countDown();
 		}
 	}
+
+	/**
+	 * Don't call this at the end of a test; use {@link NetworkTest#kickAfterTest(Socket)} instead.
+	 */
+	@Override
+	@Deprecated
+	public synchronized void disconnect() {
+		super.disconnect();
+	}
 }

enigma-server/src/test/java/org/quiltmc/enigma/network/TestEnigmaServer.java

@@ -1,5 +1,7 @@
 package org.quiltmc.enigma.network;
 
+import org.jspecify.annotations.NonNull;
+import org.junit.jupiter.api.Assertions;
 import org.quiltmc.enigma.api.translation.mapping.EntryRemapper;
 
 import java.io.IOException;
@@ -9,8 +11,11 @@
 import java.util.concurrent.ConcurrentHashMap;
 import java.util.concurrent.CountDownLatch;
 import java.util.concurrent.LinkedBlockingDeque;
+import java.util.concurrent.TimeUnit;
 
 public class TestEnigmaServer extends EnigmaServer {
+	@NonNull
+	private CountDownLatch connectionLatch = new CountDownLatch(0);
 	private final Map<Socket, CountDownLatch> changeConfirmationLatches = new ConcurrentHashMap<>();
 	private final BlockingQueue<Runnable> tasks = new LinkedBlockingDeque<>();
 
@@ -38,6 +43,24 @@ public void start() throws IOException {
 		tasksThread.start();
 	}
 
+	@Override
+	Socket acceptClient() throws IOException {
+		final Socket socket = super.acceptClient();
+
+		this.connectionLatch.countDown();
+
+		return socket;
+	}
+
+	public void queueConnectionWait() {
+		Assertions.assertEquals(0, this.connectionLatch.getCount());
+		this.connectionLatch = new CountDownLatch(1);
+	}
+
+	public boolean awaitNextConnection(int timeout, TimeUnit unit) throws InterruptedException {
+		return this.connectionLatch.await(timeout, unit);
+	}
+
 	@Override
 	protected void runOnThread(Runnable task) {
 		this.tasks.add(task);