ConcurrentModificationException prevents opening the scene about artofillusion HOT 19 OPEN

And very strangely, I got the file back.

Just to test something I made a little change to Scenestarting on line 140:

    for (ObjectInfo obj : objects)
      //obj.getObject().sceneChanged(obj, this);
      System.out.println(obj.getObject());
  }

The file opened without problems, I changed the faulty script and saved....

I wonder why sceneChanged() is called for every object, at file open?? In a way nothing is changing -- it is being built for the first time.

Probably a try-catch block here, notifying the user?

from artofillusion.

@peteihis can You create Proof-of-Concept script to reproduce this issue?

from artofillusion.

I wonder why sceneChanged() is called for every object, at file open?? In a way nothing is changing -- it is being built for the first time.

Because these objects can depend on the time and location/orientation in the scene. The object will need to read these from the scene no matter what, there's no shortcut/init that would bypass doing exactly the same as sceneChanged() under the hood.

from artofillusion.

As regards the ConcurrentModificationException

The object needs to read information from the scene. Then I tried to return something into the scene to be able to look at the result (which I can not do inside the script).

Can you share a sample of what code did this? Object scripts, by definition, are supposed to only modify themselves.

from artofillusion.

There you go.

Illegal script.zip

The illegal line in Scripted curve is commented out for safety. When you 'uncomment' it, you'll need to run a tool script

window.updateImage();
window.rebuildItemList();

to see that it actually worked.

from artofillusion.

I get that this is totally agaist the way scripted objects are supposed to work but because "fools are so ingenious"...

from artofillusion.

Huh... I think I've made similar mistakes, but I don't think that I've ever tried to save/load the scene. I always went "Why didn't that show?" And then dug back in to realize that I was in the wrong kind of script...

Douglas Adams once said “A common mistake that people make when trying to design something completely foolproof is to underestimate the ingenuity of complete fools.”

I think this is more in the category "Scripting is mostly undocumented, and potentially dangerous."

However, I'd wonder if it might make sense for ObjectScripts to not have direct access to the scene. Maybe through a wrapper that only exposes the getXXX() stuff, so they can't try changing it?

from artofillusion.

ConcurrentModificationException

Actually it looks that any error in a ScriptedObject code may prevent opening the scene file at all.

It is possible to save a scene even when execution of a scripted object has failed and the views have fallen into "blank mode". Some times (like I did) the user might want to save that scripted object with the error and sort any problems out later but currently saving it is a very bad idea.

I got the file back with the same hack as before. Also there should be an improvement in running scripted objects that, when execution fails the rest of the UI would still be updated. What to do with the failing script then?...

• No big flashy dialogs declaring the error ... Just a humble marker in the object list would be sufficient. The error message window may be left as before. Sometimes the messages are not very helpful as the groovy runner usually has rather creative ideas on where the problem is but at least it tells what it is.
• The failed script should not be ran again unless it is modified (and if it still fails again should be marked not to run again.)
• And to the original problem try block to scene open, collecting all of the failures onto one note that is the displayed to the user.

How would that be for a plan?

However, I'd wonder if it might make sense for ObjectScripts to not have direct access to the scene.

Writing back to scene may not be what it was planned for but who knows what creative uses people can come up with. I don't see that possibility as a problem itself. Just how the UI & res tof it responds to it (which it doesn't).

from artofillusion.

And so much for that foolproof plan:

Putting the sceneChanged calls in Scene.setTime() into a try block helps with other problems and it allows the UI-redraw to be finished even if there is an error/exception in a ScriptedObject. However, it does not catch the ConcurrentModificationException at file load.

What I find a bit strange is the the program reaches the last line of setTime but it never returns to the next line of the call to setTimeon line 1444 of Scene.

ScriptedObject uses the sceneChanged of ObjectCollection and the program bounces around inside ObjectCollection quite a bit. I have yet to find the call that actually launches building the scripted object...

The CroovyScriptEngine though only catches CompilationFailedException ... which is my current best guess to where things go wrong.

from artofillusion.

Putting the setTime() call on Scene line 1444 in a try block catches the Concurrent... exception & allows the scene opening to be finished. It seems that this is the only case where java uses java.util.Vector$Itr.checkForComodification which throws the exception.

from artofillusion.

This catch just masks an issue at this moment only.
Once I load scene open Score panel and move time marker I see a lot of ConcurrentModificationException exceptions in log output and I see a lot of new 'New Curve' items in scene obejct list and Score animation seems to be completly broken for me.

Moreover this exception is not catched when line 142 rewritten with lambda expression this way...
objects.forEach(item -> item.getObject().sceneChanged(item, this));

from artofillusion.

You're right.

LayoutWindow.setTime() first sets time on scene. Then it starts to update the score, the item list and the UI but since the exception happens on the first line the rest will never happen.

This is a bit of a strange thing. The ScriptedObject does add the new object to the scene and only after that Java notices that it could have been a problem throwing the exception. Usually an exception means that the action causing it will not be completed.

An another strange bit: LogPlugin reports the concurrent exceptions in 3.1.0 but in my edited version, where I was testing to handle it, log does not react. I wonder if I have made little change somewhere that I don't remember any more? But the problem is still there, the animation score does not work and so on...

from artofillusion.

I'm not sure why you expected the lambda syntax to make a difference?

ScriptedObject uses the sceneChanged of ObjectCollection and the program bounces around inside ObjectCollection quite a bit. I have yet to find the call that actually launches building the scripted object...

It's actually in the constructor for ScriptedObjectController, which surprised me a bit. It spins up a separate thread to do the (potentially heavy) lifting.

The ScriptedObject does add the new object to the scene and only after that Java notices that it could have been a problem throwing the exception.

This is actually correct behavior according to the official documentation.

When iterating through a collection, nothing is supposed to modify the collection itself. (Add, Remove, or Reorder items) Any script that tries to add things to the scene during the update loop is breaking this rule.

This exception is, however, "Best Effort" meaning that the code to detect these kind of things is not guaranteed to work.

Also note: This script probably wouldn't do what you thought even if the runtime didn't catch the concurrent modification. It would add a new curve to the scene every time sceneChanged() is called, so you'd soon have a flock of orphaned curves, growing out of control.

TLDR: Adding items to the scene from an Object Script can't work, and I'd be suspicious of any attempts at modifying the scene at all. The question is "Where do we try to catch and recover from this?"

from artofillusion.

I'm not expected difference with lambda syntax... I see this difference in my code...

from artofillusion.

You mean that it does make a difference when you test?

If so, you're running into that "Best Effort" caveat. The Exception not being thrown does not mean that the underlying issue is not still there.

from artofillusion.

so you'd soon have a flock of orphaned curves

Exactly. That would be the expectation and that is what actually happens.

What ever the solution will eventually be, the first thing is that what ever stupidities the use may have saved, it still must be possible to open the file again. I've got one way to handle that. (Notifies the user about the problem too...)

For the moment I could live with something that reacts to the problem every time and notifies the user. The read-only scene wrapper might be an idea to think about and not even very difficult, but right now I'd just patch the obvious.

And a very different issue:

*** The following plugins were not loaded because their imports could not be resolved:
LogPlugin.jar

That's why I did not get alerts.

from artofillusion.

When iterating through a collection, nothing is supposed to modify the collection itself.

This pointed me the right direction.

from artofillusion.

I'm still not sure that the flock of curves will be what you expect.

Let's say that you have a script that adds an item based on time, such as at each full second. If you play the score three times, you might get up to three copies of the item, per each full second. You might also get less, as it's not guaranteed playing the score will render on each full second.

from artofillusion.

I'm still not sure that the flock of curves will be what you expect.

I the case that the script adds an object every time it is ran that is exactly what I'd expect.

If we start to speculate with what any user might come up with, we'd soon end up with a limitless number of ways the script can edit the scene. The question is how to avoid a runtime error that prevents opening the file of blanks the screen without the user having any idea why.

What I've got so far

• Makes sure that the file can be opened again (Avoids this particular runtime error)
• The user is given a warning in the case that the problem is detected during opening the file or OK-ing the script.
• The changes done on the scene are made visible immediately (So the user will be aware of what is happening)
• The UI update cycle will not be interrupted by the error (So the user is less likely to panic. I was probably baffled pretty good when I did something like this the first time and the UI did not respond.)

I think that's pretty much as far as we can take it. To me adding more warnings would be more just annoying rather than informative and then it would easily lead to adding "Ignore this warning" buttons and so on.

EDIT -- got a bit carried away -- /EDIT And anyway this should be a rare(ish) incident.

from artofillusion.