test: wait for virtualenv provisioning and fix a TestServer bind race

README.md

@@ -73,14 +73,19 @@ Use **-DskipITs** to avoid running integration tests.
 
 The integration tests check to see if a virtual env is running. If not they will
 attempt to start one. This requires dockerd or desktop docker to be running.
+They then wait until `install_PKAM_Keys` has exited having provisioned every
+atSign the container hosts. If it misses any, or has not exited within three
+minutes, the integration tests fail, naming the atSigns it did not provision.
+A virtual env that is already running (anything answering on
+vip.ve.atsign.zone:64) is used as it is, with no check that it is provisioned.
 
 For CI, "standing up" and then "tearing down" the docker container is the intended
 behavior. However, for a developer this will be slow so it's preferable to "standup"
 the virtual env independently, like this:
 
 ```shell
 cd at_client/src/test/resources/org/atsign/virtualenv
-docker compose -f up
+docker compose up
 ```
 
 The start-up can take a few minutes and involves running scripts that install the
@@ -89,11 +94,13 @@ following log output:
 
 ```text
 ...
-virtualenv-1  | SHOUT|...| install_PKAM_Keys |cramAndPkamAuth successful for @chris
-virtualenv-1  | SHOUT|...| install_PKAM_Keys |cramAndPkamAuth successful for @policy1
-virtualenv-1  | SHOUT|...| install_PKAM_Keys |cramAndPkamAuth successful for @emoji
+virtualenv-1  | install_PKAM_Keys exited with status 0
 ```
 
+Each atSign the script provisions logs a `cramAndPkamAuth successful` or
+`cramAuthOnly successful` line before that; a status other than 0, or an atSign
+with no such line, means the environment is not fully provisioned.
+
 The tests require test keys (CRAM keys and atKeys) which the virtual env was
 built with. The pom contains a plugin which downloads the
 [at_demo_data](https://pub.dev/packages/at_demo_data) package and unpacks it
@@ -110,8 +117,8 @@ be necessary to reset the docker container, like this:
 
 ```shell
 cd at_client/src/test/resources/org/atsign/virtualenv
-docker compose -f down
-docker compose -f up
+docker compose down
+docker compose up
 ```
 
 ## Virtual Environment

at_client/src/test/java/org/atsign/client/impl/AtClientsIT.java

@@ -1,6 +1,5 @@
 package org.atsign.client.impl;
 
-import static java.util.concurrent.TimeUnit.SECONDS;
 import static org.hamcrest.MatcherAssert.assertThat;
 import static org.hamcrest.Matchers.*;
 
@@ -11,7 +10,6 @@
 import org.atsign.client.api.AtSign;
 import org.atsign.client.impl.common.ReconnectStrategy;
 import org.atsign.client.impl.util.KeysUtils;
-import org.atsign.cucumber.helpers.Helpers;
 import org.atsign.virtualenv.VirtualEnv;
 import org.junit.jupiter.api.AfterAll;
 import org.junit.jupiter.api.BeforeAll;
@@ -27,9 +25,7 @@ class AtClientsIT {
 
   @BeforeAll
   public static void classSetup() throws Exception {
-    if (!Helpers.isHostPortReachable("vip.ve.atsign.zone:64", SECONDS.toMillis(2))) {
-      VirtualEnv.setUp();
-    }
+    VirtualEnv.setUp();
     atSign = AtSign.createAtSign("colin");
     keysFile = new File("target/at_demo_data/lib/assets/atkeys/@colin.atKeys");
     keys = KeysUtils.loadKeys(keysFile);

at_client/src/test/java/org/atsign/client/impl/AtCommandExecutorsIT.java

@@ -1,6 +1,5 @@
 package org.atsign.client.impl;
 
-import static java.util.concurrent.TimeUnit.SECONDS;
 import static org.hamcrest.MatcherAssert.assertThat;
 import static org.hamcrest.Matchers.*;
 import static org.junit.jupiter.api.Assertions.assertThrows;
@@ -14,7 +13,6 @@
 import org.atsign.client.impl.commands.ScanCommands;
 import org.atsign.client.impl.common.ReconnectStrategy;
 import org.atsign.client.impl.util.KeysUtils;
-import org.atsign.cucumber.helpers.Helpers;
 import org.atsign.virtualenv.VirtualEnv;
 import org.junit.jupiter.api.BeforeAll;
 import org.junit.jupiter.api.Test;
@@ -29,9 +27,7 @@ class AtCommandExecutorsIT {
 
   @BeforeAll
   public static void classSetup() throws Exception {
-    if (!Helpers.isHostPortReachable("vip.ve.atsign.zone:64", SECONDS.toMillis(2))) {
-      VirtualEnv.setUp();
-    }
+    VirtualEnv.setUp();
     atSign = AtSign.createAtSign("colin");
     keys = KeysUtils.loadKeys(new File("target/at_demo_data/lib/assets/atkeys/@colin.atKeys"));
   }

at_client/src/test/java/org/atsign/client/impl/cli/ActivateIT.java

@@ -22,7 +22,6 @@
 
 import org.atsign.client.impl.util.KeysUtils;
 import org.atsign.cucumber.helpers.AtDemoData;
-import org.atsign.cucumber.helpers.Helpers;
 import org.atsign.virtualenv.VirtualEnv;
 import org.junit.jupiter.api.AfterEach;
 import org.junit.jupiter.api.BeforeAll;
@@ -40,9 +39,7 @@ class ActivateIT {
 
   @BeforeAll
   public static void classSetup() {
-    if (!Helpers.isHostPortReachable("vip.ve.atsign.zone:64", SECONDS.toMillis(2))) {
-      VirtualEnv.setUp();
-    }
+    VirtualEnv.setUp();
   }
 
   @BeforeEach

at_client/src/test/java/org/atsign/client/impl/cli/CliIT.java

@@ -1,6 +1,5 @@
 package org.atsign.client.impl.cli;
 
-import static java.util.concurrent.TimeUnit.SECONDS;
 import static org.hamcrest.MatcherAssert.assertThat;
 import static org.hamcrest.Matchers.*;
 import static org.junit.jupiter.api.Assertions.assertThrows;
@@ -14,7 +13,6 @@
 import java.util.stream.Collectors;
 
 import org.atsign.client.impl.util.KeysUtils;
-import org.atsign.cucumber.helpers.Helpers;
 import org.atsign.virtualenv.VirtualEnv;
 import org.junit.jupiter.api.AfterAll;
 import org.junit.jupiter.api.BeforeAll;
@@ -31,9 +29,7 @@ class CliIT {
 
   @BeforeAll
   public static void classSetup() {
-    if (!Helpers.isHostPortReachable(VIRTUAL_ENV_ROOT, SECONDS.toMillis(2))) {
-      VirtualEnv.setUp();
-    }
+    VirtualEnv.setUp();
     AT_SIGN_KEYS_DIR = KeysUtils.expectedKeysFilesLocation;
     ATSIGN_KEYS_SUFFIX = KeysUtils.keysFileSuffix;
     KeysUtils.expectedKeysFilesLocation = "target/at_demo_data/lib/assets/atkeys";

at_client/src/test/java/org/atsign/client/impl/netty/NettyAtCommandExecutorIT.java

@@ -1,6 +1,5 @@
 package org.atsign.client.impl.netty;
 
-import static java.util.concurrent.TimeUnit.SECONDS;
 import static org.atsign.client.api.AtSign.createAtSign;
 import static org.awaitility.Awaitility.await;
 import static org.hamcrest.MatcherAssert.assertThat;
@@ -11,7 +10,6 @@
 
 import org.atsign.client.api.AtSign;
 import org.atsign.client.impl.netty.NettyAtCommandExecutor.NettyAtCommandExecutorBuilder;
-import org.atsign.cucumber.helpers.Helpers;
 import org.atsign.virtualenv.VirtualEnv;
 import org.hamcrest.Matchers;
 import org.junit.jupiter.api.BeforeAll;
@@ -24,9 +22,7 @@ class NettyAtCommandExecutorIT {
 
   @BeforeAll
   public static void classSetup() {
-    if (!Helpers.isHostPortReachable("vip.ve.atsign.zone:64", SECONDS.toMillis(2))) {
-      VirtualEnv.setUp();
-    }
+    VirtualEnv.setUp();
   }
 
   @Test

at_client/src/test/java/org/atsign/client/impl/netty/TestServer.java

@@ -1,6 +1,7 @@
 package org.atsign.client.impl.netty;
 
 import static java.util.concurrent.TimeUnit.MILLISECONDS;
+import static java.util.concurrent.TimeUnit.SECONDS;
 import static org.atsign.client.impl.common.Preconditions.checkNotNull;
 
 import java.io.*;
@@ -56,6 +57,8 @@ public class TestServer implements AutoCloseable {
 
   private AtomicBoolean isAccept = new AtomicBoolean(true);
 
+  private final AtomicBoolean isWorkerInAccept = new AtomicBoolean();
+
   private AtomicBoolean shutdown = new AtomicBoolean();
 
   private final Thread workerThread;
@@ -72,9 +75,15 @@ public TestServer() throws Exception {
     workerThread = new Thread(() -> {
       while (!shutdown.get()) {
         try {
-          if (serverSocket != null && socket == null && isAccept.get()) {
-            log.debug("accepting on {}", serverSocket.getLocalPort());
-            socket = serverSocket.accept();
+          ServerSocket listening = serverSocket;
+          if (listening != null && socket == null && isAccept.get()) {
+            log.debug("accepting on {}", listening.getLocalPort());
+            isWorkerInAccept.set(true);
+            try {
+              socket = listening.accept();
+            } finally {
+              isWorkerInAccept.set(false);
+            }
             writer = new PrintWriter(new BufferedWriter(new OutputStreamWriter(socket.getOutputStream())), false);
             reader = new BufferedReader(new InputStreamReader(socket.getInputStream()));
             requestHandler.accept(null);
@@ -145,13 +154,29 @@ public void closeClientSocket() {
     }
   }
 
+  /**
+   * Closes the listening socket and any accepted one, returning once the worker thread has left
+   * {@code accept()}, so that the port is free to be bound again.
+   */
   public void closeServerSocket() {
     try {
-      closeNoThrow(serverSocket, socket, reader, writer);
+      closeNoThrow(serverSocket);
     } finally {
       serverSocket = null;
-      socket = null;
     }
+    // NOTE the JDK keeps a socket listening until the thread blocked in its accept() wakes
+    long deadline = System.nanoTime() + SECONDS.toNanos(5);
+    while (isWorkerInAccept.get()) {
+      if (System.nanoTime() - deadline > 0) {
+        throw new IllegalStateException("the worker thread has not left accept()");
+      }
+      Thread.onSpinWait();
+    }
+    closeClientSocket();
+  }
+
+  public boolean isWorkerInAccept() {
+    return isWorkerInAccept.get();
   }
 
   public void accept() {

at_client/src/test/java/org/atsign/client/impl/netty/TestServerTest.java

@@ -0,0 +1,27 @@
+package org.atsign.client.impl.netty;
+
+import static java.util.concurrent.TimeUnit.MILLISECONDS;
+import static java.util.concurrent.TimeUnit.SECONDS;
+import static org.awaitility.Awaitility.await;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.equalTo;
+import static org.hamcrest.Matchers.is;
+
+import org.junit.jupiter.api.Test;
+
+class TestServerTest {
+
+  @Test
+  void testCloseServerSocketReturnsOnceTheWorkerHasLeftAcceptSoThePortRebindsAtOnce() throws Exception {
+    try (TestServer server = new TestServer()) {
+      int port = server.getPort();
+      await().pollInterval(1, MILLISECONDS).atMost(1, SECONDS).until(server::isWorkerInAccept);
+
+      server.closeServerSocket();
+
+      assertThat("worker is still inside accept()", server.isWorkerInAccept(), is(false));
+      server.newServerSocket();
+      assertThat(server.getPort(), equalTo(port));
+    }
+  }
+}

at_client/src/test/java/org/atsign/cucumber/CucumberVirtualEnvPlugin.java

@@ -1,8 +1,5 @@
 package org.atsign.cucumber;
 
-import static java.util.concurrent.TimeUnit.SECONDS;
-
-import org.atsign.cucumber.helpers.Helpers;
 import org.atsign.virtualenv.VirtualEnv;
 
 import io.cucumber.plugin.EventListener;
@@ -13,8 +10,6 @@ public class CucumberVirtualEnvPlugin implements EventListener {
 
   @Override
   public void setEventPublisher(EventPublisher publisher) {
-    if (!Helpers.isHostPortReachable("vip.ve.atsign.zone:64", SECONDS.toMillis(2))) {
-      publisher.registerHandlerFor(TestRunStarted.class, e -> VirtualEnv.setUp());
-    }
+    publisher.registerHandlerFor(TestRunStarted.class, e -> VirtualEnv.setUp());
   }
 }