Concurrent modification exceptions thrown when dynamically removing or adding controllers

[OliverColeman]

Methods in ControllerGroup that modify or iterate over the ControllerList should be synchronized to prevent concurrent modification exceptions when dynamically removing or adding controllers. For example code like:

    cp5 = new ControlP5(applet);
    group = cp5.addGroup(name);
    ...
    Toggle toggle = cp5.addToggle(label).setGroup(group);
    ...
    cp5.remove(toggle);
    group.remove(toggle);

executing in a separate thread can result in:

java.util.ConcurrentModificationException
    at java.util.Vector$Itr.checkForComodification(Vector.java:1184)
    at java.util.Vector$Itr.next(Vector.java:1137)
    at controlP5.ControllerGroup.drawControllers(Unknown Source)
    at controlP5.ControllerGroup.draw(Unknown Source)
    at controlP5.ControlWindow.draw(Unknown Source)
    at controlP5.ControlWindow.draw(Unknown Source)
    at sun.reflect.GeneratedMethodAccessor4.invoke(Unknown Source)
    at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
    at java.lang.reflect.Method.invoke(Method.java:498)
    at processing.core.PApplet$RegisteredMethods.handle(PApplet.java:1398)
    at processing.core.PApplet$RegisteredMethods.handle(PApplet.java:1391)
    at processing.core.PApplet.handleMethods(PApplet.java:1585)
    at processing.core.PApplet.handleDraw(PApplet.java:2416)
    at processing.awt.PSurfaceAWT$12.callDraw(PSurfaceAWT.java:1527)
    at processing.core.PSurfaceNone$AnimationThread.run(PSurfaceNone.java:316)

[GoToLoop]

synchronized is gonna reasonably slow down access to all of those methods.
And concurrency is considered an advanced technique among Processing users.
If we decide to apply such techs, it's our responsibility to use synchronized in our own sketches at the sections which they are needed.

[GoToLoop]

at java.util.Vector$Itr.next(Vector.java:1137)

It's strange this library relies on Vector container.
That class was @deprecated æons ago due to the fact it was somewhat already synchronized! O_o

[OliverColeman]

I did a bit of research and it sounds like (as with a lot of things Java) performance of synchronization was an issue in earlier JVMs but is unlikely to be in modern JVMs except for extreme cases. For the typical case of a single threaded application (as you indicate is usually the case in Processing) it appears the performance cost would be negligible: http://www.oracle.com/technetwork/java/6-performance-137236.html#2.1.1

"concurrency is considered an advanced technique among Processing users." Perhaps because handling concurrency is an advanced issue, and that Processing is intended for non-advanced users, it should not be up to the user to deal with it. There are cases where concurrency can become an issue without the user being aware of it. For example the reason I came across this issue is because we're developing a Processing library that automatically generates sets of ControlP5 Controllers based on data from a file or other data source. The library uses a thread to monitor changes to the data source and sometimes must update the available Controllers in response. Ideally the synchronization of this process with the rendering of control elements would be hidden from the user.

I'm not sure how I'd go about externally synchronizing the with the automatic rendering of control elements, other than by synchronizing the draw method in a sketch (which seems like a very coarse grained synchronization). Is there some way to prevent control elements from rendering automatically and rendering them manually (thus being able to synchronize just that portion of the rendering process)?

[OliverColeman]

Yes, I was also very surprised to see Vector used! I assumed the library must be fairly old. My first attempt at a fix for the issue I'm having was to replace these with an ArrayList wrapped via Collections.synchronizedList(), but of course this doesn't prevent modifications while an Iterator is in use.

[GoToLoop]

... was to replace these with an ArrayList wrapped via Collections.synchronizedList(), ...

Like I've mentioned, Java deprecated class Vector b/c it was unnecessarily synchronized everywhere.
If that's a desirable behavior, there's not much diff. between using straight Vector & applying Collections.synchronizedList() over an ArrayList.

[GoToLoop]

• Main problem I see in your pull is that you're spreading synchronized even to methods w/o loops!
• I also see you're applying synchronized in order to safeguard ControllerGroup::controllers field.
• And ControllerGroup::controllers is of datatype ControllerList.
• After some look up I've found out class ControllerList is merely a wrap over 2 Vector containers.
• Given they're of datatype Vector, it means they're already synchronized.
• The only thing lacking then is to always safeguard them when they're being iterated.
• Though I dunno much about this library, I'm gonna try to come up w/ something more precise as a counter-proposal to yours.

[OliverColeman]

"Like I've mentioned, Java deprecated class Vector b/c it was unnecessarily synchronized everywhere."

Indeed, I later remembered that the old Vector class was in fact already liberally synchronized and so my first attempt was completely pointless. ;)

"you're spreading synchronized even to methods w/o loops!"

Yes I realised at 3am last night that I'd synchronized every method dealing with the controllers field, including ones that don't modify or iterate over the lists in ControllerList, oops. :P I figured if the general approach was approved then I'd update the request.

"Given they're of datatype Vector, it means they're already synchronized. The only thing lacking then is to always safeguard them when they're being iterated. Though I dunno much about this library, I'm gonna try to come up w/ something more precise"

Yes, two Vectors in ControllerList. If synchronisation is such a performance killer (perhaps it's not given the library has been using Vectors the whole time ;) ), perhaps the thing to do is replace the Vectors with (unsynchronized) ArrayLists, and synchronize as necessary in ControllerGroup (and anything else that uses ControllerList, if necessary)?

[GoToLoop]

... is replace the Vectors with (unsynchronized) ArrayLists, and synchronize as necessary in ControllerGroup...

That's a much more refined approach. But it can be modified later in some follow-up pull request at any time. But now I'm gonna focus on ControllerList class only.

[OliverColeman]

Okay. In the meantime, is there a way to prevent control elements from rendering automatically and rendering them manually (thus being able to externally synchronize just that portion of the rendering process)?

[GoToLoop]

In order for synchronized to work, all parts which need it gotta use it.
If we synchronized our own code, but the library isn't doing as well for the dangerous parts, it's all in vain.

You were right that synchronized needs to go into the library for multi-threading sketch.
However I believe there's a workaround: place all your mutable code under sketch's "Animation Thread".

Let's say your other Thread wants to run cp5.remove(toggle);.
Instead use a boolean variable, let's call it removeRequested, and set it to true.
Inside draw() check for it. Set it back to false and run cp5.remove(toggle);.

This approach emulates a single-threaded sketch, b/c everything which would crash under a diff. Thread is transferred to sketch's "Animation Thread".

[OliverColeman]

The workaround sounds workable, good idea, thanks. :) I've just figured out how to hook into the "Animation Thread" before it calls draw, and how ControlP5 automatically draws its controllers, so will use the same mechanism to implement the workaround invisible to the user. I'm new to Processing.

[sojamo]

Hi, I am currently not able to follow up on this immediately but will look into it. The library started all the way back in 2005, so it is fairly old but has been extended, maintained, convoluted and adapted to quite a lot of processing version over the years.
The vector class is indeed a left over from the past, instead of replacing it with an java.util.arraylist and keeping the concurrency issues in mind, a more favourable replacement would be CopyOnWriteArrayList to avoid making use of synchronised. Most of the time you will be reading from this list and occasionally modify it by adding or removing elements. Reading in almost all cases outnumbers writing, hence CopyOnWriteArrayList would be the preferred choice. There is no performance overhead when reading from that list. When writing to it, it makes a backup copy to deal with concurrency.

[GoToLoop]

Counter-proposal done. Lotsa refactors there: #73

[damianfallas]

I found this works:

noLoop();
//ControlP5 modifications
loop();

[Neurogami]

This is still a problem. Tried the noLoop/loop hack but even that doesn't help.

Is this a "won't fix" issue? If so, then at least I and others would know to look for a different GUI library.