From 0e45c4f88630ad211534105e602c84d3a1a2f007 Mon Sep 17 00:00:00 2001 From: Andy Janata Date: Sun, 16 Feb 2014 00:43:13 -0800 Subject: [PATCH] Switch to using a ScheduledThreadPoolExecutor instead of a Timer for scheduled tasks. This will allow 2 * CPU count threads to handle background tasks instead of using a single thread, which could cause the server to effectively die if a task gets stuck for some reason. This addresse the symptom of #89, but not the cause (which is yet to be determined). --- src/net/socialgamer/cah/CahModule.java | 22 ++++++++++++++++--- src/net/socialgamer/cah/SafeTimerTask.java | 4 +--- src/net/socialgamer/cah/StartupUtils.java | 13 ++++++----- src/net/socialgamer/cah/UserPing.java | 6 ++--- src/net/socialgamer/cah/data/Game.java | 21 +++++++++--------- .../socialgamer/cah/data/GameManagerTest.java | 4 ++-- test/net/socialgamer/cah/data/GameTest.java | 4 ++-- 7 files changed, 46 insertions(+), 28 deletions(-) diff --git a/src/net/socialgamer/cah/CahModule.java b/src/net/socialgamer/cah/CahModule.java index ac7e26e..8778e2e 100644 --- a/src/net/socialgamer/cah/CahModule.java +++ b/src/net/socialgamer/cah/CahModule.java @@ -29,7 +29,9 @@ import java.util.Collections; import java.util.HashSet; import java.util.Properties; import java.util.Set; -import java.util.Timer; +import java.util.concurrent.ScheduledThreadPoolExecutor; +import java.util.concurrent.ThreadFactory; +import java.util.concurrent.atomic.AtomicInteger; import net.socialgamer.cah.data.GameManager; import net.socialgamer.cah.data.GameManager.GameId; @@ -65,10 +67,24 @@ public class CahModule extends AbstractModule { }) .annotatedWith(BanList.class) .toInstance(Collections.synchronizedSet(new HashSet())); - bind(Timer.class) - .toInstance(new Timer("timer-task", true)); bind(Properties.class) .toInstance(properties); + + final ScheduledThreadPoolExecutor threadPool = + new ScheduledThreadPoolExecutor(2 * Runtime.getRuntime().availableProcessors(), + new ThreadFactory() { + final AtomicInteger threadCount = new AtomicInteger(); + + @Override + public Thread newThread(final Runnable r) { + final Thread t = new Thread(r); + t.setDaemon(true); + t.setName("timer-task-" + threadCount.incrementAndGet()); + return t; + } + }); + threadPool.setRemoveOnCancelPolicy(true); + bind(ScheduledThreadPoolExecutor.class).toInstance(threadPool); } /** diff --git a/src/net/socialgamer/cah/SafeTimerTask.java b/src/net/socialgamer/cah/SafeTimerTask.java index 6096870..c4c95c5 100644 --- a/src/net/socialgamer/cah/SafeTimerTask.java +++ b/src/net/socialgamer/cah/SafeTimerTask.java @@ -1,11 +1,9 @@ package net.socialgamer.cah; -import java.util.TimerTask; - import org.apache.log4j.Logger; -public abstract class SafeTimerTask extends TimerTask { +public abstract class SafeTimerTask implements Runnable { private static final Logger logger = Logger.getLogger(SafeTimerTask.class); diff --git a/src/net/socialgamer/cah/StartupUtils.java b/src/net/socialgamer/cah/StartupUtils.java index 670de7c..e1d26a6 100644 --- a/src/net/socialgamer/cah/StartupUtils.java +++ b/src/net/socialgamer/cah/StartupUtils.java @@ -27,7 +27,8 @@ import java.io.File; import java.io.FileReader; import java.util.Date; import java.util.Properties; -import java.util.Timer; +import java.util.concurrent.ScheduledThreadPoolExecutor; +import java.util.concurrent.TimeUnit; import javax.servlet.ServletContext; import javax.servlet.ServletContextEvent; @@ -83,8 +84,9 @@ public class StartupUtils extends GuiceServletContextListener { final ServletContext context = contextEvent.getServletContext(); final Injector injector = (Injector) context.getAttribute(INJECTOR); - final Timer timer = injector.getInstance(Timer.class); - timer.cancel(); + final ScheduledThreadPoolExecutor timer = injector + .getInstance(ScheduledThreadPoolExecutor.class); + timer.shutdownNow(); context.removeAttribute(INJECTOR); context.removeAttribute(DATE_NAME); @@ -97,8 +99,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 = injector.getInstance(Timer.class); - timer.schedule(ping, PING_START_DELAY, PING_CHECK_DELAY); + final ScheduledThreadPoolExecutor timer = injector + .getInstance(ScheduledThreadPoolExecutor.class); + timer.scheduleAtFixedRate(ping, PING_START_DELAY, PING_CHECK_DELAY, TimeUnit.MILLISECONDS); serverStarted = new Date(); context.setAttribute(INJECTOR, injector); context.setAttribute(DATE_NAME, serverStarted); diff --git a/src/net/socialgamer/cah/UserPing.java b/src/net/socialgamer/cah/UserPing.java index b2ad8ae..fadb117 100644 --- a/src/net/socialgamer/cah/UserPing.java +++ b/src/net/socialgamer/cah/UserPing.java @@ -23,7 +23,7 @@ package net.socialgamer.cah; -import java.util.Timer; +import java.util.concurrent.ScheduledThreadPoolExecutor; import net.socialgamer.cah.data.ConnectedUsers; @@ -38,10 +38,10 @@ import com.google.inject.Inject; public class UserPing extends SafeTimerTask { private final ConnectedUsers users; - private final Timer globalTimer; + private final ScheduledThreadPoolExecutor globalTimer; @Inject - public UserPing(final ConnectedUsers users, final Timer globalTimer) { + public UserPing(final ConnectedUsers users, final ScheduledThreadPoolExecutor globalTimer) { this.users = users; this.globalTimer = globalTimer; } diff --git a/src/net/socialgamer/cah/data/Game.java b/src/net/socialgamer/cah/data/Game.java index 301e4ae..5a955be 100644 --- a/src/net/socialgamer/cah/data/Game.java +++ b/src/net/socialgamer/cah/data/Game.java @@ -33,7 +33,9 @@ import java.util.LinkedList; import java.util.List; import java.util.Map; import java.util.Set; -import java.util.Timer; +import java.util.concurrent.ScheduledFuture; +import java.util.concurrent.ScheduledThreadPoolExecutor; +import java.util.concurrent.TimeUnit; import javax.annotation.Nonnull; import javax.annotation.Nullable; @@ -146,8 +148,8 @@ public class Game { * Lock to prevent missing timer updates. */ private final Object roundTimerLock = new Object(); - private volatile SafeTimerTask lastTimerTask; - private final Timer globalTimer; + private volatile ScheduledFuture lastScheduledFuture; + private final ScheduledThreadPoolExecutor globalTimer; private int scoreGoal = 8; private final Set cardSets = new HashSet(); @@ -169,7 +171,7 @@ public class Game { */ @Inject public Game(@GameId final Integer id, final ConnectedUsers connectedUsers, - final GameManager gameManager, final Timer globalTimer) { + final GameManager gameManager, final ScheduledThreadPoolExecutor globalTimer) { this.id = id; this.connectedUsers = connectedUsers; this.gameManager = gameManager; @@ -902,10 +904,10 @@ public class Game { private void killRoundTimer() { synchronized (roundTimerLock) { - if (lastTimerTask != null) { - logger.trace(String.format("Killing timer task %s", lastTimerTask)); - lastTimerTask.cancel(); - lastTimerTask = null; + if (null != lastScheduledFuture) { + logger.trace(String.format("Killing timer task %s", lastScheduledFuture)); + lastScheduledFuture.cancel(false); + lastScheduledFuture = null; } } } @@ -914,8 +916,7 @@ public class Game { synchronized (roundTimerLock) { killRoundTimer(); logger.trace(String.format("Scheduling timer task %s after %d ms", task, timeout)); - lastTimerTask = task; - globalTimer.schedule(task, timeout); + lastScheduledFuture = globalTimer.schedule(task, timeout, TimeUnit.MILLISECONDS); } } diff --git a/test/net/socialgamer/cah/data/GameManagerTest.java b/test/net/socialgamer/cah/data/GameManagerTest.java index 57e4102..ba0d373 100644 --- a/test/net/socialgamer/cah/data/GameManagerTest.java +++ b/test/net/socialgamer/cah/data/GameManagerTest.java @@ -35,7 +35,7 @@ import static org.junit.Assert.assertNull; import java.util.Collection; import java.util.HashMap; -import java.util.Timer; +import java.util.concurrent.ScheduledThreadPoolExecutor; import net.socialgamer.cah.HibernateUtil; import net.socialgamer.cah.data.GameManager.GameId; @@ -65,7 +65,7 @@ public class GameManagerTest { private ConnectedUsers cuMock; private User userMock; private int gameId; - private final Timer timer = new Timer("junit-timer", true); + private final ScheduledThreadPoolExecutor timer = new ScheduledThreadPoolExecutor(1); @Before public void setUp() throws Exception { diff --git a/test/net/socialgamer/cah/data/GameTest.java b/test/net/socialgamer/cah/data/GameTest.java index 972a7fe..336d58f 100644 --- a/test/net/socialgamer/cah/data/GameTest.java +++ b/test/net/socialgamer/cah/data/GameTest.java @@ -36,7 +36,7 @@ import static org.junit.Assert.assertTrue; import java.util.Collection; import java.util.HashMap; -import java.util.Timer; +import java.util.concurrent.ScheduledThreadPoolExecutor; import net.socialgamer.cah.data.Game.TooManyPlayersException; import net.socialgamer.cah.data.QueuedMessage.MessageType; @@ -55,7 +55,7 @@ public class GameTest { private Game game; private ConnectedUsers cuMock; private GameManager gmMock; - private final Timer timer = new Timer("junit-timer", true); + private final ScheduledThreadPoolExecutor timer = new ScheduledThreadPoolExecutor(1); @Before public void setUp() throws Exception {