diff --git a/jme3-core/src/main/java/com/jme3/app/SimpleApplication.java b/jme3-core/src/main/java/com/jme3/app/SimpleApplication.java index 704d0eb77a..7850b4b218 100644 --- a/jme3-core/src/main/java/com/jme3/app/SimpleApplication.java +++ b/jme3-core/src/main/java/com/jme3/app/SimpleApplication.java @@ -307,13 +307,6 @@ public void initialize() { simpleInitApp(); } - @Override - public void stop(boolean waitFor) { - //noinspection AssertWithSideEffects - assert SceneGraphThreadWarden.reset(); - super.stop(waitFor); - } - @Override public void update() { if (prof != null) { diff --git a/jme3-core/src/main/java/com/jme3/scene/threadwarden/SceneGraphThreadWarden.java b/jme3-core/src/main/java/com/jme3/scene/threadwarden/SceneGraphThreadWarden.java index 72724c380d..81bd2688de 100644 --- a/jme3-core/src/main/java/com/jme3/scene/threadwarden/SceneGraphThreadWarden.java +++ b/jme3-core/src/main/java/com/jme3/scene/threadwarden/SceneGraphThreadWarden.java @@ -4,7 +4,7 @@ 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 @@ * Only has an effect if asserts are on *

*/ +@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 spatialsThatAreMainThreadReserved = Collections.synchronizedSet(Collections.newSetFromMap(new WeakHashMap<>())); + public static final Map spatialsThatAreMainThreadReserved = Collections.synchronizedMap(new WeakHashMap<>()); /** * Marks the given node as being reserved for the main thread. @@ -48,12 +48,11 @@ public static boolean setup(Node rootNode){ 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 static void disableChecks(){ * 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 static boolean updateRequirement(Spatial spatial, Node newParent){ 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 static boolean updateRequirement(Spatial spatial, Node newParent){ return true; } if(shouldNowBeRestricted){ - setTreeRestricted(spatial); + setTreeRestricted(spatial, Thread.currentThread()); }else{ setTreeNotRestricted(spatial); } @@ -118,9 +117,11 @@ public static boolean updateRequirement(Spatial spatial, Node newParent){ 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 static boolean assertOnCorrectThread(Spatial spatial){ 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(); diff --git a/jme3-core/src/test/java/com/jme3/scene/threadwarden/SceneGraphThreadWardenTest.java b/jme3-core/src/test/java/com/jme3/scene/threadwarden/SceneGraphThreadWardenTest.java index 7c92b2edd6..b06c4cc8cc 100644 --- a/jme3-core/src/test/java/com/jme3/scene/threadwarden/SceneGraphThreadWardenTest.java +++ b/jme3-core/src/test/java/com/jme3/scene/threadwarden/SceneGraphThreadWardenTest.java @@ -26,6 +26,7 @@ public class SceneGraphThreadWardenTest { private static ExecutorService executorService; + private static ExecutorService executorService2; @SuppressWarnings({"ReassignedVariable", "AssertWithSideEffects"}) @BeforeClass @@ -42,11 +43,13 @@ public static void setupClass() { @Before public void setup() { executorService = newSingleThreadDaemonExecutor(); + executorService2 = newSingleThreadDaemonExecutor(); } @After public void tearDown() { executorService.shutdown(); + executorService2.shutdown(); SceneGraphThreadWarden.reset(); } @@ -71,6 +74,31 @@ public void testNormalNodeMutationOnMainThread() { 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. *

@@ -107,7 +135,7 @@ public void testNodeMutationOnNonConnectedNodesOnNonMainThread() throws Executio * exception. */ @Test - public void testAddingNodeToSceneGraphOnNonMainThread() throws InterruptedException { + public void testAddingNodeToSceneGraphOnNonMainThread() { Node rootNode = new Node("root"); SceneGraphThreadWarden.setup(rootNode); @@ -122,14 +150,7 @@ public void testAddingNodeToSceneGraphOnNonMainThread() throws InterruptedExcept 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 void testAddingNodeToSceneGraphOnNonMainThread() throws InterruptedExcept *

*/ @Test - public void testMovingNodeAttachedToRootOnNonMainThread() throws InterruptedException { + public void testMovingNodeAttachedToRootOnNonMainThread() { Node rootNode = new Node("root"); SceneGraphThreadWarden.setup(rootNode); @@ -157,14 +178,7 @@ public void testMovingNodeAttachedToRootOnNonMainThread() throws InterruptedExce 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 void testDetachmentReleasesProtection() throws ExecutionException, Interr * 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 void testAddingAChildToTheRootNodeAlsoRestrictsTheGrandChild() throws Int 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 void testDisableChecksAllowsIllegalMutation() throws ExecutionException, 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). + *

+ * This test tests that using two seperate executor services with their own seperate threads + *

+ */ + @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 mutationFuture1 = executorService.submit(() -> { + SceneGraphThreadWarden.setup(rootNodeThread1); + rootNodeThread1.attachChild(childThread1); + return null; + }); + + Future 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. + * + *

+ * This is a weird thing to do and unlikely to come up in practice but is allowed + *

+ */ + @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 mutationFuture1 = executorService.submit(() -> { + SceneGraphThreadWarden.setup(rootNodeThread1); + rootNodeThread1.attachChild(childThread1); + rootNodeThread1.attachChild(childTransferred); + return null; + }); + + Future 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 legalRemovalFuture = executorService.submit(() -> { + childTransferred.removeFromParent(); + return null; + }); + + legalRemovalFuture.get(); + + Future legalAttachmentFuture = executorService2.submit(() -> { + rootNodeThread2.attachChild(childTransferred); + return null; + }); + legalAttachmentFuture.get(); + } /** * Creates a single-threaded executor service with daemon threads. @@ -313,4 +409,17 @@ private static ThreadFactory daemonThreadFactory() { return t; }; } + + private void expectSceneGraphException(Future 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"); + } + } }