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 f1f21b58cc..7fe97af210 100644 --- a/jme3-core/src/main/java/com/jme3/app/SimpleApplication.java +++ b/jme3-core/src/main/java/com/jme3/app/SimpleApplication.java @@ -45,6 +45,7 @@ import com.jme3.renderer.queue.RenderQueue.Bucket; import com.jme3.scene.Node; import com.jme3.scene.Spatial.CullHint; +import com.jme3.scene.threadwarden.SceneGraphThreadWarden; import com.jme3.system.AppSettings; import com.jme3.system.JmeContext.Type; import com.jme3.system.JmeSystem; @@ -197,6 +198,11 @@ protected BitmapFont loadGuiFont() { public void initialize() { super.initialize(); + //noinspection AssertWithSideEffects + assert SceneGraphThreadWarden.setup(rootNode); + //noinspection AssertWithSideEffects + assert SceneGraphThreadWarden.setup(guiNode); + // Several things rely on having this guiFont = loadGuiFont(); @@ -240,6 +246,13 @@ 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/Geometry.java b/jme3-core/src/main/java/com/jme3/scene/Geometry.java index 008e987f1d..dd3da84c9e 100644 --- a/jme3-core/src/main/java/com/jme3/scene/Geometry.java +++ b/jme3-core/src/main/java/com/jme3/scene/Geometry.java @@ -44,6 +44,7 @@ import com.jme3.renderer.Camera; import com.jme3.scene.VertexBuffer.Type; import com.jme3.scene.mesh.MorphTarget; +import com.jme3.scene.threadwarden.SceneGraphThreadWarden; import com.jme3.util.TempVars; import com.jme3.util.clone.Cloner; import com.jme3.util.clone.IdentityCloneFunction; @@ -183,6 +184,7 @@ public void setIgnoreTransform(boolean ignoreTransform) { */ @Override public void setLodLevel(int lod) { + assert SceneGraphThreadWarden.assertOnCorrectThread(this); if (mesh.getNumLodLevels() == 0) { throw new IllegalStateException("LOD levels are not set on this mesh"); } @@ -239,6 +241,7 @@ public int getTriangleCount() { * @throws IllegalArgumentException If mesh is null */ public void setMesh(Mesh mesh) { + assert SceneGraphThreadWarden.assertOnCorrectThread(this); if (mesh == null) { throw new IllegalArgumentException(); } @@ -269,6 +272,7 @@ public Mesh getMesh() { */ @Override public void setMaterial(Material material) { + assert SceneGraphThreadWarden.assertOnCorrectThread(this); this.material = material; nbSimultaneousGPUMorph = -1; if (isGrouped()) { diff --git a/jme3-core/src/main/java/com/jme3/scene/Node.java b/jme3-core/src/main/java/com/jme3/scene/Node.java index 0424cea053..46bb0829e8 100644 --- a/jme3-core/src/main/java/com/jme3/scene/Node.java +++ b/jme3-core/src/main/java/com/jme3/scene/Node.java @@ -38,6 +38,7 @@ import com.jme3.export.JmeExporter; import com.jme3.export.JmeImporter; import com.jme3.material.Material; +import com.jme3.scene.threadwarden.SceneGraphThreadWarden; import com.jme3.util.SafeArrayList; import com.jme3.util.clone.Cloner; import java.io.IOException; @@ -201,6 +202,7 @@ private void addUpdateChildren(SafeArrayList results) { * that would change state. */ void invalidateUpdateList() { + assert SceneGraphThreadWarden.assertOnCorrectThread(this); updateListValid = false; if (parent != null) { parent.invalidateUpdateList(); @@ -344,6 +346,7 @@ public int attachChild(Spatial child) { * @throws IllegalArgumentException if child is null or this */ public int attachChildAt(Spatial child, int index) { + assert SceneGraphThreadWarden.assertOnCorrectThread(this); if (child == null) { throw new IllegalArgumentException("child cannot be null"); } @@ -428,6 +431,7 @@ public int detachChildNamed(String childName) { * @return the child at the supplied index. */ public Spatial detachChildAt(int index) { + assert SceneGraphThreadWarden.assertOnCorrectThread(this); Spatial child = children.remove(index); if (child != null) { child.setParent(null); @@ -455,6 +459,7 @@ public Spatial detachChildAt(int index) { * node. */ public void detachAllChildren() { + assert SceneGraphThreadWarden.assertOnCorrectThread(this); // Note: this could be a bit more efficient if it delegated // to a private method that avoided setBoundRefresh(), etc. // for every child and instead did one in here at the end. @@ -483,6 +488,7 @@ public int getChildIndex(Spatial sp) { * @param index2 The index of the second child to swap */ public void swapChildren(int index1, int index2) { + assert SceneGraphThreadWarden.assertOnCorrectThread(this); Spatial c2 = children.get(index2); Spatial c1 = children.remove(index1); children.add(index1, c2); @@ -562,6 +568,7 @@ public List getChildren() { @Override public void setMaterial(Material mat) { + assert SceneGraphThreadWarden.assertOnCorrectThread(this); for (int i = 0; i < children.size(); i++) { children.get(i).setMaterial(mat); } @@ -778,6 +785,7 @@ public void read(JmeImporter importer) throws IOException { @Override public void setModelBound(BoundingVolume modelBound) { + assert SceneGraphThreadWarden.assertOnCorrectThread(this); if (children != null) { for (Spatial child : children.getArray()) { child.setModelBound(modelBound != null ? modelBound.clone(null) : null); @@ -787,6 +795,7 @@ public void setModelBound(BoundingVolume modelBound) { @Override public void updateModelBound() { + assert SceneGraphThreadWarden.assertOnCorrectThread(this); if (children != null) { for (Spatial child : children.getArray()) { child.updateModelBound(); diff --git a/jme3-core/src/main/java/com/jme3/scene/Spatial.java b/jme3-core/src/main/java/com/jme3/scene/Spatial.java index 2fe1775d3e..7da3efaa5e 100644 --- a/jme3-core/src/main/java/com/jme3/scene/Spatial.java +++ b/jme3-core/src/main/java/com/jme3/scene/Spatial.java @@ -49,6 +49,7 @@ import com.jme3.renderer.queue.RenderQueue.Bucket; import com.jme3.renderer.queue.RenderQueue.ShadowMode; import com.jme3.scene.control.Control; +import com.jme3.scene.threadwarden.SceneGraphThreadWarden; import com.jme3.util.SafeArrayList; import com.jme3.util.TempVars; import com.jme3.util.clone.Cloner; @@ -278,11 +279,13 @@ protected void setRequiresUpdates(boolean f) { * a refresh is required. */ protected void setTransformRefresh() { + assert SceneGraphThreadWarden.assertOnCorrectThread(this); refreshFlags |= RF_TRANSFORM; setBoundRefresh(); } protected void setLightListRefresh() { + assert SceneGraphThreadWarden.assertOnCorrectThread(this); refreshFlags |= RF_LIGHTLIST; // Make sure next updateGeometricState() visits this branch // to update lights. @@ -299,6 +302,7 @@ protected void setLightListRefresh() { } protected void setMatParamOverrideRefresh() { + assert SceneGraphThreadWarden.assertOnCorrectThread(this); refreshFlags |= RF_MATPARAM_OVERRIDE; Spatial p = parent; while (p != null) { @@ -316,6 +320,7 @@ protected void setMatParamOverrideRefresh() { * a refresh is required. */ protected void setBoundRefresh() { + assert SceneGraphThreadWarden.assertOnCorrectThread(this); refreshFlags |= RF_BOUND; Spatial p = parent; @@ -364,7 +369,8 @@ public boolean checkCulling(Camera cam) { throw new IllegalStateException("Scene graph is not properly updated for rendering.\n" + "State was changed after rootNode.updateGeometricState() call. \n" + "Make sure you do not modify the scene from another thread!\n" - + "Problem spatial name: " + getName()); + + "Problem spatial name: " + getName() + "\n" + + SceneGraphThreadWarden.getTurnOnAssertsPrompt()); } CullHint cm = getCullHint(); @@ -612,6 +618,7 @@ protected void updateMatParamOverrides() { * @see MatParamOverride */ public void addMatParamOverride(MatParamOverride override) { + assert SceneGraphThreadWarden.assertOnCorrectThread(this); if (override == null) { throw new IllegalArgumentException("override cannot be null"); } @@ -626,6 +633,7 @@ public void addMatParamOverride(MatParamOverride override) { * @see MatParamOverride */ public void removeMatParamOverride(MatParamOverride override) { + assert SceneGraphThreadWarden.assertOnCorrectThread(this); if (localOverrides.remove(override)) { setMatParamOverrideRefresh(); } @@ -637,6 +645,7 @@ public void removeMatParamOverride(MatParamOverride override) { * @see #addMatParamOverride(com.jme3.material.MatParamOverride) */ public void clearMatParamOverrides() { + assert SceneGraphThreadWarden.assertOnCorrectThread(this); if (!localOverrides.isEmpty()) { setMatParamOverrideRefresh(); } @@ -772,6 +781,7 @@ public void runControlRender(RenderManager rm, ViewPort vp) { * @see Spatial#removeControl(java.lang.Class) */ public void addControl(Control control) { + assert SceneGraphThreadWarden.assertOnCorrectThread(this); boolean before = requiresUpdates(); controls.add(control); control.setSpatial(this); @@ -823,6 +833,7 @@ public void addControlAt(int index, Control control) { * @see Spatial#addControl(com.jme3.scene.control.Control) */ public void removeControl(Class controlType) { + assert SceneGraphThreadWarden.assertOnCorrectThread(this); boolean before = requiresUpdates(); for (int i = 0; i < controls.size(); i++) { if (controlType.isAssignableFrom(controls.get(i).getClass())) { @@ -850,6 +861,7 @@ public void removeControl(Class controlType) { * @see Spatial#addControl(com.jme3.scene.control.Control) */ public boolean removeControl(Control control) { + assert SceneGraphThreadWarden.assertOnCorrectThread(this); boolean before = requiresUpdates(); boolean result = controls.remove(control); if (result) { @@ -1005,6 +1017,7 @@ public Node getParent() { * the parent of this node. */ protected void setParent(Node parent) { + assert SceneGraphThreadWarden.updateRequirement(this, parent); this.parent = parent; } @@ -1369,6 +1382,7 @@ public RenderQueue.ShadowMode getShadowMode() { * @param lod The lod level to set. */ public void setLodLevel(int lod) { + assert SceneGraphThreadWarden.assertOnCorrectThread(this); } /** diff --git a/jme3-core/src/main/java/com/jme3/scene/threadwarden/IllegalThreadSceneGraphMutation.java b/jme3-core/src/main/java/com/jme3/scene/threadwarden/IllegalThreadSceneGraphMutation.java new file mode 100644 index 0000000000..64fe4bee8a --- /dev/null +++ b/jme3-core/src/main/java/com/jme3/scene/threadwarden/IllegalThreadSceneGraphMutation.java @@ -0,0 +1,7 @@ +package com.jme3.scene.threadwarden; + +public class IllegalThreadSceneGraphMutation extends IllegalStateException{ + public IllegalThreadSceneGraphMutation(String message){ + super(message); + } +} 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 new file mode 100644 index 0000000000..72724c380d --- /dev/null +++ b/jme3-core/src/main/java/com/jme3/scene/threadwarden/SceneGraphThreadWarden.java @@ -0,0 +1,160 @@ +package com.jme3.scene.threadwarden; + +import com.jme3.scene.Node; +import com.jme3.scene.Spatial; + +import java.util.Collections; +import java.util.Set; +import java.util.WeakHashMap; +import java.util.logging.Level; +import java.util.logging.Logger; + +/** + * Thread warden keeps track of mutations to the scene graph and ensures that they are only done on the main thread. + * IF the parent node is marked as being reserved for the main thread (which basically means it's connected to the + * root node) + *

+ * Only has an effect if asserts are on + *

+ */ +public class SceneGraphThreadWarden { + + private static final Logger logger = Logger.getLogger(SceneGraphThreadWarden.class.getName()); + + /** + * If THREAD_WARDEN_ENABLED is true AND asserts are on the checks are made. + * This parameter is here to allow asserts to run without thread warden checks (by setting this parameter to false) + */ + public static boolean THREAD_WARDEN_ENABLED = !Boolean.getBoolean("nothreadwarden"); + + public static boolean ASSERTS_ENABLED = false; + + static{ + //noinspection AssertWithSideEffects + assert ASSERTS_ENABLED = true; + } + + public static Thread mainThread; + public static final Set spatialsThatAreMainThreadReserved = Collections.synchronizedSet(Collections.newSetFromMap(new WeakHashMap<>())); + + /** + * Marks the given node as being reserved for the main thread. + * Additionally, sets the current thread as the main thread (if it hasn't already been set) + * @param rootNode the root node of the scene graph. This is used to determine if a spatial is a child of the root node. + * (Can add multiple "root" nodes, e.g. gui nodes or overlay nodes) + */ + public static boolean setup(Node rootNode){ + if(checksDisabled()){ + 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()); + } + mainThread = thisThread; + setTreeRestricted(rootNode); + + return true; // return true so can be a "side effect" of an assert + } + + /** + * Disables the thread warden checks (even when other asserts are on). + *

+ * Alternatively can be disabled by adding the -Dnothreadwarden=true parameter + *

+ */ + public static void disableChecks(){ + THREAD_WARDEN_ENABLED = false; + } + + /** + * 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); + if(spatial instanceof Node){ + for(Spatial child : ((Node) spatial).getChildren()){ + setTreeRestricted(child); + } + } + } + + /** + * Releases this tree from being only allowed to be mutated on the main thread + * @param spatial the node (and children) to release the restriction state of. + */ + private static void setTreeNotRestricted(Spatial spatial){ + spatialsThatAreMainThreadReserved.remove(spatial); + if(spatial instanceof Node){ + for(Spatial child : ((Node) spatial).getChildren()){ + setTreeNotRestricted(child); + } + } + } + + @SuppressWarnings("SameReturnValue") + public static boolean updateRequirement(Spatial spatial, Node newParent){ + if(checksDisabled()){ + return true; + } + + boolean shouldNowBeRestricted = newParent !=null && spatialsThatAreMainThreadReserved.contains(newParent); + boolean wasPreviouslyRestricted = spatialsThatAreMainThreadReserved.contains(spatial); + + if(shouldNowBeRestricted || wasPreviouslyRestricted ){ + assertOnCorrectThread(spatial); + } + + if(shouldNowBeRestricted == wasPreviouslyRestricted){ + return true; + } + if(shouldNowBeRestricted){ + setTreeRestricted(spatial); + }else{ + setTreeNotRestricted(spatial); + } + + return true; // return true so can be a "side effect" of an assert + } + + 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 + } + + private static boolean checksDisabled(){ + return !THREAD_WARDEN_ENABLED || !ASSERTS_ENABLED; + } + + @SuppressWarnings("SameReturnValue") + public static boolean assertOnCorrectThread(Spatial spatial){ + if(checksDisabled()){ + return true; + } + if(spatialsThatAreMainThreadReserved.contains(spatial)){ + if(Thread.currentThread() != mainThread){ + // 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(); + IllegalThreadSceneGraphMutation ex = new IllegalThreadSceneGraphMutation(message); + logger.log(Level.WARNING, message, ex); + + throw ex; + } + } + return true; // return true so can be a "side effect" of an assert + } + + public static String getTurnOnAssertsPrompt(){ + if(ASSERTS_ENABLED){ + return ""; + } else{ + return "To get more accurate debug consider turning on asserts. This will allow JME to do additional checks which *may* find the source of the problem. To do so, add -ea to the JVM arguments."; + } + } + +} + diff --git a/jme3-core/src/test/java/com/jme3/scene/threadwarden/SceneGraphThreadWardenGeometryExtendedTest.java b/jme3-core/src/test/java/com/jme3/scene/threadwarden/SceneGraphThreadWardenGeometryExtendedTest.java new file mode 100644 index 0000000000..3b264a4177 --- /dev/null +++ b/jme3-core/src/test/java/com/jme3/scene/threadwarden/SceneGraphThreadWardenGeometryExtendedTest.java @@ -0,0 +1,207 @@ +package com.jme3.scene.threadwarden; + +import com.jme3.material.Material; +import com.jme3.scene.Geometry; +import com.jme3.scene.Mesh; +import com.jme3.scene.Node; +import com.jme3.scene.shape.Box; +import org.junit.After; +import org.junit.Before; +import org.junit.BeforeClass; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.Parameterized; +import org.mockito.Mockito; + +import java.util.Arrays; +import java.util.Collection; +import java.util.concurrent.ExecutionException; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.Future; +import java.util.concurrent.ThreadFactory; +import java.util.function.Consumer; + +import static org.junit.Assert.assertTrue; +import static org.junit.Assert.fail; + +/** + * Parameterized tests for SceneGraphThreadWarden class with Geometry objects. + * These tests verify that various scene graph mutations are properly checked for thread safety. + */ +@RunWith(Parameterized.class) +public class SceneGraphThreadWardenGeometryExtendedTest { + + private static ExecutorService executorService; + + private final String testName; + private final Consumer action; + + @SuppressWarnings({"ReassignedVariable", "AssertWithSideEffects"}) + @BeforeClass + public static void setupClass() { + // Make sure assertions are enabled + boolean assertsEnabled = false; + assert assertsEnabled = true; + if (!assertsEnabled) { + throw new RuntimeException("WARNING: Assertions are not enabled! Tests may not work correctly."); + } + } + + @Before + public void setup() { + executorService = newSingleThreadDaemonExecutor(); + } + + @After + public void tearDown() { + executorService.shutdown(); + SceneGraphThreadWarden.reset(); + } + + /** + * Constructor for the parameterized test. + * + * @param testName A descriptive name for the test + * @param action The action to perform on the spatial + */ + public SceneGraphThreadWardenGeometryExtendedTest(String testName, Consumer action) { + this.testName = testName; + this.action = action; + } + + /** + * Define the parameters for the test. + * Each parameter is a pair of (test name, action to perform on spatial). + */ + @Parameterized.Parameters(name = "{0}") + public static Collection data() { + Material mockMaterial = Mockito.mock(Material.class); + Box box = new Box(1, 1, 1); + + return Arrays.asList(new Object[][] { + { + "setMaterial", + (Consumer) spatial -> spatial.setMaterial(mockMaterial) + }, + { + "setMesh", + (Consumer) spatial -> spatial.setMesh(box) + }, + { + "setLodLevel", + (Consumer) spatial -> { + // Need to set a mesh with LOD levels first + Mesh mesh = new Box(1, 1, 1); + mesh.setLodLevels(new com.jme3.scene.VertexBuffer[]{ + mesh.getBuffer(com.jme3.scene.VertexBuffer.Type.Index) + }); + spatial.setMesh(mesh); + spatial.setLodLevel(0); + } + }, + { + "removeFromParent", + (Consumer) Geometry::removeFromParent + } + }); + } + + /** + * Test that scene graph mutation is fine on the main thread when the object is attached to the root. + */ + @Test + public void testMutationOnMainThreadOnAttachedObject() { + Node rootNode = new Node("root"); + SceneGraphThreadWarden.setup(rootNode); + + // Create a geometry and attach it to the root node + Geometry geometry = new Geometry("geometry", new Box(1, 1, 1)); + rootNode.attachChild(geometry); + + // This should work fine since we're on the main thread + action.accept(geometry); + } + + /** + * Test that scene graph mutation is fine on the main thread when the object is not attached to the root. + */ + @Test + public void testMutationOnMainThreadOnDetachedObject() { + Node rootNode = new Node("root"); + SceneGraphThreadWarden.setup(rootNode); + + // Create a geometry but don't attach it to the root node + Geometry geometry = new Geometry("geometry", new Box(1, 1, 1)); + + // This should work fine since we're on the main thread + action.accept(geometry); + } + + /** + * Test that scene graph mutation is fine on a non-main thread when the object is not attached to the root. + */ + @Test + public void testMutationOnNonMainThreadOnDetachedObject() throws ExecutionException, InterruptedException { + Node rootNode = new Node("root"); + SceneGraphThreadWarden.setup(rootNode); + + // Create a geometry but don't attach it to the root node + Geometry geometry = new Geometry("geometry", new Box(1, 1, 1)); + + Future future = executorService.submit(() -> { + // This should work fine since the geometry is not connected to the root node + action.accept(geometry); + return null; + }); + + // This should complete without exceptions + future.get(); + } + + /** + * Test that scene graph mutation is not allowed on a non-main thread when the object is attached to the root. + */ + @Test + public void testMutationOnNonMainThreadOnAttachedObject() throws InterruptedException { + Node rootNode = new Node("root"); + SceneGraphThreadWarden.setup(rootNode); + + // Create a geometry and attach it to the root node + Geometry geometry = new Geometry("geometry", new Box(1, 1, 1)); + rootNode.attachChild(geometry); + + Future future = executorService.submit(() -> { + // This should fail because we're trying to modify a geometry that's connected to the scene graph + action.accept(geometry); + return null; + }); + + 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); + } + } + + /** + * Creates a single-threaded executor service with daemon threads. + */ + private static ExecutorService newSingleThreadDaemonExecutor() { + return Executors.newSingleThreadExecutor(daemonThreadFactory()); + } + + /** + * Creates a thread factory that produces daemon threads. + */ + private static ThreadFactory daemonThreadFactory() { + return r -> { + Thread t = Executors.defaultThreadFactory().newThread(r); + t.setDaemon(true); + return t; + }; + } +} \ No newline at end of file diff --git a/jme3-core/src/test/java/com/jme3/scene/threadwarden/SceneGraphThreadWardenNodeExtendedTest.java b/jme3-core/src/test/java/com/jme3/scene/threadwarden/SceneGraphThreadWardenNodeExtendedTest.java new file mode 100644 index 0000000000..7c36f07bcd --- /dev/null +++ b/jme3-core/src/test/java/com/jme3/scene/threadwarden/SceneGraphThreadWardenNodeExtendedTest.java @@ -0,0 +1,203 @@ +package com.jme3.scene.threadwarden; + +import com.jme3.material.Material; +import com.jme3.material.MatParamOverride; +import com.jme3.scene.Node; +import com.jme3.scene.Spatial; +import com.jme3.shader.VarType; +import org.junit.After; +import org.junit.Before; +import org.junit.BeforeClass; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.Parameterized; +import org.mockito.Mockito; + +import java.util.Arrays; +import java.util.Collection; +import java.util.concurrent.ExecutionException; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.Future; +import java.util.concurrent.ThreadFactory; +import java.util.function.Consumer; + +import static org.junit.Assert.assertTrue; +import static org.junit.Assert.fail; + +/** + * Parameterized tests for SceneGraphThreadWarden class. + * These tests verify that various scene graph mutations are properly checked for thread safety. + */ +@RunWith(Parameterized.class) +public class SceneGraphThreadWardenNodeExtendedTest { + + private static ExecutorService executorService; + + private final String testName; + private final Consumer action; + + @SuppressWarnings({"ReassignedVariable", "AssertWithSideEffects"}) + @BeforeClass + public static void setupClass() { + // Make sure assertions are enabled + boolean assertsEnabled = false; + assert assertsEnabled = true; + if (!assertsEnabled) { + throw new RuntimeException("WARNING: Assertions are not enabled! Tests may not work correctly."); + } + } + + @Before + public void setup() { + executorService = newSingleThreadDaemonExecutor(); + } + + @After + public void tearDown() { + executorService.shutdown(); + SceneGraphThreadWarden.reset(); + } + + /** + * Constructor for the parameterized test. + * + * @param testName A descriptive name for the test + * @param action The action to perform on the spatial + */ + public SceneGraphThreadWardenNodeExtendedTest(String testName, Consumer action) { + this.testName = testName; + this.action = action; + } + + /** + * Define the parameters for the test. + * Each parameter is a pair of (test name, action to perform on spatial). + */ + @Parameterized.Parameters(name = "{0}") + public static Collection data() { + Material mockMaterial = Mockito.mock(Material.class); + MatParamOverride override = new MatParamOverride(VarType.Float, "TestParam", 1.0f); + + return Arrays.asList(new Object[][] { + { + "setMaterial", + (Consumer) spatial -> spatial.setMaterial(mockMaterial) + }, + { + "setLodLevel", + (Consumer) spatial -> spatial.setLodLevel(1) + }, + { + "addMatParamOverride", + (Consumer) spatial -> spatial.addMatParamOverride(override) + }, + { + "removeMatParamOverride", + (Consumer) spatial -> spatial.removeMatParamOverride(override) + }, + { + "clearMatParamOverrides", + (Consumer) Spatial::clearMatParamOverrides + } + }); + } + + /** + * Test that scene graph mutation is fine on the main thread when the object is attached to the root. + */ + @Test + public void testMutationOnMainThreadOnAttachedObject() { + Node rootNode = new Node("root"); + SceneGraphThreadWarden.setup(rootNode); + + // Create a child node and attach it to the root node + Node child = new Node("child"); + rootNode.attachChild(child); + + // This should work fine since we're on the main thread + action.accept(child); + } + + /** + * Test that scene graph mutation is fine on the main thread when the object is not attached to the root. + */ + @Test + public void testMutationOnMainThreadOnDetachedObject() { + Node rootNode = new Node("root"); + SceneGraphThreadWarden.setup(rootNode); + + // Create a child node but don't attach it to the root node + Node child = new Node("child"); + + // This should work fine since we're on the main thread + action.accept(child); + } + + /** + * Test that scene graph mutation is fine on a non-main thread when the object is not attached to the root. + */ + @Test + public void testMutationOnNonMainThreadOnDetachedObject() throws ExecutionException, InterruptedException { + Node rootNode = new Node("root"); + SceneGraphThreadWarden.setup(rootNode); + + // Create a child node but don't attach it to the root node + Node child = new Node("child"); + + Future future = executorService.submit(() -> { + // This should work fine since the node is not connected to the root node + action.accept(child); + return null; + }); + + // This should complete without exceptions + future.get(); + } + + /** + * Test that scene graph mutation is not allowed on a non-main thread when the object is attached to the root. + */ + @Test + public void testMutationOnNonMainThreadOnAttachedObject() throws InterruptedException { + Node rootNode = new Node("root"); + SceneGraphThreadWarden.setup(rootNode); + + // Create a child node and attach it to the root node + Node child = new Node("child"); + rootNode.attachChild(child); + + Future future = executorService.submit(() -> { + // This should fail because we're trying to modify a node that's connected to the scene graph + action.accept(child); + return null; + }); + + 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); + } + } + + /** + * Creates a single-threaded executor service with daemon threads. + */ + private static ExecutorService newSingleThreadDaemonExecutor() { + return Executors.newSingleThreadExecutor(daemonThreadFactory()); + } + + /** + * Creates a thread factory that produces daemon threads. + */ + private static ThreadFactory daemonThreadFactory() { + return r -> { + Thread t = Executors.defaultThreadFactory().newThread(r); + t.setDaemon(true); + return t; + }; + } +} 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 new file mode 100644 index 0000000000..7c92b2edd6 --- /dev/null +++ b/jme3-core/src/test/java/com/jme3/scene/threadwarden/SceneGraphThreadWardenTest.java @@ -0,0 +1,316 @@ +package com.jme3.scene.threadwarden; + +import com.jme3.scene.Node; +import org.junit.After; +import org.junit.Before; +import org.junit.BeforeClass; +import org.junit.Test; + +import java.util.concurrent.ExecutionException; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.Future; +import java.util.concurrent.ThreadFactory; + +import static org.junit.Assert.assertTrue; +import static org.junit.Assert.fail; + +/** + * Tests for SceneGraphThreadWarden class. + * These tests verify that: + * - Normal node mutation is fine on the main thread + * - Node mutation on nodes not connected to the root node is fine even on a non main thread + * - Adding a node to the scene graph (indirectly) connected to the root node isn't fine on a non main thread + * - Adding a node currently attached to a root node to a different node isn't fine on a non main thread + */ +public class SceneGraphThreadWardenTest { + + private static ExecutorService executorService; + + @SuppressWarnings({"ReassignedVariable", "AssertWithSideEffects"}) + @BeforeClass + public static void setupClass() { + // Make sure assertions are enabled + boolean assertsEnabled = false; + assert assertsEnabled = true; + //noinspection ConstantValue + if (!assertsEnabled) { + throw new RuntimeException("WARNING: Assertions are not enabled! Tests may not work correctly."); + } + } + + @Before + public void setup() { + executorService = newSingleThreadDaemonExecutor(); + } + + @After + public void tearDown() { + executorService.shutdown(); + SceneGraphThreadWarden.reset(); + } + + /** + * Test that normal node mutation is fine on the main thread. + */ + @Test + public void testNormalNodeMutationOnMainThread() { + Node rootNode = new Node("root"); + SceneGraphThreadWarden.setup(rootNode); + + // This should work fine since we're on the main thread + Node child = new Node("child"); + rootNode.attachChild(child); + + // Add another level of children + Node grandchild = new Node("grandchild"); + child.attachChild(grandchild); + + // Detach should also work fine + child.detachChild(grandchild); + rootNode.detachChild(child); + } + + /** + * Test that node mutation on nodes not connected to the root node is fine even on a non main thread. + *

+ * This is a use case where a thread is preparing things for later attachment to the scene graph. + *

+ */ + @Test + public void testNodeMutationOnNonConnectedNodesOnNonMainThread() throws ExecutionException, InterruptedException { + Node rootNode = new Node("root"); + SceneGraphThreadWarden.setup(rootNode); + + Future nonConnectedNodeFuture = executorService.submit(() -> { + // This should work fine since these nodes are not connected to the root node + Node parent = new Node("parent"); + Node child = new Node("child"); + parent.attachChild(child); + + // Add another level of children + Node grandchild = new Node("grandchild"); + child.attachChild(grandchild); + + return parent; + }); + + // Get the result to ensure the task completed without exceptions + Node nonConnectedNode = nonConnectedNodeFuture.get(); + + // Now we can attach it to the root node on the main thread + rootNode.attachChild(nonConnectedNode); + } + + /** + * Test that adding a node to the scene graph connected to the root node in a non main thread leads to an + * exception. + */ + @Test + public void testAddingNodeToSceneGraphOnNonMainThread() throws InterruptedException { + Node rootNode = new Node("root"); + SceneGraphThreadWarden.setup(rootNode); + + // Create a child node and attach it to the root node + Node child = new Node("child"); + rootNode.attachChild(child); + + Future illegalMutationFuture = executorService.submit(() -> { + // This should fail because we're trying to add a node to a node that's connected to the scene graph + Node grandchild = new Node("grandchild"); + child.attachChild(grandchild); + 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); + } + } + + /** + * Test that adding a node currently attached to a root node to a different node leads to an exception. + *

+ * This is testing an edge case where you think you'd working with non-connected nodes, but in reality + * one of your nodes is already attached to the scene graph (and you're attaching it to a different node which will + * detach it from the scene graph). + *

+ */ + @Test + public void testMovingNodeAttachedToRootOnNonMainThread() throws InterruptedException { + Node rootNode = new Node("root"); + SceneGraphThreadWarden.setup(rootNode); + + // Create two child nodes and attach them to the root node + Node child1 = new Node("child1"); + Node child2 = new Node("child2"); + + rootNode.attachChild(child2); + + Future illegalMutationFuture = executorService.submit(() -> { + // This should fail because we're trying to move a node that's connected to the root node + child1.attachChild(child2); // This implicitly detaches child2 from rootNode + 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); + } + } + + /** + * Test that detaching a node releases it from thread protection. + */ + @Test + public void testDetachmentReleasesProtection() throws ExecutionException, InterruptedException { + Node rootNode = new Node("root"); + SceneGraphThreadWarden.setup(rootNode); + + // Create a child node and attach it to the root node + Node child = new Node("child"); + rootNode.attachChild(child); + + // Now detach it from the root node + child.removeFromParent(); + + // Now we should be able to modify it on another thread + Future legalMutationFuture = executorService.submit(() -> { + Node grandchild = new Node("grandchild"); + child.attachChild(grandchild); + return null; + }); + + // This should complete without exceptions + legalMutationFuture.get(); + } + + /** + * Test that adding a child to the root node also restricts the grandchild. + * This test will add a grandchild to a child BEFORE adding the child to the root, + * then try (and fail) to make an illegal on-thread change to the grandchild. + */ + @Test + public void testAddingAChildToTheRootNodeAlsoRestrictsTheGrandChild() throws InterruptedException { + Node rootNode = new Node("root"); + SceneGraphThreadWarden.setup(rootNode); + + // Create a child node and a grandchild node + Node child = new Node("child"); + Node grandchild = new Node("grandchild"); + + // Attach the grandchild to the child BEFORE adding the child to the root + child.attachChild(grandchild); + + // Now attach the child to the root node + rootNode.attachChild(child); + + // Try to make an illegal on-thread change to the grandchild + Future illegalMutationFuture = executorService.submit(() -> { + // This should fail because the grandchild is now restricted + Node greatGrandchild = new Node("greatGrandchild"); + grandchild.attachChild(greatGrandchild); + 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); + } + } + + /** + * Test that removing a child from the root node also unrestricts the grandchild. + * This test will add a child with a grandchild to the root node, then remove the child + * and verify that the grandchild can be modified on a non-main thread. + */ + @Test + public void testRemovingAChildFromTheRootNodeAlsoUnrestrictsTheGrandChild() throws ExecutionException, InterruptedException { + Node rootNode = new Node("root"); + SceneGraphThreadWarden.setup(rootNode); + + // Create a child node and a grandchild node + Node child = new Node("child"); + Node grandchild = new Node("grandchild"); + + // Attach the grandchild to the child + child.attachChild(grandchild); + + // Attach the child to the root node + rootNode.attachChild(child); + + // Now remove the child from the root node + child.removeFromParent(); + + // Try to make a change to the grandchild on a non-main thread + Future legalMutationFuture = executorService.submit(() -> { + // This should succeed because the grandchild is no longer restricted + Node greatGrandchild = new Node("greatGrandchild"); + grandchild.attachChild(greatGrandchild); + return null; + }); + + // This should complete without exceptions + legalMutationFuture.get(); + } + + /** + * Test that an otherwise illegal scene graph mutation won't throw an exception + * if the checks have been disabled by calling disableChecks(). + */ + @Test + public void testDisableChecksAllowsIllegalMutation() throws ExecutionException, InterruptedException { + Node rootNode = new Node("root"); + SceneGraphThreadWarden.setup(rootNode); + + // Create a child node and attach it to the root node + Node child = new Node("child"); + rootNode.attachChild(child); + + // Disable the thread warden checks + SceneGraphThreadWarden.disableChecks(); + + // Try to make a change to the child on a non-main thread + // This would normally be illegal, but should succeed because checks are disabled + Future mutationFuture = executorService.submit(() -> { + Node grandchild = new Node("grandchild"); + child.attachChild(grandchild); + return null; + }); + + // This should complete without exceptions + mutationFuture.get(); + } + + + + /** + * Creates a single-threaded executor service with daemon threads. + */ + private static ExecutorService newSingleThreadDaemonExecutor() { + return Executors.newSingleThreadExecutor(daemonThreadFactory()); + } + + /** + * Creates a thread factory that produces daemon threads. + */ + private static ThreadFactory daemonThreadFactory() { + return r -> { + Thread t = Executors.defaultThreadFactory().newThread(r); + t.setDaemon(true); + return t; + }; + } +}