Merge pull request #5053 from kwvanderlinde/bugfix/5052-ServerCommand…

…-memory-leak

Add a start/stop lifecycle for ServerCommandClientImpl

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

@@ -68,7 +68,7 @@ public enum State {
   private final MapToolConnection conn;
   private Campaign campaign;
   private ServerPolicy serverPolicy;
-  private final ServerCommand serverCommand;
+  private final ServerCommandClientImpl serverCommand;
   private State currentState = State.New;
 
   private MapToolClient(
@@ -101,6 +101,7 @@ private MapToolClient(
           }
 
           if (transitionToState(State.Started, State.Connected)) {
+            serverCommand.start();
             this.conn.addMessageHandler(new ClientMessageHandler(this));
           }
         });
@@ -181,13 +182,17 @@ public void start() throws IOException {
         // Make sure we're in a reasonable state before propagating.
         log.error("Failed to start client", e);
         transitionToState(State.Closed);
+        // Just in case the exception was somehow late in the handshake and we already started the
+        // server command.
+        serverCommand.stop();
         throw e;
       }
     }
   }
 
   public void close() {
     if (transitionToState(State.Closed)) {
+      serverCommand.stop();
       if (conn.isAlive()) {
         conn.close();
       }

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

@@ -21,6 +21,7 @@
 import java.util.Arrays;
 import java.util.List;
 import java.util.Set;
+import java.util.concurrent.atomic.AtomicBoolean;
 import java.util.stream.Collectors;
 import javax.annotation.Nullable;
 import net.rptools.lib.MD5Key;
@@ -58,7 +59,18 @@ public class ServerCommandClientImpl implements ServerCommand {
 
   public ServerCommandClientImpl(MapToolClient client) {
     this.client = client;
-    movementUpdateQueue.start();
+  }
+
+  public void start() {
+    try {
+      movementUpdateQueue.start();
+    } catch (IllegalThreadStateException e) {
+      log.error("ServerCommand was already started", e);
+    }
+  }
+
+  public void stop() {
+    movementUpdateQueue.stopRunning();
   }
 
   public void heartbeat(String data) {
@@ -812,23 +824,30 @@ public void updatePlayerStatus(Player player) {
    * this way, only the most current version of the event is released.
    */
   private class TimedEventQueue extends Thread {
-
-    Message msg;
-    long delay;
-
-    final Object sleepSemaphore = new Object();
+    private final AtomicBoolean done = new AtomicBoolean(false);
+    private final long delay;
+    private Message msg;
 
     public TimedEventQueue(long millidelay) {
       setName("ServerCommandClientImpl.TimedEventQueue");
       delay = millidelay;
     }
 
+    public void stopRunning() {
+      done.set(true);
+      try {
+        interrupt();
+        join();
+      } catch (InterruptedException e) {
+        log.error("Interrupted thread join. Thread may not be done running.", e);
+      }
+    }
+
     public void enqueue(Message message) {
       msg = message;
     }
 
     public synchronized void flush() {
-
       if (msg != null) {
         makeServerCall(msg);
         msg = null;
@@ -837,16 +856,12 @@ public synchronized void flush() {
 
     @Override
     public void run() {
-
-      while (true) {
-
+      while (!done.get()) {
         flush();
-        synchronized (sleepSemaphore) {
-          try {
-            Thread.sleep(delay);
-          } catch (InterruptedException ie) {
-            // nothing to do
-          }
+        try {
+          Thread.sleep(delay);
+        } catch (InterruptedException ie) {
+          // nothing to do
         }
       }
     }

src/main/java/net/rptools/maptool/server/MapToolServer.java

@@ -20,6 +20,7 @@
 import java.util.Map;
 import java.util.Map.Entry;
 import java.util.Random;
+import java.util.concurrent.atomic.AtomicBoolean;
 import javax.annotation.Nullable;
 import javax.swing.SwingUtilities;
 import net.rptools.clientserver.ConnectionFactory;
@@ -536,15 +537,15 @@ public void shutdown() {
   ////
   // CLASSES
   private class AssetProducerThread extends Thread {
-    private boolean stop = false;
+    private final AtomicBoolean stop = new AtomicBoolean(false);
 
     public AssetProducerThread() {
       setName("AssetProducerThread");
     }
 
     @Override
     public void run() {
-      while (!stop) {
+      while (!stop.get()) {
         Entry<String, AssetTransferManager> entryForException = null;
         try {
           boolean lookForMore = false;
@@ -575,7 +576,7 @@ public void run() {
     }
 
     public void shutdown() {
-      stop = true;
+      stop.set(true);
     }
   }
 }