Race condition in AppState/FlyByCamera?

What I observed is that FlyByCamera.setEnabled calls InputManager.setCursorVisible, which uses a nonatomic check&update. I suspect that’s a race condition if FlyByCamera.setEnabled is called through AppState from outside the rendering thread (say, from the AWT thread).

I’m not sure how to best verify how serious this is as a problem.

Hey, maybe my assumptions about what is supposed to be called from where are wrong, and setEnabled is supposed to be called exclusively from the render thread. Where do I turn for docs on such issues?

App states should always be called from the renderer’s thread by using something along the line of:



[java]

stateManager.getState(SomeClass.class).setEnabled(true);

[/java]



Technically you can call it from anywhere I guess, but prevent headaches and don’t. :wink:

1 Like

The opengl update loop works pretty much the same as the swing update loop. Like you have to queue any modification to a swing widget via SwingUtilities.invokeLater() you also have to use app.enqueue() with the update loop (and all things attached). If this is new to you read up on the topic (for swing and jme) and better check back your swing apps :wink:

Edit: The big “Documentation” link on the top of this page or pressing “F1” in the SDK is a good start for some documentation ^^

You can attach or detach an app state from any thread but enabling is almost always using a non-volatile, non-atomic field… so best to do it from the render thread.



In this case, since FlyByCamera never provides any threading guarantees for setEnabled() then you should always call that from the render thread.

1 Like

Thanks to @madjack and @pspeed for useful answers. I’ll take them as “informed hearsay”, which is a good start.

Something “official” would still be helpful, for two reasons:

  1. It would help me distinguish real race conditions from misinterpretations of the code.
  2. I’d like to write an SVN patch that adds the info to AppState’s javadoc. This would have two positive effects:

    2a) The next person who runs into that question won’t come here and waste everybody’s time getting that cleared up.

    2b) The following scenario won’t happen: Somebody else runs into the same question, doesn’t ask but simply guesses, gets into all kinds of trouble, fails the project, and starts to badmouth jME because it’s “full of race conditions”.



    @normen: Please stop wasting everybody’s time and getting on my nerves; just don’t respond unless you have something constructive to say.

    Demonstrating your oh-so-competence by ridiculing people about what F1 and reading the sources for it generally not considered constructive (in case you didn’t notice: it’s also an insult, and considered highly unprofessional by everybody except Gavin King).
@toolforger said:
Thanks to @madjack and @pspeed for useful answers. I'll take them as "informed hearsay", which is a good start.
Something "official" would still be helpful, for two reasons:
1) It would help me distinguish real race conditions from misinterpretations of the code.
2) I'd like to write an SVN patch that adds the info to AppState's javadoc. This would have two positive effects:
2a) The next person who runs into that question won't come here and waste everybody's time getting that cleared up.
2b) The following scenario won't happen: Somebody else runs into the same question, doesn't ask but simply guesses, gets into all kinds of trouble, fails the project, and starts to badmouth jME because it's "full of race conditions".


It is more efficient to document the places that it _IS_ ok to do things from the non-open GL thread because in general the default is that you must do everything from the Open GL thread.

@toolforger said:
@normen: Please stop wasting everybody's time and getting on my nerves; just don't respond unless you have something constructive to say.
Demonstrating your oh-so-competence by ridiculing people about what F1 and reading the sources for it generally not considered constructive (in case you didn't notice: it's also an insult, and considered highly unprofessional by everybody except Gavin King).


Except many users don't use F1 and don't look at the source or javadocs. We can't really tell until we mention it because 80% of the time the links are useful. I routinely paste single javadoc links as answers and get thanked.
1 Like

Ah. Now that’s important information that the standard mode of operation for jME code is that it’s running inside the OpenGL thread. That’s making the overall picture a whole lot clearer.

(I take it that “OpenGL thread” and “render thread” are synonyms, is that correct?)

I’d still like to see more docs on thread safety. It’s really hard to find your way around… and it’s not even true that most code is limited to the render thread, you can modify Spatials just fine before attaching them to a ViewPort, for example - Spatial does not need to be documented, the routine that transfers thread association is (for the Spatial-vs.-ViewPort case, that would be the function that attaches a Node to the ViewPort).

Also, for AppState, it would be really helpful if each AppState function documented which of its functions can be called from where. The JavaDoc on initialize, attach, cleanup, and detach was rather unhelpful, I had to look into its subclasses to find the comments that really explained their role. And I’m still somewhat fuzzy about the details - sure, one pair is for calling from anywhere and the other is reserved for calling from the render thread, but is one of them expected to be fast and the other not, i.e. how often are they (supposed to be) called? Is isEnabled supposed to be safely callable from outside the render thread or not? (Probably not, but it would be silly to safeguard against it if it’s unnecessary - unnecessary code tends to create gratuitous bugs.)



So I think these things should be documented:

  1. If some code is supposed to run in a specific thread and that specific thread only. E.g. initialize() and cleanup() have hardcore ties to the render thread (actually to whatever thread the AppState is working for, in theory, it could be used inside the AWT event loop with creative abuse).
  2. Is a function attaches some object to a different thread, which should now be the only one allowed to modify it. Attaching a Node to the ViewPort of an AppState should have that.
  3. If a class has functions that transfer thread attachment, all functions should, for each parameter state whether they change thread attachment for that parameter. This allows distinguishing between “this parameter remains owned by whatever thread owns it” and “the developer didn’t get around to document the thread reattachment behaviour for this parameter”. This could be relaxed to allow not documenting parameters where no race conditions can exist: those of immutable types, and those of types that are documented as threadsafe.



    If there’s interest, I can set up an example of that. I’d be posting a modified AppState.java since that’s the one that I understand best.



    On users that don’t use F1: Yes these do exist, even among programmers, but wait until they ask really dumb questions before assuming that. normen’s wording was really grating.

    And he knew already that I was reading the sources, and more than one level deep, so he could have know I’m a bit more knowledgeable than the standard noob.

    (What ticked me off is that this isn’t the first time he’s being unhelpful or condescending towards me. I smell a general trait here.)

If you felt ridiculed you should maybe think about why. I don’t remember the name of every noob that comes through here but I’ll remember yours now and never answer you again, no problem. If you don’t do your homework its always such a drag anyway I guess. I told you swing works the same and theres docs on that too. Your lengthy text shows that you didn’t read any of them and probably do your threading in all your swing apps wrong as I “condescendingly” indicated before as well. I smell a general trait of lazyass mixed with googling problems.

1 Like
@toolforger said:
Ah. Now that's important information that the standard mode of operation for jME code is that it's running inside the OpenGL thread. That's making the overall picture a whole lot clearer.
(I take it that "OpenGL thread" and "render thread" are synonyms, is that correct?)
I'd still like to see more docs on thread safety. It's really hard to find your way around... and it's not even true that most code is limited to the render thread, you can modify Spatials just fine before attaching them to a ViewPort, for example - Spatial does not need to be documented, the routine that transfers thread association is (for the Spatial-vs.-ViewPort case, that would be the function that attaches a Node to the ViewPort).
Also, for AppState, it would be really helpful if each AppState function documented which of its functions can be called from where. The JavaDoc on initialize, attach, cleanup, and detach was rather unhelpful, I had to look into its subclasses to find the comments that really explained their role. And I'm still somewhat fuzzy about the details - sure, one pair is for calling from anywhere and the other is reserved for calling from the render thread, but is one of them expected to be fast and the other not, i.e. how often are they (supposed to be) called? Is isEnabled supposed to be safely callable from outside the render thread or not? (Probably not, but it would be silly to safeguard against it if it's unnecessary - unnecessary code tends to create gratuitous bugs.)

I tried to cover that at the beginning of this class:
http://hub.jmonkeyengine.org/javadoc/com/jme3/app/state/AppStateManager.html
@toolforger said:
So I think these things should be documented:
1) If some code is supposed to run in a specific thread and that specific thread only. E.g. initialize() and cleanup() have hardcore ties to the render thread (actually to whatever thread the AppState is working for, in theory, it could be used inside the AWT event loop with creative abuse).
2) Is a function attaches some object to a different thread, which should now be the only one allowed to modify it. Attaching a Node to the ViewPort of an AppState should have that.
3) If a class has functions that transfer thread attachment, all functions should, for each parameter state whether they change thread attachment for that parameter. This allows distinguishing between "this parameter remains owned by whatever thread owns it" and "the developer didn't get around to document the thread reattachment behaviour for this parameter". This could be relaxed to allow not documenting parameters where no race conditions can exist: those of immutable types, and those of types that are documented as threadsafe.

If there's interest, I can set up an example of that. I'd be posting a modified AppState.java since that's the one that I understand best.


Seriously, though. It's easy. If there are 900 methods in all of JME you can bet that 850 of them will be single threaded. JME is a single threaded scene graph. The things that can be called from multiple threads are vanishingly small. You can load an asset from any thread. You can attach a state from any thread. Pretty much everything else is single threaded... so once you give it to the OpenGL thread then you must forever call it there until it is no longer attached to that thread.

Otherwise you end up documenting every method of nearly every class (Vector3f, Node, Spatial, Quaternion)... the better part of some 1000 classes to all say the same thing "if this is attached to the open GL thread then you must call it from there". (A notable exception is SpiderMonkey which tries to be 100% thread safe because it is heavily multithreaded internally... but it sort of has to be.)

I don't know what path I originally took through the docs but I somehow knew this within 3 hours of using JME3 for the first time last year. I had used different scene graphs before (multithreaded and not multithreaded) so maybe I was already familiar with the limitations.
1 Like

There is a tutorial on threading that covers most of this (at least it gives you the big picture that you can then work from).

1 Like

@pspeed No, you don’t need to add docs to Quaternion etc. They are not threadsafe like any other class that does not take special precautions, so you don’t need that.



What needs to be documented is that AppState.attachRoot makes the attached Node owned by the rendering thread.

I don’t think there are that many routines of that kind - but those that change thread ownership should be notified.



BTW the AppState class doc is is a noble attempt but AppStateManager is already more complete.

I’d suggest replacing the AppState class doc with a reference to AppStateManager.

Or maybe move the doc from AppStateManager to AppState and do the reference the other way round :slight_smile:



It might also be useful to cast the docs in terms of states: unattached, initializing, running, terminating.

Then each function can be documented in terms of these states, and the lifecycle as a whole can just be mentioned in passing in the class header comment.

It would also make it easier to document that it’s okay (actually normal) to terminate and reinitialize an AppState. The current javadoc does not say that (or I overlooked it).



@zarch thanks for the hint, I had read that actually.

In 20/20 hindsight, I see why it didn’t make a lasting impression on me - the tutorials were all talking about how to live inside the render loop, but all the setup and initialization code lives outside the render loop, so I’m feeling a bit lost because the issues I’m facing right now are about when to initialize what and in which thread.

I guess that’s what I’m getting for rewriting SimpleApplication to better suit my use case.



----



Just a final round of normen… sorry for that, but such a short message and so many points that are plain wrong on so many levels and can’t be left standing uncommented.

Anybody who’s not interested in that flame can simply stop reading here, nothing project-relevant is below this point.


@normen said:
If you felt ridiculed you should maybe think about why.


I'm not having this kind of problem with 99% of people.
Maybe you should think why you're in the 1% group that I don't get along with.
You might also want to think about how many people who feel the same like me have already left in disgust, but without saying anything. For every one like me who's vocal about such things, there's a multitude of quiet dissenters - that's a well-known society fact.

@normen said:I don't remember the name of every noob that comes through here


Insult noted.
Maybe you should think about whether it's wise to insult random guys.

@normen said:
If you don't do your homework its always such a drag anyway I guess.


It would be if I didn't, but I do. In fact you have been the only person I ever met who insisted on coming over with preconceptions about my needs and problems instead of simply listening to what I say, and ask if something isn't clear.
Maybe you should think about that.

@normen said:
I told you swing works the same and theres docs on that too. Your lengthy text shows that you didn't read any of them and probably do your threading in all your swing apps wrong as I "condescendingly" indicated before as well. I smell a general trait of lazyass mixed with googling problems.


@normen said:
I told you swing works the same and theres docs on that too.


Unfortunately, such general statements don't help much. For example, Swing has four different message queues that are being processed at different priorities. More on topic, it has a SwingUtils.isEventDispatchThread. Your statement is a bit too general to infer whether these details are exactly equivalent, roughly similar, or simply not there in jME.

@normen said:
Your lengthy text shows that you didn't read any of them and probably do your threading in all your swing apps wrong as I "condescendingly" indicated before as well. I smell a general trait of lazyass mixed with googling problems.


Pfft... notreading... threading issues... lazyass and googling problems... omigod, I'M SO UNWORTHY, now I know why all my apps are so crashing all over the place and I quite never get the problems sorted out...
Not.
I'm really wondering why you think you can read minds. If you attended classes for that, I recommend demanding your money back.

And, again, sorry to all innocent bystanders.
@toolforger said:
Insult noted.

Its not an insult, its a fact. You are a jme noob. I am also a noob in writing shaders for example if that pleases you. We can play your favorite game together and then you can call me a noob. But here you are the noob I'm afraid. You know that the translation of "noob" is "newbie", right?
@toolforger said:
It would be if I didn’t, but I do [my homework].

No you didn't, else you would have had questions about the swing event queue instead of just being pissed at me. Most people understand what I said and see it as a proper answer to your question.
@toolforger said:
Unfortunately, such general statements don't help much.

Its not a general statement. I said you have to use app.enqueue() with anything attached to the update loop like you have to use SwingUtilities.invokeLater() (which also specifies what queue I am talking about) for everything that is swing UI elements. You also won't find any notes on the single methods of swing widgets what you can call and what not because there as well most everything isn't "thread safe" except some things like refresh().
@toolforger said:
I'm really wondering why you think you can read minds. If you attended classes for that, I recommend demanding your money back.

I can't, I can just take what you post here and it underlines what I say. But be honest, do you modify a swing item (e.g. a slider value) from another thread but the EDT in any of your applications? I did so when I started with swing and its wrong and causes problems so I thought I mention that.

So what exactly do you think you get out of the energy you put into this war with me if you are so experienced in how to cope with others that you want to give me tips on it?

Edit: And please, whatever issues you have with me generally, put my first post and your direct answer to me in relation please. I gave you the actual answer plus a hint at the docs which contain the info which made you +1 others that said so too, giving me again evidence for my "smell accusation".

Edit2: Couldn't keep this one to me:
@toolforger said:
Maybe you should think why you’re in the 1% group that I don’t get along with.

For every one like me who’s vocal about such things, there’s a multitude of quiet dissenters – that’s a well-known society fact.
@toolforger said:
@zarch thanks for the hint, I had read that actually.
In 20/20 hindsight, I see why it didn't make a lasting impression on me - the tutorials were all talking about how to live inside the render loop, but all the setup and initialization code lives outside the render loop, so I'm feeling a bit lost because the issues I'm facing right now are about when to initialize what and in which thread.


I'm not sure what you mean here. The tutorials tend to do everything in simpleInit() which is run on the render loop.

@toolforger said:
I guess that's what I'm getting for rewriting SimpleApplication to better suit my use case.


Yeah. SimpleApplication is misnamed. It should be called "ClassYouShouldDefinitelyExtendToMakeAGame". You have to be a JME expert to roll your own and even then you rewrite SimpleApplication almost exactly, eventually.
1 Like

@pspeed: Well, the more I looked at SimpleApplication’s code, the more I disliked it.



It started with FlyByCamera. Key bindings aren’t overridable.

Same goes for StatsAppState.

Also, a good deal of the logic was distributed across AppStates and SimpleAplication. That’s not self-contained OO design, not at all.



So I’m currently reworking SimpleApplication and moving all that global stuff into separate AppStates:

  • rootNode
  • guiNode
  • flyCam
  • stats and fps display



    Since it’s essentially just moving SimpleApplication code into AppState classes, this sounds neither hard nor like writing yet another SimpleApplication clone.



    I’ll see. To the very least, it will give me more insights into jME’s architecture.



    ----



    (Further arguments with normen cancelled. Neither normen nor me is going to change point of view, all relevant arguments have been exchanged, and I guess there’s little if any entertainment value left, so I’m stopping this.)
@toolforger said:
@pspeed: Well, the more I looked at SimpleApplication's code, the more I disliked it.

It started with FlyByCamera. Key bindings aren't overridable.
Same goes for StatsAppState.
Also, a good deal of the logic was distributed across AppStates and SimpleAplication. That's not self-contained OO design, not at all.


All of that used to be hard-coded right in SimpleApplication. I moved them out to app states precisely so that you could leave them out in your own SimpleApplication subclass.

Right, now that you mention it, I’m looking into the diffs and seeing these changes.



You may want to consider creating abstract GuiAppState and SceneAppState classes, and pushing any relevant code from initialize() and update() down into them.

At least that’s what I’m going to do for my Application subclass. :slight_smile:



I’m going to do a few more things: push down rootNode, guiNode, fpsText, guiFont, and flyCam, and rebuild the settings dialog as a Nifty screen.

That will allow me to use a vanilla Application, which I find ultra-cool because it removes a whole lot of special-case consideration from the project.

It might break too much compatibility for jME3 though.

Without a root node and a gui node… you have almost nothing. You almost might as well extend java.lang.Object.



I could think of no cases where one would want to leave out the root node and gui node so I left them in.

1 Like

The two things I don’t like about simple application are the fact that you can’t really modify the settings screen (I’d have liked to customize it a bit more than just changing the image and title) and that the flycam etc implementation is a bit tangled in at the moment…really there should be a “HumanCameraInterface” type interface that flycam etc implement and you then pass that into the super constructor (or set from init or something) on the simple application.

1 Like
@toolforger said:
I'm going to do a few more things: push down rootNode, guiNode, fpsText, guiFont, and flyCam, and rebuild the settings dialog as a Nifty screen.
That will allow me to use a vanilla Application, which I find ultra-cool because it removes a whole lot of special-case consideration from the project.
It might break too much compatibility for jME3 though.


The problem with doing settings as a nifty screen is that if you change to something your monitor can't display then you can't change it back. You also need to make sure your app either copes with settings changing on the fly (including things like turning on/off anti-aliasing etc) or force people to restart after changing settings...
@zarch said:
The two things I don't like about simple application are the fact that you can't really modify the settings screen...

SimpleApplication doesn't stop you from changing the settings screen, just overwrite start() and provide your own window instead of the JME default one. Providing a wholly customizable settings screen is way out of scope for the engine; it would be impossible to predict every desired look.

@rest-of-the-thread
If everything gets taken out of SimpleApplication, then it becomes next-to useless. It is there to boostrap what 95% of the users need. Don't want the root node? Just provide another and put it in an app state. You will probably end up with a couple root nodes anyways as you start using multiple view ports. But as pspeed said, you really need at least one.
1 Like