From 82d4f36c1aed6164aca275ddebaaf48674d51a40 Mon Sep 17 00:00:00 2001 From: Jamie Doornbos Date: Mon, 3 Nov 2008 22:20:16 +0000 Subject: [PATCH] Fixes for shutdown sequence, including prevention of a race condition * Throw an exception if any events, runnables or messages are posted after shutdown. These were previosuly being silently ignored. We really should know about them. * Don't exit the connection manager thread until the object manager thread exits. This fixes a race condition where the wakeup time of the conmgr thread would determine if it shutdown before or after the object manager, meaning it would sometimes ignore messages around shutdown time. * Add a shutdown constraint to make the client manager shutdown before the invoker/event/conmgr threads. This gives a single point in the shutdown sequence that applications can use as a constraint (TODO: shutdown groups?) * Emit a log message for each shutdown method call. This could be very useful in production. git-svn-id: svn+ssh://src.earth.threerings.net/narya/trunk@5491 542714f4-19e9-0310-aa3c-eee0fc999fb1 --- .../presents/server/PresentsDObjectMgr.java | 22 ++++++++++++++----- .../presents/server/PresentsServer.java | 11 ++++++++++ .../presents/server/ShutdownManager.java | 1 + .../server/net/ConnectionManager.java | 14 ++++++++++++ 4 files changed, 43 insertions(+), 5 deletions(-) diff --git a/src/java/com/threerings/presents/server/PresentsDObjectMgr.java b/src/java/com/threerings/presents/server/PresentsDObjectMgr.java index ae4a66348..bee73fc73 100644 --- a/src/java/com/threerings/presents/server/PresentsDObjectMgr.java +++ b/src/java/com/threerings/presents/server/PresentsDObjectMgr.java @@ -201,6 +201,10 @@ public class PresentsDObjectMgr // from interface DObjectManager public void postEvent (DEvent event) { + if (!_running) { + throw new IllegalStateException("Object manager has been shutdown"); + } + // assign the event's id and append it to the queue event.eventId = getNextEventId(true); _evqueue.append(event); @@ -273,6 +277,10 @@ public class PresentsDObjectMgr */ public void postRunnable (Runnable unit) { + if (!_running) { + throw new IllegalStateException("Object manager has been shutdown"); + } + // just append it to the queue _evqueue.append(unit); } @@ -563,6 +571,15 @@ public class PresentsDObjectMgr } } + /** + * Tests if the event processing thread is still running. This is required by the + * {@link ConnectionManager} to ensure messages posted just before or during shutdown are sent. + */ + public synchronized boolean isRunning () + { + return _running; + } + /** * Processes a single unit from the queue. */ @@ -790,11 +807,6 @@ public class PresentsDObjectMgr } } - protected synchronized boolean isRunning () - { - return _running; - } - protected int getNextOid () { // look for the next unused oid. in theory if we had two billion objects, this would loop diff --git a/src/java/com/threerings/presents/server/PresentsServer.java b/src/java/com/threerings/presents/server/PresentsServer.java index d1f65b5d1..a2f535fe2 100644 --- a/src/java/com/threerings/presents/server/PresentsServer.java +++ b/src/java/com/threerings/presents/server/PresentsServer.java @@ -158,6 +158,17 @@ public class PresentsServer // initialize the time base services TimeBaseProvider.init(_invmgr, _omgr); + + // Make the client manager shut down before the invoker and dobj threads. This will help + // application code to avoid long chains of shutdown constraints (e.g. msoy bureau manager). + // TODO: is this check needed? + if (_invoker instanceof PresentsInvoker) { + _shutmgr.addConstraint( + _clmgr, ShutdownManager.Constraint.RUNS_BEFORE, (PresentsInvoker)_invoker); + + } else { + log.warning("Invoker is not a PresentsInvoker", "invoker", _invoker); + } } /** diff --git a/src/java/com/threerings/presents/server/ShutdownManager.java b/src/java/com/threerings/presents/server/ShutdownManager.java index db481c3b6..d968400c8 100644 --- a/src/java/com/threerings/presents/server/ShutdownManager.java +++ b/src/java/com/threerings/presents/server/ShutdownManager.java @@ -127,6 +127,7 @@ public class ShutdownManager // shut down all shutdown participants downers.apply(new ObserverList.ObserverOp() { public boolean apply (Shutdowner downer) { + log.info("Calling shutdown registrant", "downer", downer); downer.shutdown(); return true; } diff --git a/src/java/com/threerings/presents/server/net/ConnectionManager.java b/src/java/com/threerings/presents/server/net/ConnectionManager.java index 50c70335b..f3bb18ca2 100644 --- a/src/java/com/threerings/presents/server/net/ConnectionManager.java +++ b/src/java/com/threerings/presents/server/net/ConnectionManager.java @@ -255,6 +255,13 @@ public class ConnectionManager extends LoopingThread report.append(avgOut).append(" avg size, "); report.append(bytesOut*1000/sinceLast).append(" bps\n"); } + + @Override // from LoopingThread + public boolean isRunning () + { + // Prevent exiting our thread until the object manager is done. + return super.isRunning() || _omgr.isRunning(); + } /** * Notifies the connection observers of a connection event. Used internally. @@ -722,6 +729,9 @@ public class ConnectionManager extends LoopingThread sendOutgoingMessages(System.currentTimeMillis()); // unbind our listening socket + // Note: because we wait for the object manager to exit before we do, we will still be + // accepting connections as long as there are events pending. + // TODO: consider shutting down the listen socker earlier, like in the shutdown method try { _ssocket.socket().close(); } catch (IOException ioe) { @@ -846,6 +856,10 @@ public class ConnectionManager extends LoopingThread */ void postMessage (Connection conn, DownstreamMessage msg) { + if (!isRunning()) { + throw new IllegalStateException("Connection manager has been shutdown"); + } + // sanity check if (conn == null || msg == null) { log.warning("postMessage() bogosity", "conn", conn, "msg", msg, new Exception());