More client lifecycle improvements

clientserver/src/main/java/net/rptools/clientserver/simple/Handshake.java

@@ -14,53 +14,23 @@
  */
 package net.rptools.clientserver.simple;
 
-import net.rptools.clientserver.simple.connection.Connection;
+import java.util.function.BiConsumer;
 
-public interface Handshake {
+public interface Handshake<T> {
 
-  /**
-   * Returns if the handshake has been successful or not.
-   *
-   * @return {@code true} if the handshake has been successful, {code false} if it has failed or is
-   *     still in progress.
-   */
-  boolean isSuccessful();
-
-  /**
-   * Returns the message for the error -- if any -- that occurred during the handshake.
-   *
-   * @return the message for the error that occurred during handshake.
-   */
-  String getErrorMessage();
-
-  /**
-   * Returns the connection for this {@code ServerHandshake}.
-   *
-   * @return the connection for this {@code ServerHandshake}.
-   */
-  Connection getConnection();
-
-  /**
-   * Returns the exception -- if any -- that occurred during processing of the handshake.
-   *
-   * @return the exception that occurred during the processing of the handshake.
-   */
-  Exception getException();
-
-  /**
-   * Adds an observer to the handshake process.
-   *
-   * @param observer the observer of the handshake process.
-   */
-  void addObserver(HandshakeObserver observer);
-
-  /**
-   * Removes an observer from the handshake process.
-   *
-   * @param observer the observer of the handshake process.
-   */
-  void removeObserver(HandshakeObserver observer);
+  void whenComplete(BiConsumer<? super T, ? super Throwable> callback);
 
   /** Starts the handshake process. */
   void startHandshake();
+
+  class Failure extends Exception {
+    // TODO When we have access to I18N, force this to be translatable.
+    public Failure(String message) {
+      super(message);
+    }
+
+    public Failure(String message, Throwable cause) {
+      super(message, cause);
+    }
+  }
 }

clientserver/src/main/java/net/rptools/clientserver/simple/server/AbstractServer.java

@@ -16,27 +16,24 @@
 
 import java.util.*;
 import net.rptools.clientserver.simple.DisconnectHandler;
-import net.rptools.clientserver.simple.Handshake;
-import net.rptools.clientserver.simple.HandshakeObserver;
 import net.rptools.clientserver.simple.MessageHandler;
 import net.rptools.clientserver.simple.connection.Connection;
 import org.apache.logging.log4j.LogManager;
 import org.apache.logging.log4j.Logger;
 
-public abstract class AbstractServer implements DisconnectHandler, Server, HandshakeObserver {
+public abstract class AbstractServer implements DisconnectHandler, Server {
 
   private static final Logger log = LogManager.getLogger(AbstractServer.class);
-  //    private final ReaperThread reaperThread;
 
   private final Map<String, Connection> clients =
       Collections.synchronizedMap(new HashMap<String, Connection>());
   private final List<ServerObserver> observerList =
       Collections.synchronizedList(new ArrayList<ServerObserver>());
 
-  private final HandshakeProvider handshakeProvider;
+  private final HandshakeProvider<?> handshakeProvider;
   private final MessageHandler messageHandler;
 
-  public AbstractServer(HandshakeProvider handshakeProvider, MessageHandler messageHandler) {
+  public AbstractServer(HandshakeProvider<?> handshakeProvider, MessageHandler messageHandler) {
     this.handshakeProvider = handshakeProvider;
     this.messageHandler = messageHandler;
   }
@@ -130,32 +127,26 @@ public void handleDisconnect(Connection conn) {
 
   protected void handleConnection(Connection conn) {
     var handshake = handshakeProvider.getConnectionHandshake(conn);
-    handshake.addObserver(this);
+    handshake.whenComplete(
+        (result, error) -> {
+          if (error != null) {
+            log.error("Client closing: bad handshake", error);
+            conn.close();
+          } else {
+            conn.addMessageHandler(messageHandler);
+            conn.addDisconnectHandler(this);
+
+            log.debug("About to add new client");
+            synchronized (clients) {
+              reapClients();
+
+              log.debug("Adding new client {}", conn.getId());
+              clients.put(conn.getId(), conn);
+              fireClientConnect(conn);
+            }
+          }
+        });
     // Make sure the client is allowed
     handshake.startHandshake();
   }
-
-  public void onCompleted(Handshake handshake) {
-    handshake.removeObserver(this);
-    var conn = handshake.getConnection();
-    handshakeProvider.releaseHandshake(conn);
-    if (handshake.isSuccessful()) {
-      conn.addMessageHandler(messageHandler);
-      conn.addDisconnectHandler(this);
-
-      log.debug("About to add new client");
-      synchronized (clients) {
-        reapClients();
-
-        log.debug("Adding new client");
-        clients.put(conn.getId(), conn);
-        fireClientConnect(conn);
-        // System.out.println("new client " + conn.getId() + " added, " + server.clients.size()
-        // + " total");
-      }
-    } else {
-      log.debug("Client closing: bad handshake");
-      conn.close();
-    }
-  }
 }

clientserver/src/main/java/net/rptools/clientserver/simple/server/HandshakeProvider.java

@@ -17,8 +17,6 @@
 import net.rptools.clientserver.simple.Handshake;
 import net.rptools.clientserver.simple.connection.Connection;
 
-public interface HandshakeProvider {
-  Handshake getConnectionHandshake(Connection conn);
-
-  void releaseHandshake(Connection conn);
+public interface HandshakeProvider<T> {
+  Handshake<T> getConnectionHandshake(Connection conn);
 }

clientserver/src/main/java/net/rptools/clientserver/simple/server/SocketServer.java

@@ -32,7 +32,7 @@ public class SocketServer extends AbstractServer {
   private ServerSocket socket;
   private ListeningThread listeningThread;
 
-  public SocketServer(int port, HandshakeProvider handshake, MessageHandler messageHandler) {
+  public SocketServer(int port, HandshakeProvider<?> handshake, MessageHandler messageHandler) {
     super(handshake, messageHandler);
     this.port = port;
   }

clientserver/src/main/java/net/rptools/clientserver/simple/server/WebRTCServer.java

@@ -54,7 +54,7 @@ public interface Listener {
 
   public WebRTCServer(
       String serverName,
-      HandshakeProvider handshake,
+      HandshakeProvider<?> handshake,
       MessageHandler messageHandler,
       Listener listener) {
     super(handshake, messageHandler);

src/main/java/net/rptools/clientserver/ConnectionFactory.java

@@ -49,7 +49,7 @@ public void onLoginError() {
   }
 
   public Server createServer(
-      ServerConfig config, HandshakeProvider handshake, MessageHandler messageHandler) {
+      ServerConfig config, HandshakeProvider<?> handshake, MessageHandler messageHandler) {
     if (!config.getUseWebRTC()) {
       return new SocketServer(config.getPort(), handshake, messageHandler);
     }

src/main/java/net/rptools/maptool/client/MapToolClient.java

@@ -33,12 +33,13 @@
 import net.rptools.maptool.model.player.Player;
 import net.rptools.maptool.model.player.PlayerDatabase;
 import net.rptools.maptool.model.player.PlayerDatabaseFactory;
-import net.rptools.maptool.model.player.Players;
 import net.rptools.maptool.server.MapToolServer;
 import net.rptools.maptool.server.PersonalServer;
 import net.rptools.maptool.server.ServerCommand;
 import net.rptools.maptool.server.ServerConfig;
 import net.rptools.maptool.server.ServerPolicy;
+import org.apache.logging.log4j.LogManager;
+import org.apache.logging.log4j.Logger;
 
 /**
  * The client side of a client-server channel.
@@ -48,6 +49,15 @@
  * net.rptools.maptool.client.MapTool} and elsewhere.
  */
 public class MapToolClient {
+  private static final Logger log = LogManager.getLogger(MapToolClient.class);
+
+  public enum State {
+    New,
+    Started,
+    Connected,
+    Closed
+  }
+
   private final LocalPlayer player;
   private final PlayerDatabase playerDatabase;
 
@@ -58,8 +68,8 @@ public class MapToolClient {
   private Campaign campaign;
   private ServerPolicy serverPolicy;
   private final ServerCommand serverCommand;
-
   private boolean disconnectExpected = false;
+  private State currentState = State.New;
 
   private MapToolClient(
       boolean isForLocalServer,
@@ -83,7 +93,9 @@ private MapToolClient(
     this.conn.addDisconnectHandler(conn -> onDisconnect(isForLocalServer, conn));
     this.conn.onCompleted(
         () -> {
-          this.conn.addMessageHandler(new ClientMessageHandler(this));
+          if (transitionToState(State.Started, State.Connected)) {
+            this.conn.addMessageHandler(new ClientMessageHandler(this));
+          }
         });
   }
 
@@ -121,16 +133,60 @@ public MapToolClient(LocalPlayer player, MapToolServer server) {
     this(true, player, server.getPlayerDatabase(), server.getPolicy(), server.getConfig());
   }
 
+  /**
+   * Transition from any state except {@code newState} to {@code newState}.
+   *
+   * @param newState The new state to set.
+   */
+  private boolean transitionToState(State newState) {
+    if (currentState == newState) {
+      log.warn(
+          "Failed to transition to state {} because that is already the current state", newState);
+      return false;
+    } else {
+      currentState = newState;
+      return true;
+    }
+  }
+
+  /**
+   * Transition from {@code expectedState} to {@code newState}.
+   *
+   * @param expectedState The state to transition from
+   * @param newState The new state to set.
+   */
+  private boolean transitionToState(State expectedState, State newState) {
+    if (currentState != expectedState) {
+      log.warn(
+          "Failed to transition from state {} to state {} because the current state is actually {}",
+          expectedState,
+          newState,
+          currentState);
+      return false;
+    } else {
+      currentState = newState;
+      return true;
+    }
+  }
+
+  public State getState() {
+    return currentState;
+  }
+
   public void start() throws IOException {
-    conn.start();
+    if (transitionToState(State.New, State.Started)) {
+      conn.start();
+    }
   }
 
   public void close() throws IOException {
-    if (conn.isAlive()) {
-      conn.close();
-    }
+    if (transitionToState(State.Closed)) {
+      if (conn.isAlive()) {
+        conn.close();
+      }
 
-    playerList.clear();
+      playerList.clear();
+    }
   }
 
   public void expectDisconnection() {
@@ -153,7 +209,7 @@ public void addPlayer(Player player) {
     if (!playerList.contains(player)) {
       playerList.add(player);
       new MapToolEventBus().getMainEventBus().post(new PlayerConnected(player));
-      new Players(playerDatabase).playerSignedIn(player);
+      playerDatabase.playerSignedIn(player);
 
       playerList.sort((arg0, arg1) -> arg0.getName().compareToIgnoreCase(arg1.getName()));
     }
@@ -162,7 +218,7 @@ public void addPlayer(Player player) {
   public void removePlayer(Player player) {
     playerList.remove(player);
     new MapToolEventBus().getMainEventBus().post(new PlayerDisconnected(player));
-    new Players(playerDatabase).playerSignedOut(player);
+    playerDatabase.playerSignedOut(player);
   }
 
   public boolean isPlayerConnected(String playerName) {