Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.

Bug 495962

Summary: ConcurrentModificationException in JavaExtensionsManager.loadJavaExtensions
Product: [Modeling] Sirius Reporter: Aurelien Pupier <apupier>
Component: CoreAssignee: Pierre-Charles David <pierre-charles.david>
Status: CLOSED FIXED QA Contact:
Severity: normal    
Priority: P5 CC: julien.dupont, pierre-charles.david
Version: 4.0.0Keywords: triaged
Target Milestone: 5.1.0   
Hardware: PC   
OS: Windows NT   
See Also: https://git.eclipse.org/r/103469
https://git.eclipse.org/c/sirius/org.eclipse.sirius.git/commit/?id=adaec6b4aab67d60ff93b54bae7ade95aa8fbadc
Whiteboard:

Description Aurelien Pupier CLA 2016-06-13 04:18:48 EDT
I don't know the steps to reproduce but here is the stacktrace:

!ENTRY org.eclipse.core.jobs 4 2 2016-06-07 15:08:52.502
!MESSAGE An internal error occurred during: "Refresh".
!STACK 0
java.util.ConcurrentModificationException
	at java.util.LinkedHashMap$LinkedHashIterator.nextNode(Unknown Source)
	at java.util.LinkedHashMap$LinkedKeyIterator.next(Unknown Source)
	at org.eclipse.sirius.common.tools.api.interpreter.JavaExtensionsManager.loadJavaExtensions(JavaExtensionsManager.java:500)
	at org.eclipse.sirius.common.tools.api.interpreter.JavaExtensionsManager.reloadIfNeeded(JavaExtensionsManager.java:243)
	at org.eclipse.sirius.common.acceleo.aql.business.internal.AQLSiriusInterpreter.evaluateExpression(AQLSiriusInterpreter.java:198)
	at org.eclipse.sirius.common.acceleo.aql.business.internal.AQLSiriusInterpreter.evaluate(AQLSiriusInterpreter.java:188)
	at org.eclipse.sirius.common.tools.internal.interpreter.AbstractInterpreter.evaluateString(AbstractInterpreter.java:113)
	at org.eclipse.sirius.tools.internal.interpreter.ODesignGenericInterpreter.evaluateString(ODesignGenericInterpreter.java:188)
	at org.eclipse.sirius.diagram.business.internal.metamodel.helper.DiagramElementMappingHelper.computeLabel(DiagramElementMappingHelper.java:116)
	at org.eclipse.sirius.diagram.business.internal.experimental.sync.DDiagramElementSynchronizer.computeLabel(DDiagramElementSynchronizer.java:833)
	at org.eclipse.sirius.diagram.business.internal.experimental.sync.DDiagramElementSynchronizer.refresh(DDiagramElementSynchronizer.java:602)
	at org.eclipse.sirius.diagram.business.internal.experimental.sync.DDiagramElementSynchronizer.refresh(DDiagramElementSynchronizer.java:505)
	at org.eclipse.sirius.diagram.business.internal.experimental.sync.DDiagramSynchronizer.handleKeptNode(DDiagramSynchronizer.java:1079)
	at org.eclipse.sirius.diagram.business.internal.experimental.sync.DDiagramSynchronizer.handleKeptAndNewNodesWithOrder(DDiagramSynchronizer.java:1021)
	at org.eclipse.sirius.diagram.business.internal.experimental.sync.DDiagramSynchronizer.refreshNodeMapping(DDiagramSynchronizer.java:914)
	at org.eclipse.sirius.diagram.business.internal.experimental.sync.DDiagramSynchronizer.refreshOperation(DDiagramSynchronizer.java:406)
	at org.eclipse.sirius.diagram.business.internal.experimental.sync.DDiagramSynchronizer.refresh(DDiagramSynchronizer.java:355)
	at org.eclipse.sirius.diagram.business.internal.sync.DDiagramSynchronizer.refresh(DDiagramSynchronizer.java:91)
	at org.eclipse.sirius.diagram.business.internal.dialect.DiagramDialectServices.refresh(DiagramDialectServices.java:259)
	at org.eclipse.sirius.business.internal.dialect.DialectManagerImpl.refresh(DialectManagerImpl.java:124)
	at org.eclipse.sirius.business.api.dialect.command.RefreshRepresentationsCommand.doExecute(RefreshRepresentationsCommand.java:122)
	at org.eclipse.emf.transaction.RecordingCommand.execute(RecordingCommand.java:135)
	at org.eclipse.emf.workspace.EMFCommandOperation.doExecute(EMFCommandOperation.java:119)
	at org.eclipse.emf.workspace.AbstractEMFOperation.execute(AbstractEMFOperation.java:150)
	at org.eclipse.core.commands.operations.DefaultOperationHistory.execute(DefaultOperationHistory.java:488)
	at org.eclipse.emf.workspace.impl.WorkspaceCommandStackImpl.doExecute(WorkspaceCommandStackImpl.java:208)
	at org.eclipse.emf.transaction.impl.AbstractTransactionalCommandStack.execute(AbstractTransactionalCommandStack.java:165)
	at org.eclipse.emf.transaction.impl.AbstractTransactionalCommandStack.execute(AbstractTransactionalCommandStack.java:219)
	at org.eclipse.sirius.diagram.ui.tools.internal.editor.DDiagramEditorImpl.launchRefresh(DDiagramEditorImpl.java:1243)
	at org.eclipse.sirius.diagram.ui.tools.internal.editor.DDiagramEditorImpl.access$2(DDiagramEditorImpl.java:1215)
	at org.eclipse.sirius.diagram.ui.tools.internal.editor.DDiagramEditorImpl$2.run(DDiagramEditorImpl.java:631)
	at org.eclipse.core.internal.jobs.Worker.run(Worker.java:55)
Comment 1 Julien Dupont CLA 2016-06-14 09:35:55 EDT
Hi Aurelien, 

Without use case reproduction it's almost impossible to correct the bug.

Could you give us steps to reproduce problem.

Thanks.

Regards,
Comment 2 Aurelien Pupier CLA 2016-06-14 09:37:38 EDT
As said previously, I don't know the steps to reproduce. I was going through the tutorial given @EclipseCon France and just had time to take the stacktrace.
Comment 3 Pierre-Charles David CLA 2016-06-14 09:47:58 EDT
I don't know the exact steps either, but I'm pretty sure I've already been hit by that stack too, and looking at the code it can clearly be made more robust regarding to this, so I'm marking this as triaged.
Comment 4 Pierre-Charles David CLA 2016-06-14 09:53:05 EDT
One possible hint for a reproduction scenario (which would be compatible with Aurelien's mention that he was following the EclipseCon tutorial) would be the properties view (installed in the package provided for the tutorial) which launches its own refresh at the same time as the diagram refresh. The properties view refresh code share the same interpreter, and programmatically register its own service class, which would concurently modify the JavaExtensionsManager.imports field, which corresponds to the LinkedHashMap in the stack.
Comment 5 Pierre-Charles David CLA 2016-09-12 08:54:19 EDT
Moving out of 4.1.0, as I have not seen it happen in a while, and have no clear reproduction scenario. From reading the code itself, it is clear that it *can* happen, but identifying user-level scenarios which could trigger it is not easy. I'm pushing it to 4.1.1 instead of 5.0.0 to try and have a look if time permits, but with no guarantee.
Comment 6 Eclipse Genie CLA 2017-08-22 11:25:26 EDT
New Gerrit change created: https://git.eclipse.org/r/103469
Comment 7 Pierre-Charles David CLA 2017-08-22 11:26:22 EDT
(In reply to Eclipse Genie from comment #6)
> New Gerrit change created: https://git.eclipse.org/r/103469

Analysis: The 'imports' HashSet is private, and we never leak a reference to it to anybody (getImports() returns an immutable copy). The 'for' loop where the CME occurs does not call (directly or not) any of the mutation methods which modify 'imports', so it must come from another thread hitting on such a (public) mutation method while we're iterating. It should be enough to synchonize all public methods which end up accessing to 'imports' (loadJavaExtensions() is already synchronized via reloadIfNeeded() which is its only caller).
Comment 9 Pierre-Charles David CLA 2017-08-28 10:54:35 EDT
Fixed (probably) by adaec6b4aab67d60ff93b54bae7ade95aa8fbadc.
Comment 10 Julien Dupont CLA 2017-09-20 05:58:45 EDT
Can not be verified: missing or incomplete reproduction information.
Comment 11 Pierre-Charles David CLA 2017-09-20 11:18:22 EDT
Steps to reproduce: we don't have any, the only check we can do is verify that the patch is safe. It's been merged for a while with no regression identified, si I'll mark this as validated myself.
Comment 12 Pierre-Charles David CLA 2017-11-08 03:37:51 EST
Available in Sirius 5.1.0, see https://wiki.eclipse.org/Sirius/5.1.0.