- Remove some pointless @Provides methods and .bind().toInstance().

- Use a single global Timer and schedule multiple TimerTasks on it. Moving to a ScheduledThreadPoolExecutor may be preferable as now all TimerTasks are serialized.
- Fix tests that were broken by changed behavior in 638fac780a (shows how often I run the few crappy tests I have...)

src/net/socialgamer/cah/CahModule.java

@@ -29,6 +29,7 @@ import java.util.Collections;
 import java.util.HashSet;
 import java.util.Properties;
 import java.util.Set;
+import java.util.Timer;
 
 import net.socialgamer.cah.data.GameManager;
 import net.socialgamer.cah.data.GameManager.GameId;
@@ -39,6 +40,7 @@ import org.hibernate.Session;
 import com.google.inject.AbstractModule;
 import com.google.inject.BindingAnnotation;
 import com.google.inject.Provides;
+import com.google.inject.TypeLiteral;
 
 
 /**
@@ -48,9 +50,25 @@ import com.google.inject.Provides;
  */
 public class CahModule extends AbstractModule {
 
+  private final static Properties properties = new Properties();
+
   @Override
   protected void configure() {
-    bind(Integer.class).annotatedWith(GameId.class).toProvider(GameManager.class);
+    bind(Integer.class)
+        .annotatedWith(GameId.class)
+        .toProvider(GameManager.class);
+    /*
+     * A mutable Set of IP addresses (in String format) which are banned. This Set is
+     * thread-safe.
+     */
+    bind(new TypeLiteral<Set<String>>() {
+    })
+        .annotatedWith(BanList.class)
+        .toInstance(Collections.synchronizedSet(new HashSet<String>()));
+    bind(Timer.class)
+        .toInstance(new Timer("timer-task", true));
+    bind(Properties.class)
+        .toInstance(properties);
   }
 
   /**
@@ -71,11 +89,6 @@ public class CahModule extends AbstractModule {
     return Integer.valueOf((String) properties.get("pyx.server.max_users"));
   }
 
-  @BindingAnnotation
-  @Retention(RetentionPolicy.RUNTIME)
-  public @interface MaxUsers {
-  }
-
   /**
    * @return A Hibernate session. Objects which receive a Hibernate session should close the
    * session when they are done!
@@ -85,27 +98,13 @@ public class CahModule extends AbstractModule {
     return HibernateUtil.instance.sessionFactory.openSession();
   }
 
-  private final static Set<String> banList = Collections.synchronizedSet(new HashSet<String>());
-
-  /**
-   * @return A mutable Set of IP addresses (in String format) which are banned. This Set is
-   * thread-safe.
-   */
-  @Provides
-  @BanList
-  Set<String> provideBanList() {
-    return banList;
-  }
-
   @BindingAnnotation
   @Retention(RetentionPolicy.RUNTIME)
   public @interface BanList {
   }
 
-  private final static Properties properties = new Properties();
+  @BindingAnnotation
-
+  @Retention(RetentionPolicy.RUNTIME)
-  @Provides
+  public @interface MaxUsers {
-  Properties provideProperties() {
-    return properties;
   }
 }

src/net/socialgamer/cah/StartupUtils.java

@@ -63,11 +63,6 @@ public class StartupUtils extends GuiceServletContextListener {
    */
   private static final long PING_CHECK_DELAY = 5 * 1000;
 
-  /**
-   * Context attribute key name for the disconnected client timer.
-   */
-  private static final String PING_TIMER_NAME = "ping_timer";
-
   /**
    * Context attribute key name for the time the server was started.
    */
@@ -86,10 +81,11 @@ public class StartupUtils extends GuiceServletContextListener {
   @Override
   public void contextDestroyed(final ServletContextEvent contextEvent) {
     final ServletContext context = contextEvent.getServletContext();
-    final Timer timer = (Timer) context.getAttribute(PING_TIMER_NAME);
+
-    assert (timer != null);
+    final Injector injector = (Injector) context.getAttribute(INJECTOR);
+    final Timer timer = injector.getInstance(Timer.class);
     timer.cancel();
-    context.removeAttribute(PING_TIMER_NAME);
+
     context.removeAttribute(INJECTOR);
     context.removeAttribute(DATE_NAME);
 
@@ -101,10 +97,9 @@ public class StartupUtils extends GuiceServletContextListener {
     final ServletContext context = contextEvent.getServletContext();
     final Injector injector = getInjector();
     final UserPing ping = injector.getInstance(UserPing.class);
-    final Timer timer = new Timer("PingCheck", true);
+    final Timer timer = injector.getInstance(Timer.class);
     timer.schedule(ping, PING_START_DELAY, PING_CHECK_DELAY);
     serverStarted = new Date();
-    context.setAttribute(PING_TIMER_NAME, timer);
     context.setAttribute(INJECTOR, injector);
     context.setAttribute(DATE_NAME, serverStarted);
 

src/net/socialgamer/cah/data/Game.java

@@ -134,12 +134,18 @@ public class Game {
    * Lock object to prevent judging during idle judge detection and vice-versa.
    */
   private final Object judgeLock = new Object();
-  private Timer nextRoundTimer;
+
-  private final Object nextRoundTimerLock = new Object();
+  /**
+   * Lock to prevent missing timer updates.
+   */
+  private final Object roundTimerLock = new Object();
+  private volatile TimerTask lastTimerTask;
+  private final Timer globalTimer;
+
   private int scoreGoal = 8;
   private final Set<CardSet> cardSets = new HashSet<CardSet>();
   private String password = "";
-  private boolean useTimer = true;
+  private boolean useIdleTimer = true;
   private final Session hibernateSession;
 
   /**
@@ -152,14 +158,17 @@ public class Game {
    * @param gameManager
    *          The game manager, for broadcasting game list refresh notices and destroying this game
    *          when everybody leaves.
+   * @param hibernateSession Hibernate session from which to load cards.
+   * @param globalTimer The global timer on which to schedule tasks.
    */
   @Inject
   public Game(@GameId final Integer id, final ConnectedUsers connectedUsers,
-      final GameManager gameManager, final Session hibernateSession) {
+      final GameManager gameManager, final Session hibernateSession, final Timer globalTimer) {
     this.id = id;
     this.connectedUsers = connectedUsers;
     this.gameManager = gameManager;
     this.hibernateSession = hibernateSession;
+    this.globalTimer = globalTimer;
     state = GameState.LOBBY;
   }
 
@@ -207,7 +216,7 @@ public class Game {
    * Remove a player from the game.
    * <br/>
    * Synchronizes on {@link #players}, {@link #playedCards}, {@link #whiteDeck}, and
-   * {@link #nextRoundTimerLock}.
+   * {@link #roundTimerLock}.
    * 
    * @param user
    *          Player to remove from the game.
@@ -290,15 +299,14 @@ public class Game {
             id));
         resetState(true);
       } else if (wasJudge) {
-        synchronized (nextRoundTimerLock) {
+        synchronized (roundTimerLock) {
           final TimerTask task = new TimerTask() {
             @Override
             public void run() {
               startNextRound();
             }
           };
-          nextRoundTimer = new Timer("judge-left-" + id, true);
+          rescheduleTimer(task, ROUND_INTERMISSION);
-          nextRoundTimer.schedule(task, ROUND_INTERMISSION);
         }
       }
       return players.size() == 0;
@@ -442,7 +450,7 @@ public class Game {
     }
     this.maxBlanks = newMaxBlanks;
     this.password = newPassword;
-    this.useTimer = newUseTimer;
+    this.useIdleTimer = newUseTimer;
 
     final HashMap<ReturnableData, Object> data = getEventMap();
     data.put(LongPollResponse.EVENT, LongPollEvent.GAME_OPTIONS_CHANGED.toString());
@@ -493,7 +501,7 @@ public class Game {
     info.put(GameInfo.PLAYER_LIMIT, maxPlayers);
     info.put(GameInfo.SPECTATOR_LIMIT, maxSpectators);
     info.put(GameInfo.SCORE_LIMIT, scoreGoal);
-    info.put(GameInfo.USE_TIMER, useTimer);
+    info.put(GameInfo.USE_TIMER, useIdleTimer);
     if (includePassword) {
       info.put(GameInfo.PASSWORD, password);
     }
@@ -684,7 +692,7 @@ public class Game {
    * Move the game into the {@code PLAYING} state, drawing a new Black Card and dispatching a
    * message to all players.
    * <br/>
-   * Synchronizes on {@link #players}, {@link #blackCardLock}, and {@link #nextRoundTimerLock}.
+   * Synchronizes on {@link #players}, {@link #blackCardLock}, and {@link #roundTimerLock}.
    */
   private void playingState() {
     state = GameState.PLAYING;
@@ -716,7 +724,7 @@ public class Game {
     }
 
     // Perhaps figure out a better way to do this...
-    final int playTimer = useTimer ? PLAY_TIMEOUT_BASE
+    final int playTimer = useIdleTimer ? PLAY_TIMEOUT_BASE
         + (PLAY_TIMEOUT_PER_CARD * blackCard.getPick()) : Integer.MAX_VALUE;
 
     final HashMap<ReturnableData, Object> data = getEventMap();
@@ -727,8 +735,7 @@ public class Game {
 
     broadcastToPlayers(MessageType.GAME_EVENT, data);
 
-    synchronized (nextRoundTimerLock) {
+    synchronized (roundTimerLock) {
-      killRoundTimer();
       final TimerTask task = new TimerTask() {
         @Override
         public void run() {
@@ -736,19 +743,18 @@ public class Game {
         }
       };
       // 10 second warning
-      nextRoundTimer = new Timer("hurry-up-" + id, true);
+      rescheduleTimer(task, playTimer - 10 * 1000);
-      nextRoundTimer.schedule(task, playTimer - 10 * 1000);
     }
   }
 
   /**
    * Warn players that have not yet played that they are running out of time to do so.
    * <br/>
-   * Synchronizes on {@link #nextRoundTimerLock} and {@link #roundPlayers}.
+   * Synchronizes on {@link #roundTimerLock} and {@link #roundPlayers}.
    */
   private void warnPlayersToPlay() {
     // have to do this all synchronized in case they play while we're processing this
-    synchronized (nextRoundTimerLock) {
+    synchronized (roundTimerLock) {
       killRoundTimer();
 
       synchronized (roundPlayers) {
@@ -771,14 +777,13 @@ public class Game {
         }
       };
       // 10 seconds to finish playing
-      nextRoundTimer = new Timer("hurry-up-" + id, true);
+      rescheduleTimer(task, 10 * 1000);
-      nextRoundTimer.schedule(task, 10 * 1000);
     }
   }
 
   private void warnJudgeToJudge() {
     // have to do this all synchronized in case they play while we're processing this
-    synchronized (nextRoundTimerLock) {
+    synchronized (roundTimerLock) {
       killRoundTimer();
 
       if (state == GameState.JUDGING) {
@@ -796,8 +801,7 @@ public class Game {
         }
       };
       // 10 seconds to finish playing
-      nextRoundTimer = new Timer("hurry-up-" + id, true);
+      rescheduleTimer(task, 10 * 1000);
-      nextRoundTimer.schedule(task, 10 * 1000);
     }
   }
 
@@ -886,14 +890,24 @@ public class Game {
   }
 
   private void killRoundTimer() {
-    synchronized (nextRoundTimerLock) {
+    synchronized (roundTimerLock) {
-      if (nextRoundTimer != null) {
+      if (lastTimerTask != null) {
-        nextRoundTimer.cancel();
+        logger.trace(String.format("Killing timer task %s", lastTimerTask));
-        nextRoundTimer = null;
+        lastTimerTask.cancel();
+        lastTimerTask = null;
       }
     }
   }
 
+  private void rescheduleTimer(final TimerTask task, final long timeout) {
+    synchronized (roundTimerLock) {
+      killRoundTimer();
+      logger.trace(String.format("Scheduling timer task %s after %d ms", task, timeout));
+      lastTimerTask = task;
+      globalTimer.schedule(task, timeout);
+    }
+  }
+
   /**
    * Move the game into the {@code JUDGING} state.
    */
@@ -902,7 +916,7 @@ public class Game {
     state = GameState.JUDGING;
 
     // Perhaps figure out a better way to do this...
-    final int judgeTimer = useTimer ? JUDGE_TIMEOUT_BASE
+    final int judgeTimer = useIdleTimer ? JUDGE_TIMEOUT_BASE
         + (JUDGE_TIMEOUT_PER_CARD * playedCards.size() * blackCard.getPick()) : Integer.MAX_VALUE;
 
     final HashMap<ReturnableData, Object> data = getEventMap();
@@ -914,8 +928,7 @@ public class Game {
 
     notifyPlayerInfoChange(getJudge());
 
-    synchronized (nextRoundTimerLock) {
+    synchronized (roundTimerLock) {
-      killRoundTimer();
       final TimerTask task = new TimerTask() {
         @Override
         public void run() {
@@ -923,8 +936,7 @@ public class Game {
         }
       };
       // 10 second warning
-      nextRoundTimer = new Timer("hurry-up-" + id, true);
+      rescheduleTimer(task, judgeTimer - 10 * 1000);
-      nextRoundTimer.schedule(task, judgeTimer - 10 * 1000);
     }
   }
 
@@ -1007,12 +1019,7 @@ public class Game {
    * state.
    */
   private void startNextRound() {
-    synchronized (nextRoundTimerLock) {
+    killRoundTimer();
-      if (nextRoundTimer != null) {
-        nextRoundTimer.cancel();
-        nextRoundTimer = null;
-      }
-    }
 
     synchronized (playedCards) {
       for (final List<WhiteCard> cards : playedCards.cards()) {
@@ -1358,8 +1365,7 @@ public class Game {
     notifyPlayerInfoChange(getJudge());
     notifyPlayerInfoChange(cardPlayer);
 
-    synchronized (nextRoundTimerLock) {
+    synchronized (roundTimerLock) {
-      killRoundTimer();
       final TimerTask task;
       // TODO win-by-x option
       if (cardPlayer.getScore() >= scoreGoal) {
@@ -1377,8 +1383,7 @@ public class Game {
           }
         };
       }
-      nextRoundTimer = new Timer("round-intermission-" + id, true);
+      rescheduleTimer(task, ROUND_INTERMISSION);
-      nextRoundTimer.schedule(task, ROUND_INTERMISSION);
     }
 
     return null;

test/net/socialgamer/cah/data/GameManagerTest.java

@@ -35,6 +35,7 @@ import static org.junit.Assert.assertNull;
 
 import java.util.Collection;
 import java.util.HashMap;
+import java.util.Timer;
 
 import net.socialgamer.cah.HibernateUtil;
 import net.socialgamer.cah.data.GameManager.GameId;
@@ -64,6 +65,7 @@ public class GameManagerTest {
   private ConnectedUsers cuMock;
   private User userMock;
   private int gameId;
+  private final Timer timer = new Timer("junit-timer", true);
 
   @Before
   public void setUp() throws Exception {
@@ -116,11 +118,11 @@ public class GameManagerTest {
 
     // fill it up with 3 games
     assertEquals(0, gameManager.get().intValue());
-    gameManager.getGames().put(0, new Game(0, cuMock, gameManager, null));
+    gameManager.getGames().put(0, new Game(0, cuMock, gameManager, null, timer));
     assertEquals(1, gameManager.get().intValue());
-    gameManager.getGames().put(1, new Game(1, cuMock, gameManager, null));
+    gameManager.getGames().put(1, new Game(1, cuMock, gameManager, null, timer));
     assertEquals(2, gameManager.get().intValue());
-    gameManager.getGames().put(2, new Game(2, cuMock, gameManager, null));
+    gameManager.getGames().put(2, new Game(2, cuMock, gameManager, null, timer));
     // make sure it says it can't make any more
     assertEquals(-1, gameManager.get().intValue());
 
@@ -128,13 +130,13 @@ public class GameManagerTest {
     gameManager.destroyGame(1);
     // make sure it re-uses that id
     assertEquals(1, gameManager.get().intValue());
-    gameManager.getGames().put(1, new Game(1, cuMock, gameManager, null));
+    gameManager.getGames().put(1, new Game(1, cuMock, gameManager, null, timer));
     assertEquals(-1, gameManager.get().intValue());
 
     // remove game 1 out from under it, to make sure it'll fix itself
     gameManager.getGames().remove(1);
     assertEquals(1, gameManager.get().intValue());
-    gameManager.getGames().put(1, new Game(1, cuMock, gameManager, null));
+    gameManager.getGames().put(1, new Game(1, cuMock, gameManager, null, timer));
     assertEquals(-1, gameManager.get().intValue());
 
     gameManager.destroyGame(2);
@@ -146,7 +148,7 @@ public class GameManagerTest {
   @Test
   public void testCreateGame() {
     cuMock.broadcastToAll(eq(MessageType.GAME_EVENT), anyObject(HashMap.class));
-    expectLastCall().times(6);
+    expectLastCall().times(3);
     cuMock.broadcastToList(anyObject(Collection.class), eq(MessageType.GAME_PLAYER_EVENT),
         anyObject(HashMap.class));
     expectLastCall().times(3);

test/net/socialgamer/cah/data/GameTest.java

@@ -36,6 +36,7 @@ import static org.junit.Assert.assertTrue;
 
 import java.util.Collection;
 import java.util.HashMap;
+import java.util.Timer;
 
 import net.socialgamer.cah.data.Game.TooManyPlayersException;
 import net.socialgamer.cah.data.QueuedMessage.MessageType;
@@ -54,12 +55,13 @@ public class GameTest {
   private Game game;
   private ConnectedUsers cuMock;
   private GameManager gmMock;
+  private final Timer timer = new Timer("junit-timer", true);
 
   @Before
   public void setUp() throws Exception {
     cuMock = createMock(ConnectedUsers.class);
     gmMock = createMock(GameManager.class);
-    game = new Game(0, cuMock, gmMock, null);
+    game = new Game(0, cuMock, gmMock, null, timer);
   }
 
   @SuppressWarnings("unchecked")
@@ -68,8 +70,6 @@ public class GameTest {
     cuMock.broadcastToList(anyObject(Collection.class), eq(MessageType.GAME_PLAYER_EVENT),
         anyObject(HashMap.class));
     expectLastCall().times(4);
-    gmMock.broadcastGameListRefresh();
-    expectLastCall().times(4);
     replay(cuMock);
     gmMock.destroyGame(anyInt());
     expectLastCall().once();