Clean shutdown of StandardGame

Hello,



I have not been very active in the JME community, but still i have been working with it a bit and can look back to 12 years of professional Java experience. I want to suggest a slight modification of StandardGame going together with a couple of new classes.



The problem has come up a few times. And i have have described it here.


svenpohl said:

As discussed in other threads before, there occur problems with the shutdown of StandardGame in a threaded environment. There are always at least the main thread and the OpenGL thread, so this should apply for nearly everybody.

The relevant threads:
* Program exit?
* How to cleanly shut down StandardGame ?
* Threading and StandardGame

The problem typically occurs when the OpenGL thread closes (f.e. because the user clicked the X of the window) but other threads still run and use GameTaskQueueManager with a .get() on the returned Future object. Try closing your games window when your main thread still set's up your GameStates ;)

For cases where no cleaning up of the setup thread is required, there is a simpler solution that has been proposed by Momoko_Fan:


    final StandardGame game = new StandardGame("TestLock");
    game.start();
   
    // setup all the game states in daemon thread, to shutdown when OpenGL thread shuts
    Thread setupThread = new Thread(new SetupProcess(game));
    setupThread.setDaemon(true);
    setupThread.start();



But if any running thread wants to clean up before dying...
...


I created a pretty ugly workaround then I tried to cleanly integrate my work into JME.

The idea is to:
1) make sure the GameTask.get calls will be interrupted when the game quits
2) enable clients of GameShutdownManager to be informend when game shuts down (topically some threads)
3) enable the developer to set a final shutdown task, that will execute after shutdown of StandardGame is complete

My code so far contains
* a new class com.jme.util.GameShutdownManager,
* modifications to the GameTask, GameTaskQueue and GameTaskQueueManager classes, and
* a modification to the cleanup method of StandardGame.

The code follows below. I use the latest SVN version.

EDIT: I created a diff patch for all. Should make things easier.

Index: D:/Development/workspace/jme/src/com/jmex/game/StandardGame.java
===================================================================
--- D:/Development/workspace/jme/src/com/jmex/game/StandardGame.java   (revision 4289)
+++ D:/Development/workspace/jme/src/com/jmex/game/StandardGame.java   (working copy)
@@ -57,6 +57,7 @@
 import com.jme.system.PreferencesGameSettings;
 import com.jme.system.dummy.DummySystemProvider;
 import com.jme.system.jogl.JOGLSystemProvider;
+import com.jme.util.GameShutdownManager;
 import com.jme.util.GameTaskQueue;
 import com.jme.util.GameTaskQueueManager;
 import com.jme.util.NanoTimer;
@@ -434,6 +435,8 @@
    }
 
     protected void cleanup() {
+        GameShutdownManager.getManager().shutdownStarted();
+        GameTaskQueueManager.getManager().setEnabled(false);
         GameStateManager.getInstance().cleanup();
        
         DisplaySystem.getDisplaySystem().getRenderer().cleanup();
@@ -444,6 +447,7 @@
         if (AudioSystem.isCreated()) {
             AudioSystem.getSystem().cleanup();
         }
+        GameShutdownManager.getManager().shutdownComplete();
     }
 
     protected void quit() {
Index: D:/Development/workspace/jme/src/com/jme/util/GameShutdownManager.java
===================================================================
--- D:/Development/workspace/jme/src/com/jme/util/GameShutdownManager.java   (revision 0)
+++ D:/Development/workspace/jme/src/com/jme/util/GameShutdownManager.java   (revision 0)
@@ -0,0 +1,67 @@
+package com.jme.util;
+
+import java.util.Iterator;
+import java.util.concurrent.ConcurrentLinkedQueue;
+import java.util.concurrent.atomic.AtomicReference;
+import java.util.logging.Logger;
+
+/**
+ * This class manages tasks that are executed when <code>StandardGame</code>
+ * shuts down. At least the main thread which can be considered the
+ * initialization thread of a game could still run if the OpenGL
+ * thread stops. It is recommended to add a <code>ShutdownListener</code> to
+ * the <code>GameShutdownManager</code> for every game that uses the
+ * <code>GameTaskQueueManager</code> and calls a <code>get</code> method on
+ * the returned <code>Future</code> objects.
+ * <p>
+ * Please note, that an internal game state is created and attached to the
+ * <code>GameStateManager</code> if listeners are added. Also note that this
+ * class is thread safe.
+ *
+ * @author Sven Pohl
+ * @version $Revision$, $Date$
+ */
+public class GameShutdownManager {
+    private static final Logger logger = Logger
+            .getLogger(GameShutdownManager.class.getName());
+
+    private static final GameShutdownManager INSTANCE = new GameShutdownManager();
+
+    private ConcurrentLinkedQueue<Runnable> queue = new ConcurrentLinkedQueue<Runnable>();
+    private AtomicReference<Runnable> finalTask = new AtomicReference<Runnable>();
+
+    private GameShutdownManager() {
+    }
+
+    public static GameShutdownManager getManager() {
+        return INSTANCE;
+    }
+
+    public void addTask(Runnable task) {
+        queue.add(task);
+    }
+
+    public void removeTask(Runnable task) {
+        queue.remove(task);
+    }
+
+    public void setFinalTask(Runnable task) {
+        finalTask.set(task);
+    }
+
+    public void shutdownStarted() {
+        logger.info("shutdownStarted");
+        for (Iterator<Runnable> i = queue.iterator(); i.hasNext();) {
+            Runnable listener = i.next();
+            listener.run();
+          }
+    }
+
+    public void shutdownComplete() {
+        logger.info("shutdownComplete");
+        Runnable task = finalTask.get();
+        if (task != null)
+            finalTask.get().run();
+    }
+
+}
Index: D:/Development/workspace/jme/src/com/jme/util/GameTaskQueue.java
===================================================================
--- D:/Development/workspace/jme/src/com/jme/util/GameTaskQueue.java   (revision 4289)
+++ D:/Development/workspace/jme/src/com/jme/util/GameTaskQueue.java   (working copy)
@@ -54,6 +54,7 @@
    
     private final ConcurrentLinkedQueue<GameTask<?>> queue = new ConcurrentLinkedQueue<GameTask<?>>();
     private final AtomicBoolean executeAll = new AtomicBoolean();
+    private final AtomicBoolean enabled = new AtomicBoolean(true);
    
     /**
      * The state of this <code>GameTaskQueue</code> if it
@@ -90,7 +91,7 @@
      * @return
      */
     public <V> Future<V> enqueue(Callable<V> callable) {
-        GameTask<V> task = new GameTask<V>(callable);
+        GameTask<V> task = new GameTask<V>(callable, enabled);
         queue.add(task);
         return task;
     }
@@ -111,4 +112,34 @@
             task.invoke();
         } while ((executeAll.get()) && ((task = queue.poll()) != null));
     }
+   
+    /**
+     * This method cancels all queued tasks and prevents any new added tasks from
+     * being blocked.
+     * Call this method to assure clean shutdown of StandardGame.
+     * Otherwise waiting threads are locked since the game thread has shut down.
+     * An exception may be a transition from one StandardGame to another.
+     */
+    public void setEnabled(boolean value) {
+        enabled.set(value);
+        if (value) return;
+       
+        // cancel all tasks if not enabled
+        GameTask<?> task = queue.poll();
+        do {
+            if (task == null)
+                return;
+            while (task.isCancelled()) {
+                task = queue.poll();
+                if (task == null)
+                    return;
+            }
+            task.cancel(true);
+        } while ((executeAll.get()) && ((task = queue.poll()) != null));
+    }
+   
+    public boolean isEnabled() {
+        return enabled.get();
+    }
+   
 }
Index: D:/Development/workspace/jme/src/com/jme/util/GameTask.java
===================================================================
--- D:/Development/workspace/jme/src/com/jme/util/GameTask.java   (revision 4289)
+++ D:/Development/workspace/jme/src/com/jme/util/GameTask.java   (working copy)
@@ -36,6 +36,7 @@
 import java.util.concurrent.Future;
 import java.util.concurrent.TimeUnit;
 import java.util.concurrent.TimeoutException;
+import java.util.concurrent.atomic.AtomicBoolean;
 import java.util.concurrent.locks.Condition;
 import java.util.concurrent.locks.ReentrantLock;
 import java.util.logging.Level;
@@ -59,8 +60,11 @@
     private final ReentrantLock stateLock = new ReentrantLock();
     private final Condition finishedCondition = stateLock.newCondition();
    
-    public GameTask(Callable<V> callable) {
+    private AtomicBoolean enabled;
+   
+    public GameTask(Callable<V> callable, AtomicBoolean enabled) {
         this.callable = callable;
+        this.enabled = enabled;
     }
    
     public boolean cancel(boolean mayInterruptIfRunning) {
@@ -83,6 +87,9 @@
     public V get() throws InterruptedException, ExecutionException {
        stateLock.lock();
        try {
+           if (!enabled.get())
+               throw new InterruptedException("task queue disabled");
+          
           while (!isDone()) {
              finishedCondition.await();
           }
Index: D:/Development/workspace/jme/src/com/jme/util/GameTaskQueueManager.java
===================================================================
--- D:/Development/workspace/jme/src/com/jme/util/GameTaskQueueManager.java   (revision 4289)
+++ D:/Development/workspace/jme/src/com/jme/util/GameTaskQueueManager.java   (working copy)
@@ -32,6 +32,7 @@
 
 package com.jme.util;
 
+import java.util.Iterator;
 import java.util.concurrent.Callable;
 import java.util.concurrent.ConcurrentHashMap;
 import java.util.concurrent.ConcurrentMap;
@@ -92,4 +93,20 @@
     public <V> Future<V> render(Callable<V> callable) {
         return getQueue(GameTaskQueue.RENDER).enqueue(callable);
     }
+   
+    /**
+     * This method cancels all queued tasks and prevents any new added tasks from
+     * being blocked.
+     * Call this method to assure clean shutdown of StandardGame.
+     * Otherwise waiting threads are locked since the game thread has shut down.
+     * An exception may be a transition from one StandardGame to another.
+     */
+    public void setEnabled(boolean value) {
+        Iterator<GameTaskQueue> i = managedQueues.values().iterator();
+        while (i.hasNext()) {
+            GameTaskQueue queue = i.next();
+            queue.setEnabled(value);
+        }
+    }
+
 }



StandardGame:

I added 3 lines of code to the cleanup method, which is called only to shutdown the game.


    protected void cleanup() {
        // added these two lines
        GameShutdownManager.getManager().shutdownStarted();
        GameTaskQueueManager.getManager().setEnabled(false);
       
        GameStateManager.getInstance().cleanup();
        DisplaySystem.getDisplaySystem().getRenderer().cleanup();
        TextureManager.doTextureCleanup();
        TextureManager.clearCache();
       
        JoystickInput.destroyIfInitalized();
        if (AudioSystem.isCreated()) {
            AudioSystem.getSystem().cleanup();
        }

        // and added this one
        GameShutdownManager.getManager().shutdownComplete();
    }



Comments on the modification:

1) GameShutdownManager.getManager().shutdownStarted();
The new class GameShutdownManager is called in the beginning of the startup process to enable user code that are not game states to shutdown cleanly. Typically these are threads.
Alternative: without this it is possible to register a game state instead that does not do anything in update and render, but only in cleanup. Little drawback of that alternative: it would be called every frame without any need.

2) GameTaskQueueManager.getManager().setEnabled(false);
The new method setEnabled of GameTaskQueueManager is called to interrupt all waiting threads and make sure later calls to GameTask.get() are interrupted, too. Warning: this modification forces users who start another StandardGame after shutdown, to modify there code. They must add a setEnabled(true) call, but this really is not standard scenario.
Alternative: register a game state as described in 1)

3) GameShutdownManager.getManager().shutdownComplete();
After shutdown has been done, this enables clients to do something after shutdown is complete. I don't see any drawbacks of this feature, and it is very simple and makes things very easy that are difficult without.
Alternative: implement this in StandardGame instead of calling GameShutdownManager


GameShutdownManager:

package com.jme.util;

import java.util.Iterator;
import java.util.concurrent.ConcurrentLinkedQueue;
import java.util.concurrent.atomic.AtomicReference;
import java.util.logging.Logger;

/**
 * This class manages tasks that are executed when <code>StandardGame</code>
 * shuts down. At least the main thread which can be considered the
 * initialization thread of a game could still run if the OpenGL
 * thread stops. It is recommended to add a <code>ShutdownListener</code> to
 * the <code>GameShutdownManager</code> for every game that uses the
 * <code>GameTaskQueueManager</code> and calls a <code>get</code> method on
 * the returned <code>Future</code> objects.
 * <p>
 * Please note, that an internal game state is created and attached to the
 * <code>GameStateManager</code> if listeners are added. Also note that this
 * class is thread safe.
 *
 * @author Sven Pohl
 * @version $Revision$, $Date$
 */
public class GameShutdownManager {
    private static final Logger logger = Logger
            .getLogger(GameShutdownManager.class.getName());

    private static final GameShutdownManager INSTANCE = new GameShutdownManager();

    private ConcurrentLinkedQueue<Runnable> queue = new ConcurrentLinkedQueue<Runnable>();
    private AtomicReference<Runnable> finalTask = new AtomicReference<Runnable>();

    private GameShutdownManager() {
    }

    public static GameShutdownManager getManager() {
        return INSTANCE;
    }

    public void addTask(Runnable task) {
        queue.add(task);
    }

    public void removeTask(Runnable task) {
        queue.remove(task);
    }

    public void setFinalTask(Runnable task) {
        finalTask.set(task);
    }

    public void shutdownStarted() {
        logger.info("shutdownStarted");
        for (Iterator<Runnable> i = queue.iterator(); i.hasNext();) {
            Runnable listener = i.next();
            listener.run();
          }
    }

    public void shutdownComplete() {
        logger.info("shutdownComplete");
        Runnable task = finalTask.get();
        if (task != null)
            finalTask.get().run();
    }

}



Now we are coming to the GameTaskQueueManager, GameTaskQueue and GameTask classes.
The new setEnabled method connects to the cancel method that already existed in GameTask class. This interrupts all waiting threads. Before that an enabled flag is set on each queue to prevent later calls to GameTask.get from locking. This mechanism is also applicable for the supported custom game tasks that are not UPDATE or RENDER.

GameTaskQueueManager:


    /**
     * This method cancels all queued tasks and prevents any new added tasks from
     * being blocked.
     * Call this method to assure clean shutdown of StandardGame.
     * Otherwise waiting threads are locked since the game thread has shut down.
     * An exception may be a transition from one StandardGame to another.
     */
    public void setEnabled(boolean value) {
        Iterator<GameTaskQueue> i = managedQueues.values().iterator();
        while (i.hasNext()) {
            GameTaskQueue queue = i.next();
            queue.setEnabled(value);
        }
    }



GameTaskQueue:


    private final AtomicBoolean enabled = new AtomicBoolean(true);

    /**
     * This method cancels all queued tasks and prevents any new added tasks from
     * being blocked.
     * Call this method to assure clean shutdown of StandardGame.
     * Otherwise waiting threads are locked since the game thread has shut down.
     * An exception may be a transition from one StandardGame to another.
     */
    public void setEnabled(boolean value) {
        enabled.set(value);
        if (value) return;
       
        // cancel all tasks if not enabled
        GameTask<?> task = queue.poll();
        do {
            if (task == null)
                return;
            while (task.isCancelled()) {
                task = queue.poll();
                if (task == null)
                    return;
            }
            task.cancel(true);
        } while ((executeAll.get()) && ((task = queue.poll()) != null));
    }
   
    public boolean isEnabled() {
        return enabled.get();
    }



GameTask:


    // added
    private AtomicBoolean enabled;

    // modified constructor to save the enabled reference
    public GameTask(Callable<V> callable, AtomicBoolean enabled) {
        this.callable = callable;
        this.enabled = enabled;
    }

   // modified to check enabled flag
   public V get() throws InterruptedException, ExecutionException {
       stateLock.lock();
       try {
            // ADDED these two lines
           if (!enabled.get())
               throw new InterruptedException("task queue disabled");
          
          while (!isDone()) {
             finishedCondition.await();
          }
          if (exception != null) {
             throw exception;
          }
          return result;
       } finally {
          stateLock.unlock();
       }
    }



Since I never got an answer to my previous posts (a year ago ;) ) I would appreciate an answer very much.

Thanks for the great engine! Don't think I only argue as a newbie. I have been working with it quite a bit now even since I have not been very active in this forum.

well, i myself are using StandardGame extensively and the following works for me every time:


    synchronized (semaphore) {
      if (standardGame.isStarted()) {
        logger.debug("Shutting down the display");
        standardGame.shutdown();
      } // if
    } // synchronized



as you can see i am synchronizing to make sure nothing else happens with the standardGame AND GameTaskQueueManager at the same time and just call shutdown().

Where exactly do you use that code?



I do not see how this addresses the problem I descride because all that shutdown does is: it sets a boolean variable to true. So this could prevent the boolean from being to true twice concurrently, which is not really a problem except that it is not volatile 8)



Lets say another thread decides to start some work shortly after your synchronized block (or even just before) and calls GameTaskQueueManager and waits for it's future which will never be coming, since shutdown is initialized. Also I do not see how this helps when the user clicks the X of the window.



Could you be more explicit about that? Perhaps I missed something :slight_smile:



edit: This code is a test for the existance of the problem. Please offer a solution dealing with this few lines everybody can test.


import java.util.concurrent.Callable;
import java.util.concurrent.ExecutionException;

import com.jme.util.GameTaskQueueManager;
import com.jmex.game.StandardGame;

public class TestStandardGameShutdown {

  public static void main(String[] args) {
    final StandardGame game = new StandardGame("TestLock");
    game.start();

    // shuts down the game 2 seconds later
    // Also try closing the window manually instead.
    new Thread(new Runnable() {

      public void run() {
        try {
          Thread.sleep(2000);
        } catch (InterruptedException e) {
          throw new RuntimeException(e);
        }
        game.shutdown();
      }
    }).start();
   
    // do something, lets say we would instead setup a few complex game states
    long startTime = System.currentTimeMillis();
    while (System.currentTimeMillis() < startTime + 4000) {
   
      // continously call open gl thread for the next 4 seconds
      System.out.println("try to execute code in OpenGL thread...");
      try {
        GameTaskQueueManager.getManager().update(
            new Callable<Void>() {

              public Void call() throws Exception {
                // lets imagine we use a texture renderer or anything
                return null;
              }
            }).get();
      } catch (InterruptedException e) {
        throw new RuntimeException(e);
      } catch (ExecutionException e) {
        throw new RuntimeException(e);
      }
      System.out.println("call successful done!");
    }
  }
 
}

the point of synchronizing is that you do it for every statement or whole blocks that are involved in race conditions. so if you use the GTQM and wait for its Future<> you have to synchronize around it. The problem you describe is a standard deadlock problem, which is unsolvable within jme, so the programmer has to deal with it. you can do that manually (which can get quite ugly) or use Aspects (which i do successfully). Just have a pointcut define which statements are to be synchronized against each other, make an around advice and synchronize the proceed() call.

Okay, now think I get your idea after your code gave me the runaround because it does not pay respect to the fact that isStarted() checks the started flag while shutdown() sets the finished flag. So it can only work with a modification like this:



synchronized (semaphore) {
      if (standardGame.isStarted()) {
        logger.debug("Shutting down the display");
        standardGame.shutdown();
        while (standardGame.isStarted()) {
          Thread.sleep(1); // surround with try/catch
        }
      } // if
    } // synchronized



There are still problems with this. But let's say this would work for now. Then every get() call on a Future of GTQM must be also synchronized and check standardGame.isStarted() first to be sure that they do not run into the dead lock I describe in this thread. I try to get your idea, that's all. Do you understand why it is necessary to wait for the GL thread to really exit so that isStarted() will return false?

So now the remaining problem: with this concept the started flag must not be mofidied without this kind of synchronization. But this happens when you click on the X of the window. No shutdown() method is called but still the main loop exits due to display.isClosing() returning true.

So what ever you do here with synchronization... it does not help to solve that deadlock problem. There is no possibility to solve that problem outside of StandardGame. That's why I suggested this patch. Please do not answer just with general pointers to synchronization and wrong code. My time is expensive, too. I just want to contribute because I like the engine and want it to live. The problem I describe is not "unsolvable within jme" it is unsolvable outside jme expect with dirty hacks. At least as far as I can see.

Why don't just interrupt all waiting threads when the game shuts down? Why don't give the user a possibility to provide code that should be run after the application shutdown? These are real basic features that make jME better.

Edit (May 7): Sorry that I am a little annoyed because I feel very misunderstood. You talk about synchronizing Future.calls. I talk about never answered Future calls since the underlying Future implementation has shut down. These are two different things. This problem is not about synchronization.