From b6eb4e1257908bdb29065bfb64b4fdff2a1c62a7 Mon Sep 17 00:00:00 2001 From: Kenneth VanderLinde Date: Mon, 18 Nov 2024 14:41:10 -0800 Subject: [PATCH 1/3] Enforce a start/stop lifecycle on ServerCommandClientImpl --- .../rptools/maptool/client/MapToolClient.java | 7 +++- .../client/ServerCommandClientImpl.java | 37 +++++++++++++++---- 2 files changed, 35 insertions(+), 9 deletions(-) diff --git a/src/main/java/net/rptools/maptool/client/MapToolClient.java b/src/main/java/net/rptools/maptool/client/MapToolClient.java index 4540c9b5d4..4b56507d83 100644 --- a/src/main/java/net/rptools/maptool/client/MapToolClient.java +++ b/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,6 +182,9 @@ 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; } } @@ -188,6 +192,7 @@ public void start() throws IOException { public void close() { if (transitionToState(State.Closed)) { + serverCommand.stop(); if (conn.isAlive()) { conn.close(); } diff --git a/src/main/java/net/rptools/maptool/client/ServerCommandClientImpl.java b/src/main/java/net/rptools/maptool/client/ServerCommandClientImpl.java index bdcf998b22..598616447c 100644 --- a/src/main/java/net/rptools/maptool/client/ServerCommandClientImpl.java +++ b/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) { @@ -803,10 +815,12 @@ public void updatePlayerStatus(Player player) { * this way, only the most current version of the event is released. */ private class TimedEventQueue extends Thread { + private final AtomicBoolean done = new AtomicBoolean(false); + private final long delay; + private Message msg; - Message msg; - long delay; - + // TODO Remove the semaphore. The thread only runs once, so it has no reason to synchronize with + // itself. final Object sleepSemaphore = new Object(); public TimedEventQueue(long millidelay) { @@ -814,12 +828,21 @@ public TimedEventQueue(long millidelay) { 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; @@ -828,9 +851,7 @@ public synchronized void flush() { @Override public void run() { - - while (true) { - + while (!done.get()) { flush(); synchronized (sleepSemaphore) { try { From aebe225f9c6482fef7a22e3df905a31a4b1acba9 Mon Sep 17 00:00:00 2001 From: Kenneth VanderLinde Date: Mon, 18 Nov 2024 14:42:08 -0800 Subject: [PATCH 2/3] Remove lock that does nothing --- .../maptool/client/ServerCommandClientImpl.java | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-) diff --git a/src/main/java/net/rptools/maptool/client/ServerCommandClientImpl.java b/src/main/java/net/rptools/maptool/client/ServerCommandClientImpl.java index 598616447c..f5a56159d2 100644 --- a/src/main/java/net/rptools/maptool/client/ServerCommandClientImpl.java +++ b/src/main/java/net/rptools/maptool/client/ServerCommandClientImpl.java @@ -819,10 +819,6 @@ private class TimedEventQueue extends Thread { private final long delay; private Message msg; - // TODO Remove the semaphore. The thread only runs once, so it has no reason to synchronize with - // itself. - final Object sleepSemaphore = new Object(); - public TimedEventQueue(long millidelay) { setName("ServerCommandClientImpl.TimedEventQueue"); delay = millidelay; @@ -853,12 +849,10 @@ public synchronized void flush() { public void run() { 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 } } } From 9ee4cb9c9270a44ece7dbc3ff1cdde14d4f07059 Mon Sep 17 00:00:00 2001 From: Kenneth VanderLinde Date: Mon, 18 Nov 2024 15:07:03 -0800 Subject: [PATCH 3/3] Fix concurrency issue with AssetProducerThread.shutdown() --- .../java/net/rptools/maptool/server/MapToolServer.java | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/main/java/net/rptools/maptool/server/MapToolServer.java b/src/main/java/net/rptools/maptool/server/MapToolServer.java index c0ed28dc3a..68925dda05 100644 --- a/src/main/java/net/rptools/maptool/server/MapToolServer.java +++ b/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,7 +537,7 @@ public void shutdown() { //// // CLASSES private class AssetProducerThread extends Thread { - private boolean stop = false; + private final AtomicBoolean stop = new AtomicBoolean(false); public AssetProducerThread() { setName("AssetProducerThread"); @@ -544,7 +545,7 @@ public AssetProducerThread() { @Override public void run() { - while (!stop) { + while (!stop.get()) { Entry entryForException = null; try { boolean lookForMore = false; @@ -575,7 +576,7 @@ public void run() { } public void shutdown() { - stop = true; + stop.set(true); } } }