Multithreading issue

Hello!



I am really tired of getting this erros:

[java]Dec 1, 2011 5:11:21 PM com.jme3.scene.Node attachChild

INFO: Child (g_Earth_root3) attached to this node (Earth)

Dec 1, 2011 5:11:21 PM com.jme3.app.Application handleError

SEVERE: Uncaught exception thrown in Thread[LWJGL Renderer Thread,5,main]

java.lang.IllegalStateException: Scene graph is not properly updated for rendering.

State was changed after rootNode.updateGeometricState() call.

Make sure you do not modify the scene from another thread!

Problem spatial name: Root_NODE

at com.jme3.scene.Spatial.checkCulling(Spatial.java:254)

at com.jme3.renderer.RenderManager.renderScene(RenderManager.java:776)

at com.jme3.renderer.RenderManager.renderViewPort(RenderManager.java:1117)

at com.jme3.renderer.RenderManager.render(RenderManager.java:1168)

at com.jme3.app.SimpleApplicationMod.update(SimpleApplicationMod.java:269)

at com.jme3.system.lwjgl.LwjglAbstractDisplay.runLoop(LwjglAbstractDisplay.java:149)

at com.jme3.system.lwjgl.LwjglDisplay.runLoop(LwjglDisplay.java:185)

at com.jme3.system.lwjgl.LwjglAbstractDisplay.run(LwjglAbstractDisplay.java:223)

at java.lang.Thread.run(Thread.java:680) [/java]



In my app I do use threading via ScheduledExecutorService and custom classes, that implements Runnable, and are used to procedurally make complex geometries and add them to the rootNode. I get this error not every time I add geometry, but like 50/50. I believe this is happening when some procedures in update loop of jme aren’t finished when I try to modify rootNode, which causes this crash. My question is how to find what should be executed in jme, after which I can safely run/resume my threads that make and attach my geometry? Or, if to rephrase my question, is there any indicator in jme, that I can use to know, whether I can or I cannot run/resume my threads?

Enqueue the attaching via callables:



https://wiki.jmonkeyengine.org/legacy/doku.php/jme3:advanced:multithreading

@normen said:
Enqueue the attaching via callables:

In the topic you provided the callable is used to make a copy of spatial location so that it can be used to calculate the path (or to get primary info to get result. In my case it is like vice versa - I need not the initial values, but be able to "work" with the result). Does it mean that by using this technique I need to "run callable" to get a clone of rootNode so to safely attach my new geometry?

You should gather as much data as you can before (on the thread that can read it, so the opengl one) and then process that data without accessing data that is used by the main thread. If you have to do so, do it safely, yeah. Normally you would just get the result and do everything that has to be done on the main thread there (so if(future.isDone()){mySpatial.setLocalTranslation(future.get())} or something)

Try to never ever use futures if you can avoid them. They are a recipe for deadlocks and in 99% of cases you can structure code to by full asynchronous and avoid them (and reap other benefits). There are times when you might need them but if we are even having this conversation then Future is too dangerous to use as a general approach.



In your case, I think it might be completely unnecessary anyway. When you are done building your geometry just enqueue a callable that will add it to the root node. It’s the adding that needs to run on the render thread. You can create independent geometry all you want on another thread and AssetManager is thread safe. It’s only when you modify the connected scene graph that you have to do it on the render thread.



So, they general rule is that if a node is connected to some node managed by JME then you need to update it on the render thread (via Callable or some other mechanism). If it is not connected then it is generally safe to do whatever with.

@pspeed said:
Try to never ever use futures if you can avoid them. There are times when you might need them but if we are even having this conversation then Future is too dangerous to use as a general approach.

:? That only is true when you do your threading _not_ like I outlined but rather do not exactly know what is called from what thread. The idea is that you not just "insert" the logic code for whatever has to be done from some other part of the code using the Callable but that you do it where the logic resides that wanted the process to be started in the first place.

And never mind that it creates an ugly reverse coupling where an async notification would have been more flexible. And never mind that you skip at least one frame per-request for potentially no good reason…



It is only safe if you never use synchronize… anywhere. Or at least never ever from the render loop around anything that your other threads might want. Otherwise, you risk deadlock by accident. And in my experience, the kind of folks that need this threading advice in the first place are very likely to have sprinkled synchronized() all over the place.



It’s better to always push tasking from the render thread when you can get away with it… and then provide that tasking everything it needs to complete it’s job.



In general, if your background tasks have to go back to renderer data structures all the time then I think they are “doing it wrong”. Those get()s will skip a frame with every request and they open up the potential deadlock situations that otherwise didn’t exist. If we’re going to be prescriptivist then we should prescribe the safest path.



Future.get() is only safe if it is the only way you ever do threading in your whole application and you never share any resources except through the render thread.

@pspeed said:
And never mind that you skip at least one frame per-request for potentially no good reason...

You should probably say that this only accounts for reading data from the OpenGL thread from the other thread, not using the Future to get the result on the OpenGL thread like I suggested. I thought you were talking about that.

In the example I gave the get() is only used on the thread that also dispatches the Callable and its checked for isDone() before so what you say cannot happen, even if the Callable code also blocks on a Future to the OpenGL thread. If you add no other threads theres no way this results in a deadlock situation.

It is only safe if you never use synchronize... anywhere.
Future.get() is only safe if it is the only way you ever do threading in your whole application and you never share any resources except through the render thread.

I definitely say in the example that the logic should be on the OpenGL thread so that is obviously where the resources are shared. Again, thats the main reason why I propose this, you do your logic on the OpenGL thread. If you start having a thread for managing objects locations, another for loading objects and another for generating geometry and then try to integrate all that to work properly thats where the issues you mention start. One thread decides it needs some location, reads it, which causes another thread to request information that the previous thread is supposed to update..

So I don't think theres any need to "be afraid" of Futures, if you mess it up by using them you'd mess it up in another way sooner or later too. I think they also provide a good analogy for the task and the fact that the current thread (which is left with only the Future) has only access to this kind of information (is my task done?).

I generally agree that full async is the best way though. Just queue the calls to the right thread then queue it again to the main thread after its been executed and this way the sequential logic is kept as well. But when you have a runloop-based application anyway I think its a good practice to treat tasks like actual tasks.

I see. It’s the other direction I fear… background thread queuing a callable for the render thread and waiting for the result. I think I misread the example every time in a “tl;dr” kind of way. I seem to always skim it and lose track in all of the anonymous inner class overhead, see that something is asking for a position, and never realize it’s the render thread asking for some other position.



The “isDone() then get()” polling is alright… but then you might as well have enqueued your response and save some code. And avoid the really tempting “don’t bother to check isDone() since get() will just wait anyway” approach.



I think the double dispatch queue should be preferred, ie: fully asynchronous. It also lends itself well to more advanced event queuing where you enqueue your own data instead of callables… and potentially manage the amount you process per frame to avoid frame drops. For example, adding more than one node per frame in Mythruna seems to drop a lot of frames so I limit it to one add per frame.



For anyone looking for the next step, I think it’s a more natural progression than from the isDone()/get() polling… and you have less book-keeping to do anyway, I guess.