Community
Participate
Working Groups
Clicking compare with base opens the compare editor twice and leaves the following in the log: org.eclipse.swt.SWTException: Failed to execute runnable (java.util.ConcurrentModificationException) at org.eclipse.swt.SWT.error(SWT.java:4361) at org.eclipse.swt.SWT.error(SWT.java:4276) at org.eclipse.swt.widgets.Synchronizer.runAsyncMessages(Synchronizer.java:138) at org.eclipse.swt.widgets.Display.runAsyncMessages(Display.java:4144) at org.eclipse.swt.widgets.Display.readAndDispatch(Display.java:3761) at org.eclipse.ui.internal.Workbench.runEventLoop(Workbench.java:2701) at org.eclipse.ui.internal.Workbench.runUI(Workbench.java:2665) at org.eclipse.ui.internal.Workbench.access$4(Workbench.java:2499) at org.eclipse.ui.internal.Workbench$7.run(Workbench.java:679) at org.eclipse.core.databinding.observable.Realm.runWithDefault(Realm.java:332) at org.eclipse.ui.internal.Workbench.createAndRunWorkbench(Workbench.java:668) at org.eclipse.ui.PlatformUI.createAndRunWorkbench(PlatformUI.java:149) at org.eclipse.ui.internal.ide.application.IDEApplication.start(IDEApplication.java:124) at org.eclipse.equinox.internal.app.EclipseAppHandle.run(EclipseAppHandle.java:196) at org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.runApplication(EclipseAppLauncher.java:110) at org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.start(EclipseAppLauncher.java:79) at org.eclipse.core.runtime.adaptor.EclipseStarter.run(EclipseStarter.java:353) at org.eclipse.core.runtime.adaptor.EclipseStarter.run(EclipseStarter.java:180) at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:57) at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) at java.lang.reflect.Method.invoke(Method.java:601) at org.eclipse.equinox.launcher.Main.invokeFramework(Main.java:629) at org.eclipse.equinox.launcher.Main.basicRun(Main.java:584) at org.eclipse.equinox.launcher.Main.run(Main.java:1438) at org.eclipse.equinox.launcher.Main.main(Main.java:1414) Caused by: java.util.ConcurrentModificationException at java.util.ArrayList$Itr.checkForComodification(ArrayList.java:819) at java.util.ArrayList$Itr.next(ArrayList.java:791) at org.eclipse.mylyn.reviews.core.spi.remote.emf.RemoteEmfConsumer$ConsumerAdapter.notifyChanged(RemoteEmfConsumer.java:81) at org.eclipse.emf.common.notify.impl.BasicNotifierImpl.eNotify(BasicNotifierImpl.java:374) at org.eclipse.emf.common.notify.impl.NotificationChainImpl.dispatch(NotificationChainImpl.java:98) at org.eclipse.emf.common.notify.impl.NotificationChainImpl.dispatch(NotificationChainImpl.java:86) at org.eclipse.mylyn.reviews.core.spi.remote.emf.RemoteEmfConsumer.applyModel(RemoteEmfConsumer.java:252) at org.eclipse.mylyn.reviews.core.spi.remote.JobRemoteService$2$1.run(JobRemoteService.java:77) at org.eclipse.swt.widgets.RunnableLock.run(RunnableLock.java:35) at org.eclipse.swt.widgets.Synchronizer.runAsyncMessages(Synchronizer.java:135) ... 23 more
I don't know why this started happening today, but it happens every time.
Review: https://git.eclipse.org/r/#/c/14802
From the review in the context of RemoteEmfConsumer: It appears that almost all access to remoteEmfObservers is synchronized. I don't see any need to make it a CopyOnWriteArrayList. That said, I think it's the better choice than synchronizing access. The latter is particularly problematic since the lock is held while observers are notified which can easily lead to dead locks. I don't know what the intended semantics are so it's hard to say if removing the synchronized statement would be correct but it would at least remove the risk of deadlocks.So... let's make that change *and* remove all synchronized blocks around remoteEmfObservers. Miles?
In the latest patch set I removed synchronized blocks for remoteEmfObservers and dispose the compare UI observer directly. This seems to be working fine.
Created attachment 233785 [details] mylyn/context/zip
Tomek, were you able to reproduce the issue?
(In reply to comment #6) > Tomek, were you able to reproduce the issue? Yes, and I can confirm it's gone with the latest patch applied.
Fixed in 2925fa0c6e1f4c9ecf45d65acc3e4cd64f429e78
(In reply to comment #3) > From the review in the context of RemoteEmfConsumer: > > It appears that almost all access to remoteEmfObservers is synchronized. I don't > see any need to make it a CopyOnWriteArrayList. > > That said, I think it's the better choice than synchronizing access. The latter > is particularly problematic since the lock is held while observers are notified > which can easily lead to dead locks. I don't know what the intended semantics > are so it's hard to say if removing the synchronized statement would be correct > but it would at least remove the risk of deadlocks.So... let's make that change > *and* remove all synchronized blocks around remoteEmfObservers. > > Miles? Yeah, I'm afraid that this probably *wasn't* the way to go. :D See bug 413480 comment 1. Generally, when you start playing around w/ synchronization issues in this way in EMF, you're really just playing Whack-A-Mole. We have to get at the root cause. I'm going to reopen as when we fix bug 413480, we'll still need a decent fix for this one.
I'm marking this as resolved until we have indication that the fix has caused a problem. The change did not cause the problem in 413480.
