#2582 Correct thread warden so it can cope with multiple JME roots on multiple threads (#2583)

jme3-core/src/main/java/com/jme3/app/SimpleApplication.java

@@ -307,13 +307,6 @@ public abstract class SimpleApplication extends LegacyApplication {
         simpleInitApp();
     }
 
-    @Override
-    public void stop(boolean waitFor) {
-        //noinspection AssertWithSideEffects
-        assert SceneGraphThreadWarden.reset();
-        super.stop(waitFor);
-    }
-
     @Override
     public void update() {
         if (prof != null) {

jme3-core/src/main/java/com/jme3/scene/threadwarden/SceneGraphThreadWarden.java

@@ -4,7 +4,7 @@ import com.jme3.scene.Node;
 import com.jme3.scene.Spatial;
 
 import java.util.Collections;
-import java.util.Set;
+import java.util.Map;
 import java.util.WeakHashMap;
 import java.util.logging.Level;
 import java.util.logging.Logger;
@@ -17,6 +17,7 @@ import java.util.logging.Logger;
  *     Only has an effect if asserts are on
  * </p>
  */
+@SuppressWarnings("SameReturnValue")
 public class SceneGraphThreadWarden {
 
     private static final Logger logger = Logger.getLogger(SceneGraphThreadWarden.class.getName());
@@ -34,8 +35,7 @@ public class SceneGraphThreadWarden {
         assert ASSERTS_ENABLED = true;
     }
 
-    public static Thread mainThread;
-    public static final Set<Spatial> spatialsThatAreMainThreadReserved = Collections.synchronizedSet(Collections.newSetFromMap(new WeakHashMap<>()));
+    public static final Map<Spatial, Thread> spatialsThatAreMainThreadReserved = Collections.synchronizedMap(new WeakHashMap<>());
 
     /**
      * Marks the given node as being reserved for the main thread.
@@ -48,12 +48,11 @@ public class SceneGraphThreadWarden {
             return true;
         }
         Thread thisThread = Thread.currentThread();
-        if(mainThread != null && mainThread != thisThread ){
-            throw new IllegalStateException("The main thread has already been set to " + mainThread.getName() + " but now it's being set to " + Thread.currentThread().getName());
+        Thread existingRestriction = spatialsThatAreMainThreadReserved.get(rootNode);
+        if(existingRestriction != null && existingRestriction != thisThread ){
+            throw new IllegalStateException("The node is already restricted to " + existingRestriction.getName() + " but now it's being restricted to " + Thread.currentThread().getName());
         }
-        mainThread = thisThread;
-        setTreeRestricted(rootNode);
-
+        setTreeRestricted(rootNode, thisThread);
         return true; // return true so can be a "side effect" of an assert
     }
 
@@ -71,11 +70,11 @@ public class SceneGraphThreadWarden {
      * Runs through the entire tree and sets the restriction state of all nodes below the given node
      * @param spatial the node (and children) to set the restriction state of
      */
-    private static void setTreeRestricted(Spatial spatial){
-        spatialsThatAreMainThreadReserved.add(spatial);
+    private static void setTreeRestricted(Spatial spatial, Thread threadRestriction){
+        spatialsThatAreMainThreadReserved.put(spatial, threadRestriction);
         if(spatial instanceof Node){
             for(Spatial child : ((Node) spatial).getChildren()){
-                setTreeRestricted(child);
+                setTreeRestricted(child, threadRestriction);
             }
         }
     }
@@ -99,8 +98,8 @@ public class SceneGraphThreadWarden {
             return true;
         }
 
-        boolean shouldNowBeRestricted = newParent !=null && spatialsThatAreMainThreadReserved.contains(newParent);
-        boolean wasPreviouslyRestricted = spatialsThatAreMainThreadReserved.contains(spatial);
+        boolean shouldNowBeRestricted = newParent !=null && spatialsThatAreMainThreadReserved.containsKey(newParent);
+        boolean wasPreviouslyRestricted = spatialsThatAreMainThreadReserved.containsKey(spatial);
 
         if(shouldNowBeRestricted || wasPreviouslyRestricted ){
             assertOnCorrectThread(spatial);
@@ -110,7 +109,7 @@ public class SceneGraphThreadWarden {
             return true;
         }
         if(shouldNowBeRestricted){
-            setTreeRestricted(spatial);
+            setTreeRestricted(spatial, Thread.currentThread());
         }else{
             setTreeNotRestricted(spatial);
         }
@@ -118,9 +117,11 @@ public class SceneGraphThreadWarden {
         return true; // return true so can be a "side effect" of an assert
     }
 
+    /**
+     * This is a full reset over all options and all threads
+     */
     public static boolean reset(){
         spatialsThatAreMainThreadReserved.clear();
-        mainThread = null;
         THREAD_WARDEN_ENABLED = !Boolean.getBoolean("nothreadwarden");
         return true; // return true so can be a "side effect" of an assert
     }
@@ -134,8 +135,9 @@ public class SceneGraphThreadWarden {
         if(checksDisabled()){
             return true;
         }
-        if(spatialsThatAreMainThreadReserved.contains(spatial)){
-            if(Thread.currentThread() != mainThread){
+        Thread restrictingThread = spatialsThatAreMainThreadReserved.get(spatial);
+        if(restrictingThread!=null){
+            if(Thread.currentThread() != restrictingThread){
                 // log as well as throw an exception because we are running in a thread, if we are in an executor service the exception
                 // might not make itself known until `get` is called on the future (and JME might crash before that happens).
                 String message = "The spatial " + spatial + " was mutated on a thread other than the main thread, was mutated on " + Thread.currentThread().getName();

jme3-core/src/test/java/com/jme3/scene/threadwarden/SceneGraphThreadWardenTest.java

@@ -26,6 +26,7 @@ import static org.junit.Assert.fail;
 public class SceneGraphThreadWardenTest {
 
     private static ExecutorService executorService;
+    private static ExecutorService executorService2;
 
     @SuppressWarnings({"ReassignedVariable", "AssertWithSideEffects"})
     @BeforeClass
@@ -42,11 +43,13 @@ public class SceneGraphThreadWardenTest {
     @Before
     public void setup() {
         executorService = newSingleThreadDaemonExecutor();
+        executorService2 = newSingleThreadDaemonExecutor();
     }
 
     @After
     public void tearDown() {
         executorService.shutdown();
+        executorService2.shutdown();
         SceneGraphThreadWarden.reset();
     }
 
@@ -71,6 +74,31 @@ public class SceneGraphThreadWardenTest {
         rootNode.detachChild(child);
     }
 
+    @Test
+    public void restrictingTheSameRootNodeToTwoThreadsIsIllegal() throws ExecutionException, InterruptedException {
+        Node rootNode = new Node("root");
+
+        executorService.submit(() -> SceneGraphThreadWarden.setup(rootNode)).get();
+
+        try {
+            executorService2.submit(() -> SceneGraphThreadWarden.setup(rootNode)).get();
+            fail("Expected an IllegalThreadSceneGraphMutation exception");
+        } catch (ExecutionException e) {
+            // This is expected - verify it's the right exception type
+            assertTrue("Expected IllegalStateException, got: " + e.getCause().getClass().getName(),
+                    e.getCause() instanceof IllegalStateException);
+        }
+    }
+
+    @Test
+    public void restrictingTwoRootNodesIsFine(){
+        Node rootNode = new Node("root");
+        Node rootNode2 = new Node("root2");
+
+        SceneGraphThreadWarden.setup(rootNode);
+        SceneGraphThreadWarden.setup(rootNode2);
+    }
+
     /**
      * Test that node mutation on nodes not connected to the root node is fine even on a non main thread.
      * <p>
@@ -107,7 +135,7 @@ public class SceneGraphThreadWardenTest {
      * exception.
      */
     @Test
-    public void testAddingNodeToSceneGraphOnNonMainThread() throws InterruptedException {
+    public void testAddingNodeToSceneGraphOnNonMainThread() {
         Node rootNode = new Node("root");
         SceneGraphThreadWarden.setup(rootNode);
 
@@ -122,14 +150,7 @@ public class SceneGraphThreadWardenTest {
             return null;
         });
 
-        try {
-            illegalMutationFuture.get();
-            fail("Expected an IllegalThreadSceneGraphMutation exception");
-        } catch (ExecutionException e) {
-            // This is expected - verify it's the right exception type
-            assertTrue("Expected IllegalThreadSceneGraphMutation, got: " + e.getCause().getClass().getName(),
-                    e.getCause() instanceof IllegalThreadSceneGraphMutation);
-        }
+        expectSceneGraphException(illegalMutationFuture);
     }
 
     /**
@@ -141,7 +162,7 @@ public class SceneGraphThreadWardenTest {
      * </p>
      */
     @Test
-    public void testMovingNodeAttachedToRootOnNonMainThread() throws InterruptedException {
+    public void testMovingNodeAttachedToRootOnNonMainThread() {
         Node rootNode = new Node("root");
         SceneGraphThreadWarden.setup(rootNode);
 
@@ -157,14 +178,7 @@ public class SceneGraphThreadWardenTest {
             return null;
         });
 
-        try {
-            illegalMutationFuture.get();
-            fail("Expected an IllegalThreadSceneGraphMutation exception");
-        } catch (ExecutionException e) {
-            // This is expected - verify it's the right exception type
-            assertTrue("Expected IllegalThreadSceneGraphMutation, got: " + e.getCause().getClass().getName(),
-                    e.getCause() instanceof IllegalThreadSceneGraphMutation);
-        }
+        expectSceneGraphException(illegalMutationFuture);
     }
 
     /**
@@ -199,7 +213,7 @@ public class SceneGraphThreadWardenTest {
      * then try (and fail) to make an illegal on-thread change to the grandchild.
      */
     @Test
-    public void testAddingAChildToTheRootNodeAlsoRestrictsTheGrandChild() throws InterruptedException {
+    public void testAddingAChildToTheRootNodeAlsoRestrictsTheGrandChild() {
         Node rootNode = new Node("root");
         SceneGraphThreadWarden.setup(rootNode);
 
@@ -221,14 +235,7 @@ public class SceneGraphThreadWardenTest {
             return null;
         });
 
-        try {
-            illegalMutationFuture.get();
-            fail("Expected an IllegalThreadSceneGraphMutation exception");
-        } catch (ExecutionException e) {
-            // This is expected - verify it's the right exception type
-            assertTrue("Expected IllegalThreadSceneGraphMutation, got: " + e.getCause().getClass().getName(),
-                    e.getCause() instanceof IllegalThreadSceneGraphMutation);
-        }
+        expectSceneGraphException(illegalMutationFuture);
     }
 
     /**
@@ -294,7 +301,96 @@ public class SceneGraphThreadWardenTest {
         mutationFuture.get();
     }
 
+    /**
+     * This tests that scenarios where multiple JME applications are running simultaneously within the same
+     * JMV. This is a weird case but does happen sometimes (e.g. in TestChooser).
+     * <p>
+     * This test tests that using two seperate executor services with their own seperate threads
+     * </p>
+     */
+    @Test
+    public void testCanCopeWithMultipleSimultaneousRootThreads() throws ExecutionException, InterruptedException {
+        Node rootNodeThread1 = new Node("root1");
+        Node rootNodeThread2 = new Node("root2");
 
+        // Create a child node and attach it to the root node
+        Node childThread1 = new Node("child1");
+        Node childThread2 = new Node("child2");
+
+        // on two threads set up two root nodes (for two JME applications on the same VM)
+        Future<Void> mutationFuture1 = executorService.submit(() -> {
+            SceneGraphThreadWarden.setup(rootNodeThread1);
+            rootNodeThread1.attachChild(childThread1);
+            return null;
+        });
+
+        Future<Void> mutationFuture2 = executorService2.submit(() -> {
+            SceneGraphThreadWarden.setup(rootNodeThread2);
+            rootNodeThread2.attachChild(childThread2);
+            return null;
+        });
+
+        // These should complete without exceptions, these are independent scene graphs
+        mutationFuture1.get();
+        mutationFuture2.get();
+
+        // try to use a child from one thread on the other, this should exception as the child is reserved
+        expectSceneGraphException(executorService.submit(() -> {
+            SceneGraphThreadWarden.setup(rootNodeThread1);
+            rootNodeThread1.attachChild(childThread2);
+            return null;
+        }));
+    }
+
+    /**
+     * Where there are multiple JME roots this tests that is *is* allowed to remove a child from one root and attach it
+     * to another as long as the detachments and reattachments are done in the right threads.
+     *
+     * <p>
+     *     This is a weird thing to do and unlikely to come up in practice but is allowed
+     * </p>
+     */
+    @Test
+    public void testCanTransferNodeBetweenMultipleSimultaneousRootThreads() throws ExecutionException, InterruptedException {
+        Node rootNodeThread1 = new Node("root1");
+        Node rootNodeThread2 = new Node("root2");
+
+        // Create a child node and attach it to the root node
+        Node childThread1 = new Node("child1");
+        Node childThread2 = new Node("child2");
+        Node childTransferred = new Node("childTransferred");
+
+        // on two threads set up two root nodes (for two JME applications on the same VM)
+        Future<Void> mutationFuture1 = executorService.submit(() -> {
+            SceneGraphThreadWarden.setup(rootNodeThread1);
+            rootNodeThread1.attachChild(childThread1);
+            rootNodeThread1.attachChild(childTransferred);
+            return null;
+        });
+
+        Future<Void> mutationFuture2 = executorService2.submit(() -> {
+            SceneGraphThreadWarden.setup(rootNodeThread2);
+            rootNodeThread2.attachChild(childThread2);
+            return null;
+        });
+
+        // These should complete without exceptions, these are independent scene graphs
+        mutationFuture1.get();
+        mutationFuture2.get();
+
+        Future<Void> legalRemovalFuture = executorService.submit(() -> {
+            childTransferred.removeFromParent();
+            return null;
+        });
+
+        legalRemovalFuture.get();
+
+        Future<Void> legalAttachmentFuture = executorService2.submit(() -> {
+            rootNodeThread2.attachChild(childTransferred);
+            return null;
+        });
+        legalAttachmentFuture.get();
+    }
 
     /**
      * Creates a single-threaded executor service with daemon threads.
@@ -313,4 +409,17 @@ public class SceneGraphThreadWardenTest {
             return t;
         };
     }
+
+    private void expectSceneGraphException(Future<Void> future){
+        try {
+            future.get();
+            fail("Expected an IllegalThreadSceneGraphMutation exception");
+        } catch (ExecutionException e) {
+            // This is expected - verify it's the right exception type
+            assertTrue("Expected IllegalThreadSceneGraphMutation, got: " + e.getCause().getClass().getName(),
+                    e.getCause() instanceof IllegalThreadSceneGraphMutation);
+        } catch (InterruptedException e){
+            fail("Unexpected InterruptedException");
+        }
+    }
 }