ConcurrentModificationException in CommandStack

[ptziegler]

This error is thrown when e.g. adding an entry from the palette. Though weirdly enough, only when executed from within RCPTT. My guess is that this error always shows up when one attempts to modify the stack within a CommandStackEventListener.

Regression introduced with https://github.com/eclipse/gef-classic/commit/02a5567fb1d33f65e0addbee249801e97c230c6b#diff-ad3f9900ec82240ffca477232d57e365fc7f1d1d780c40f0ed7ef24475b5364d

java.util.ConcurrentModificationException: null
    at java.util.ArrayList.forEach(ArrayList.java:1598) ~[?:?]
    at org.eclipse.gef.commands.CommandStack.notifyListeners(CommandStack.java:362) ~[org.eclipse.gef_3.18.0.202405290843.jar:?]
    at org.eclipse.gef.commands.CommandStack.execute(CommandStack.java:245) ~[org.eclipse.gef_3.18.0.202405290843.jar:?]
    at org.eclipse.gef.tools.AbstractTool.executeCommand(AbstractTool.java:437) ~[org.eclipse.gef_3.18.0.202405290843.jar:?]
    at org.eclipse.gef.tools.AbstractTool.executeCurrentCommand(AbstractTool.java:449) ~[org.eclipse.gef_3.18.0.202405290843.jar:?]
    at org.eclipse.gef.tools.CreationTool.performCreation(CreationTool.java:283) ~[org.eclipse.gef_3.18.0.202405290843.jar:?]
    at org.eclipse.gef.tools.CreationTool.handleButtonUp(CreationTool.java:198) ~[org.eclipse.gef_3.18.0.202405290843.jar:?]
    at org.eclipse.gef.tools.AbstractTool.mouseUp(AbstractTool.java:1216) ~[org.eclipse.gef_3.18.0.202405290843.jar:?]
    at org.eclipse.gef.EditDomain.mouseUp(EditDomain.java:288) ~[org.eclipse.gef_3.18.0.202405290843.jar:?]
    at org.eclipse.gef.ui.parts.DomainEventDispatcher.dispatchMouseReleased(DomainEventDispatcher.java:433) ~[org.eclipse.gef_3.18.0.202405290843.jar:?]
    at org.eclipse.draw2d.LightweightSystem$EventHandler.mouseUp(LightweightSystem.java:598) ~[org.eclipse.draw2d_3.16.0.202405290843.jar:?]
    at org.eclipse.swt.widgets.TypedListener.handleEvent(TypedListener.java:237) ~[org.eclipse.swt.win32.win32.x86_64_3.126.0.v20240528-0813.jar:?]
    at org.eclipse.swt.widgets.EventTable.sendEvent(EventTable.java:91) ~[org.eclipse.swt.win32.win32.x86_64_3.126.0.v20240528-0813.jar:?]
    at org.eclipse.swt.widgets.Display.sendEvent(Display.java:4285) ~[org.eclipse.swt.win32.win32.x86_64_3.126.0.v20240528-0813.jar:?]
    at org.eclipse.swt.widgets.Widget.sendEvent_aroundBody2(Widget.java:1160) ~[org.eclipse.swt.win32.win32.x86_64_3.126.0.v20240528-0813.jar:?]
    at org.eclipse.swt.widgets.Widget$AjcClosure3.run(Widget.java:1) ~[?:?]
    at org.eclipse.rcptt.tesla.recording.aspects.RecordingAspect.ajc$around$org_eclipse_rcptt_tesla_recording_aspects_RecordingAspect$2$2f127892proceed(RecordingAspect.aj:96) ~[org.eclipse.rcptt.tesla.recording.aspects.swt_2.5.5.202406011405.jar:?]
    at org.eclipse.rcptt.tesla.recording.aspects.RecordingAspect.ajc$around$org_eclipse_rcptt_tesla_recording_aspects_RecordingAspect$2$2f127892(RecordingAspect.aj:120) ~[org.eclipse.rcptt.tesla.recording.aspects.swt_2.5.5.202406011405.jar:?]

[ptziegler]

@azoitl I really don't think we can do any non-trivial code changes to GEF anymore, unless we drastically increase the test coverage...

[azoitl]

@azoitl I really don't think we can do any non-trivial code changes to GEF anymore, unless we drastically increase the test coverage...

I totally agree. Not yet sure how we get there. But given issues like this one I see no other chance.

The problem I see here is not that the command stack is modified but the list of command stack listeners is modified. So I assume that one of the CommandStackListeners is adding or removing a commandStackListener in the commandStackChanged() or in stackChanged not sure which of the two interfaces is used. As I changed from a for to a foreach this is now a problem. I can change that back. But for me it would be interesting if this is a good behavior of a listener?

PS: you managed to get RCPTT working with the latest Eclipse?

[ptziegler]

As I changed from a for to a foreach this is now a problem. I can change that back.

I've already fixed it locally by iterating over a copy of the listeners. I can create a PR later today.

But for me it would be interesting if this is a good behavior of a listener?

It definitely is not. But this change is breaking client code, which is why I don't think it can stay like this.

you managed to get RCPTT working with the latest Eclipse?

You have to use the nightly build, in order to get the fixes for https://github.com/eclipse/org.eclipse.rcptt/issues/30 and https://github.com/eclipse/org.eclipse.rcptt/issues/79. Otherwise, it's humming nicely.

I totally agree. Not yet sure how we get there. But given issues like this one I see no other chance.

We have several examples we could repurpose as integration tests. Though they require interacting with the Eclipse IDE, which is why I don't think it's feasible to do via JUnit. It would, however, be possible to use other replay tools such as SWTBot or again, RCPTT.

[ptziegler]

It would, however, be possible to use other replay tools such as SWTBot or again, RCPTT.

Hm... while I'm a lot more familiar with RCPTT, I think we're better off with SWTBot, simply because it's more actively maintained. Both support GEF and are therefore an option.

I think that's something to pursue for the current release cycle.

[azoitl]

Having seen both in action, I also have a slight tendency to SWTBot. Eclipse 4diac had last year a Google Summer of Code student experimenting with SWTBot, this year there even seem to be two. I think I should mentor next year also someone for a GEF Classic topic.

[ptziegler]

I'll see if I can cook up something. At least from my perspective, this is something that urgently needs to be sorted out.

[azoitl]

I'll see if I can cook up something. At least from my perspective, this is something that urgently needs to be sorted out.

Yes this definitely can not wait till next year's GSoC.

[ptziegler]

I've noticed the same issue in the Logic example. I'll try to collect and fix them in a single PR:

java.util.ConcurrentModificationException
    at java.base/java.util.ArrayList$Itr.checkForComodification(ArrayList.java:1095)
    at java.base/java.util.ArrayList$Itr.next(ArrayList.java:1049)
    at org.eclipse.gef.examples.logicdesigner.rulers.LogicRulerProvider$1.propertyChange(LogicRulerProvider.java:47)
    at java.desktop/java.beans.PropertyChangeSupport.fire(PropertyChangeSupport.java:343)
    at java.desktop/java.beans.PropertyChangeSupport.firePropertyChange(PropertyChangeSupport.java:335)
    at java.desktop/java.beans.PropertyChangeSupport.firePropertyChange(PropertyChangeSupport.java:268)
    at org.eclipse.gef.examples.logicdesigner.model.LogicRuler.addGuide(LogicRuler.java:51)
    at org.eclipse.gef.examples.logicdesigner.model.commands.CreateGuideCommand.execute(CreateGuideCommand.java:47)
    at org.eclipse.gef.commands.CommandStack.execute(CommandStack.java:229)
    at org.eclipse.gef.tools.AbstractTool.executeCommand(AbstractTool.java:437)
    at org.eclipse.gef.tools.AbstractTool.executeCurrentCommand(AbstractTool.java:449)
    at org.eclipse.gef.internal.ui.rulers.RulerDragTracker.handleButtonUp(RulerDragTracker.java:114)
    at org.eclipse.gef.tools.AbstractTool.mouseUp(AbstractTool.java:1216)
    at org.eclipse.gef.tools.SelectionTool.mouseUp(SelectionTool.java:596)
    at org.eclipse.gef.EditDomain.mouseUp(EditDomain.java:288)
    at org.eclipse.gef.ui.parts.DomainEventDispatcher.dispatchMouseReleased(DomainEventDispatcher.java:433)
    at org.eclipse.draw2d.LightweightSystem$EventHandler.mouseUp(LightweightSystem.java:598)
    at org.eclipse.swt.widgets.TypedListener.handleEvent(TypedListener.java:237)
    at org.eclipse.swt.widgets.EventTable.sendEvent(EventTable.java:91)
    at org.eclipse.swt.widgets.Display.sendEvent(Display.java:5855)
    at org.eclipse.swt.widgets.Widget.sendEvent(Widget.java:1617)
    at org.eclipse.swt.widgets.Display.runDeferredEvents(Display.java:5065)
    at org.eclipse.swt.widgets.Display.readAndDispatch(Display.java:4517)
    at org.eclipse.e4.ui.internal.workbench.swt.PartRenderingEngine$5.run(PartRenderingEngine.java:1151)
    at org.eclipse.core.databinding.observable.Realm.runWithDefault(Realm.java:339)
    at org.eclipse.e4.ui.internal.workbench.swt.PartRenderingEngine.run(PartRenderingEngine.java:1042)
    at org.eclipse.e4.ui.internal.workbench.E4Workbench.createAndRunUI(E4Workbench.java:152)
    at org.eclipse.ui.internal.Workbench.lambda$3(Workbench.java:639)
    at org.eclipse.core.databinding.observable.Realm.runWithDefault(Realm.java:339)
    at org.eclipse.ui.internal.Workbench.createAndRunWorkbench(Workbench.java:546)
    at org.eclipse.ui.PlatformUI.createAndRunWorkbench(PlatformUI.java:173)
    at org.eclipse.ui.internal.ide.application.IDEApplication.start(IDEApplication.java:152)
    at org.eclipse.equinox.internal.app.EclipseAppHandle.run(EclipseAppHandle.java:208)
    at org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.runApplication(EclipseAppLauncher.java:143)
    at org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.start(EclipseAppLauncher.java:109)
    at org.eclipse.core.runtime.adaptor.EclipseStarter.run(EclipseStarter.java:439)
    at org.eclipse.core.runtime.adaptor.EclipseStarter.run(EclipseStarter.java:271)
    at java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:103)
    at java.base/java.lang.reflect.Method.invoke(Method.java:580)
    at org.eclipse.equinox.launcher.Main.invokeFramework(Main.java:668)
    at org.eclipse.equinox.launcher.Main.basicRun(Main.java:605)
    at org.eclipse.equinox.launcher.Main.run(Main.java:1481)
    at org.eclipse.equinox.launcher.Main.main(Main.java:1454)

[azoitl]

@ptziegler but this is a different list and also a different cause. I think this should be fixed in a separate PR. I can look into it if you want.

[ptziegler]

but this is a different list and also a different cause

It in a different part of the code, yes. But the source is again caused by a for loop being converted to a for-each loop. In any case, I've created https://github.com/eclipse/gef-classic/pull/460 for the exception thrown by the ruler.

[Phillipus]

Looking at SWT, JFace and other examples of adding/removing/notifying listeners they either use a for loop (as in first version of CommandStack), an iterator, or make a copy of the listener list when notifying. Isn't the latter simpler than using a CopyOnWriteArrayList?

[ptziegler]

Isn't the latter simpler than using a CopyOnWriteArrayList?

I assume that the events are fired much more often than listeners are added/removed from the list. My concern is that it creates a significant overhead, if a copy is created whenever the listeners are notified.

It should also make everything less error-prone, as I don't have to find all places where a for-each loop is used...

[Phillipus]

I assume that the events are fired much more often than listeners are added/removed from the list.

Yes. I've just researched this further and thought the same.

[Phillipus]

Seems that CopyOnWriteArrayList is the way to go. I notice that the JGit project uses it for listeners as well as other projects. Now I have to revisit all my listener code...

[azoitl]

Thank you @ptziegler and @Phillipus for this detailed analysis and discussion. This helped me a lot to better handle these cases in the future.